Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented ProgramCacheLayer, updated CI #15

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

mfep
Copy link

@mfep mfep commented Sep 28, 2023

  • Implemented a new ProgramCacheLayer which performs the transparent caching of OpenCL program binaries on the filesystem.
    • The cache lookup signature is based on the platform, device and the program's preprocessed source.
    • The majority of the caching logic is implemented in a ProgramCache static lib, which can be installed and consumed independently from the layer.
    • Unit tests are added, as well as the ProgramCacheLayer was tested via a subset of the CTS (basic, api and compiler suites), with multiple runtimes (Intel CPU, Nvidia).
    • More information about the interfaces and internals of the aforementioned components is provided via inline documentation.
    • New dependencies are added
      • googletest for unit testing
      • Boost::Wave preprocessing library
  • Added .gitignore file,
  • Added .clang-format (based on OpenCL-SDK's corresponding file) and format enforcing script.
  • Minor changes to source files to avoid warnings on all compilers tested in CI.
    • -Werror//WX is enabled in CI
  • Updated the CI (pre-release) script. Since the new layer required many changes to CI (e.g. new dependencies), it is practical that the CI update is part of the same PR.
    • Removed the previous separate workflows for Windows, MacOS and Linux. All CI scripts reside in the presubmit workflow now.
    • Added a job that checks the changed code for formatting conformance via git-clang-format.
    • Updated Linux CI to use a new container image
      • Most jobs in the matrix are based on Ubuntu 22.04
      • Compilers gcc-11, gcc-13, clang-14, clang-16 and gcc-9 on Ubuntu 20.04
      • Binary targets: x86-64 and x86 on Ubuntu 20.04
      • CMake versions: 3.26 and 3.16 on Ubuntu 20.04
      • CMake generators: Ninja Multi-Config, Unix Makefiles
    • Updated Windows CI, which runs on Windows Server 2022
      • MSVC toolsets: v142, v143, clang-cl (this latter with msbuild only)
      • Binary targets: x86-64, x86
      • CMake: system installed, currently 3.27
      • CMake generators: Ninja Multi-Config, Visual Studio 17 2022
      • Added the result checking of the external command invocations in the PowerShell snippets
    • Updated MacOS CI, which runs on MacOS 13
      • Compilers: Apple-Clang 14, gcc-11, gcc-13 (gcc with Ninja Multi-Config only)
      • Binary target: x86-64
      • CMake: system installed, currently 3.27
      • CMake generators: Ninja Multi-Config, XCode
    • Added Android CI, which is build only (virtualization is not available with the default GitHub runners)
      • Binary targets: x86-64, arm64
      • Android API levels: 19, 33
    • Added tests checking the consumption of the ProgramCache library from the install tree
    • Testing multiple ways of consuming the dependencies
      • via the system package manager: apt on Ubuntu, homebrew on MacOS, omitted on Windows and Android
      • via vcpkg
      • via FetchContent that is built directly in to the CMakeLists

This PR is considered to be complete, albeit review remarks and/or changes to related PRs might warrant minor updates.

@CLAassistant
Copy link

CLAassistant commented Sep 28, 2023

CLA assistant check
All committers have signed the CLA.

Copy link

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't build for me using the previous instructions in the README, so either a) the README needs to be updated, or b) whatever changed that caused the build to break needs to be reverted.

@mfep
Copy link
Author

mfep commented Dec 6, 2023

This didn't build for me using the previous instructions in the README, so either a) the README needs to be updated, or b) whatever changed that caused the build to break needs to be reverted.

Hi,

The project should still build with the instructions in the README. Albeit we have a few new dependencies employed by the new ProgramCacheLayer, those should be automatically picked by FetchContent if not found on the system.

There is a potential problem that I see, but it is not new with this PR. The CMake script assumes that the OpenCL-Headers are installed on the system.

Can you please elaborate on what error you got, and on what kind of system?

@bashbaug
Copy link

bashbaug commented Dec 8, 2023

Can you please elaborate on what error you got, and on what kind of system?

Sorry about that, I should have provided a better comment initially.

I'm testing on my Ubuntu 22.04 Linux system. I get an error right away generating build files from CMake:

$ cmake ..
-- The C compiler identification is GNU 11.4.0
-- The CXX compiler identification is GNU 11.4.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Could NOT find GTest (missing: GTest_DIR)
-- Could NOT find GTest (missing: GTEST_LIBRARY GTEST_INCLUDE_DIR GTEST_MAIN_LIBRARY) 
-- Fetching googletest
-- Found Python: /usr/bin/python3.10 (found version "3.10.12") found components: Interpreter 
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE  
CMake Warning at cmake/OpenCLHeadersCpp.cmake:2 (find_package):
  By not providing "FindOpenCLHeadersCpp.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "OpenCLHeadersCpp", but CMake did not find one.

  Could not find a package configuration file provided by "OpenCLHeadersCpp"
  with any of the following names:

    OpenCLHeadersCppConfig.cmake
    openclheaderscpp-config.cmake

  Add the installation prefix of "OpenCLHeadersCpp" to CMAKE_PREFIX_PATH or
  set "OpenCLHeadersCpp_DIR" to a directory containing one of the above
  files.  If "OpenCLHeadersCpp" provides a separate development package or
  SDK, be sure it has been installed.
Call Stack (most recent call first):
  cmake/Dependencies.cmake:4 (include)
  CMakeLists.txt:26 (include)


-- Fetching OpenCLHeadersCpp
-- Could NOT find Doxygen (missing: DOXYGEN_EXECUTABLE) 
CMake Error at build/_deps/openclheaderscpp-src/examples/src/CMakeLists.txt:11 (find_package):
  By not providing "FindOpenCLICDLoader.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "OpenCLICDLoader", but CMake did not find one.

  Could not find a package configuration file provided by "OpenCLICDLoader"
  with any of the following names:

    OpenCLICDLoaderConfig.cmake
    openclicdloader-config.cmake

  Add the installation prefix of "OpenCLICDLoader" to CMAKE_PREFIX_PATH or
  set "OpenCLICDLoader_DIR" to a directory containing one of the above files.
  If "OpenCLICDLoader" provides a separate development package or SDK, be
  sure it has been installed.


-- Configuring incomplete, errors occurred!

I'm following the instructions from the README:

mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=Release ../
make

@mfep
Copy link
Author

mfep commented Dec 11, 2023

I'm testing on my Ubuntu 22.04 Linux system. I get an error right away generating build files from CMake:

Thank you! This error originates from a quite long chain of problems. The direct source of it is the fact that with the current PR adds OpenCL-CLHPP as a dependency to OpenCL-Layers.

  1. The root of the problem is the following: to perform a default build of the OpenCL-CLHPP, OpenCL-ICD-Loader has to be installed on the system. Note that, the apt package ocl-icd-opencl-dev won't suffice, since it does not ship the OpenCLICDLoader.config. A simple fix could and should be issued in OpenCL-CLHPP to be able to depend the aforementioned Debian package to build.
  2. The above problem only comes up because, for some reason, the default build of OpenCL-CLHPP builds the example targets. This is discrepant from the build of the tests for example, because the tests only build if explicitly enabled, or if the project is the root project. I think that this behavior should be uniform, but changing that obviously breaks users.
  3. But even if OpenCL-CLHPP builds the examples by default, it is possible to disable that when consuming the CLHPP in OpenCL-Layers, however ugly that workaround is.

For now, I can propose 2 options:

Option A): Since the OpenCL-Headers and the OpenCL-ICD-Loader were required to be installed on the system to build the OpenCL-Layers even before this changeset, it is arguable that OpenCL-CLHPP would join this group. The FetchContent(OpenCL-CLHPP) should be removed in that case, so this problem goes away (from this project - the aforementioned problems should still be handled in the corresponding projects). The change should be indicated in the README - with the addition of the other two pre-existing but unmentioned dependencies.

Option B): Add the aforementioned fix (3.) to override BUILD_EXAMPLES when using FetchContent to get OpenCL-CLHPP in this project. Thereby, we wouldn't need a new dependency to be installed, and it would be compatible with the current version of OpenCL-CLHPP - it would still be beneficial to patch up the aforementioned problems separately.

@bashbaug please give input about the direction I should take. Thank you!

@bashbaug
Copy link

I think option (B) is an OK short-term fix. Can you please file an issue in the OpenCL-CLHPP project though, so we don't lose track of this?

(Would it be viable to only build the examples (and the docs?) by default when building the OpenCLHeadersCpp project directly? We do this already for most of our testing.)

@mfep
Copy link
Author

mfep commented Dec 13, 2023

I think option (B) is an OK short-term fix. Can you please file an issue in the OpenCL-CLHPP project though, so we don't lose track of this?

(Would it be viable to only build the examples (and the docs?) by default when building the OpenCLHeadersCpp project directly? We do this already for most of our testing.)

Fix pushed, issue filed: KhronosGroup/OpenCL-CLHPP#283

@Beanavil
Copy link
Member

Latest commits:

  • update Docker images so that the ones from the khronosgroup Docker Hub are used

  • fix MSVC compiler toolset version due to breaking change in the MSVC versioning (see cppblog)

    We have incremented the minor version number of the MSVC toolset from 19.39 (VS 2022 v17.9) to 19.40 (VS 2022 v17.10)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants