Skip to content

Samples and Utility libraries#37

Merged
MathiasMagnus merged 203 commits intoKhronosGroup:mainfrom
StreamHPC:develop_stream
Jan 25, 2022
Merged

Samples and Utility libraries#37
MathiasMagnus merged 203 commits intoKhronosGroup:mainfrom
StreamHPC:develop_stream

Conversation

@MathiasMagnus
Copy link
Copy Markdown
Collaborator

This PR ships a larger batch of samples and the initial set of C/C++ utility libraries and SDK libraries. This PR includes:

  • Temporary redirection of git submodules to the related Stream HPC SDK repos. This PR is dependent on changes found in:
  • References to existing and new chapters of the OpenCL-Guide:
  • The batch of samples include:
    • SAXPY (C & C++)
    • Reduction (C & C++)
    • Histogram soon (C++)
    • Blur (C)
    • Load/Save Binaries (C)
    • Conway's Game of Life (C++)
    • NBody (C++)
  • All samples are accompanied by README markdown detailing the purpose of the samples, their feature set, some of their internal working and the utilized API surface.
  • Two libraries are built within the SDK
    • OpenCLUtils(Cpp) which consumers of the SDK can use to facilitate some common OpenCL tasks
      • The utility libraries only depend on other OpenCL packages
    • OpenCLSDK(Cpp) which the samples use to deduplicate some of the common aspects of SDK samples, such as command-line parsing and window handling
      • The OpenCLSDK libraries have more dependencies, but nothing out of ordinary
      • The windowing library of choice is SFML, because it has explicit support for Android, and that is a platform the SDK wishes to target along with the native samples
  • Building the development packages only don't require any dependencies, a non-recursive clone with submodules is enough. This is tested in CI.

Some notes on the shape of the SDK samples and some of the design choices:

  • The C dependent libraries are bundled within the source tree. This was done because while Linux typically has these libraries available via the system package managers, users of Windows will have to rely on C/C++ package managers like Conan/Vcpkg/Hunter/etc. Once the availability of the deps improve, removing them from the repository may be appropriate.
    • Although it is a contentious topic, using Git submodules as a form of package management isn't ideal, nor is bundling deps in the source tree. While pkg managers gain popularity, best case scenario is catering to both crowds: those not using pkg managers but wanting a clean build experience and those who readily have custom built versions of all their deps, often newer than those bundled.
    • Being an author of various Vcpkg ports myself and maintaining HIP libraries, I know for a fact that self-hosting and wanting to provide a clean build experience for 3 platforms has non-zero cost. Offloading that pain to package manager maintainers is the best and easiest approach.
      • Do note that once the CMake rework of the SDK goes through, one of my first tasks will be to simplify the OpenCL Vcpkg port.
    • TCLAP has not been bundled. Should it be decided that the SDK should bundle that as well, we can add it next to the other deps.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Dec 3, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Contributor

@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.

Thanks for pulling this all together! This is a huge upgrade to the SDK.

I haven't done an in-depth review but I did do a quick skim through the documentation and identified a few issues.

I also managed to build the updated SDK including all samples following the attached build instructions on my Ubuntu install. Getting vcpkg setup wasn't entirely trivial, but once it was done things pretty much "just worked". I flagged the few build warnings I saw.

The biggest issue I have after building and installing is that a lot of samples don't run due to missing libraries (I've installed into my own install directory, underneath the SDK build tree). Should we build the OpenCL utils as a static library or copy it into the same directory as the samples so this is more robust?

bashbaug@bashbaug-nuc:~/git/OpenCL-SDK/install/Release$ ./saxpy 
./saxpy: error while loading shared libraries: libOpenCLUtils.so: cannot open shared object file: No such file or directory

I can run the samples out of the build directory, where the executables still have the libs RPATH'd, but this has other problems. Some examples:

saxpy fails with CL_INVALID_VALUE, though I don't see anywhere we are returning this error:

bashbaug@bashbaug-nuc:~/git/OpenCL-SDK/build/bin$ ./saxpy 
Selected platform by Intel(R) Corporation
Selected device: Intel(R) HD Graphics 620 [0x5916]
OpenCL C 3.0 


Error: CL_INVALID_VALUE

cppsaxpy can't find the kernel source, which is understandable since I am running from the build directory, but it isn't in the install directory either:

bashbaug@bashbaug-nuc:~/git/OpenCL-SDK/build/bin$ ./cppsaxpy 
Selected platform: Intel(R) Corporation
Selected device: Intel(R) HD Graphics 620 [0x5916]

Error: Cannot open kernel source: ./saxpy.cl

binaries cannot find the right binary, but I'm not sure how to create it:

bashbaug@bashbaug-nuc:~/git/OpenCL-SDK/build/bin$ ./binaries 
Selected platform by Intel(R) Corporation
Selected device: Intel(R) HD Graphics 620 [0x5916]
OpenCL C 3.0 

No suitable file Collatz-Intel(R) HD Graphics 620 [0x5916].bin found

Error: CL_INVALID_VALUE

How can we improve the odds that this all "just works" and/or improve diagnostics when an error occurs?

@MathiasMagnus
Copy link
Copy Markdown
Collaborator Author

@bashbaug I think I know where your woes are coming from (and both are totally legit).

  1. Not finding the library happens only with multi-config generators. Building using the default Unix Makefiles or Ninja works. Working on a fix.
  2. The headers is a sticky subject. (Semi-extensive exploration of the phase space is here, which I intend on merging into either the OpenCL-Guide or the SDK.) The 'current working directory' (henceforth cwd)-relative solution is simple, but is fairly brittle. Copying the kernels into the build tree is problematic because
  • One may copy at configuration time using file(COPY ...) or configure_file() but then one needs to reconfigure to update the files on disk. (It's annoying to develop samples that way.)
  • One may set it as a post-build step, but then it will only trigger when one modifies the host code. (This is annoying too.)
  • One may promote it to a build step, in which case utility targets like saxpy-kernel and nbody-kernel-shader start mildly polluting the list of available targets. Samples run fine from the install tree, because install commands can properly track the timestamps of both the executables and the kernel files.

Symlinking the kernels would be awesome, as it follows updates to code in the source tree, but that has many problems too:

  • The problem with that 🥁🥁🥁🥁🥁🥁🥁🥁 file(CREATE_LINK ...) is CMake 3.14. (And cmake -E create_symlink as non-UNIX-only is 3.13.) We can raise minimum version when building the samples to 3.13 / 3.14 or accept the bloat
if(CMAKE_SYSTEM_NAME MATHCES Windows)
  execute_process(COMMAND "mklink ...")
else()
  execute_process(COMMAND "ln -s ...")
endif()

but even then, symlinking on Windows without admin rights requires developer mode enabled on the system. 🤯 Wow...

Ultimately I accepted the fact that running the samples from the build tree requires making the cwd the path to the kernel in the source tree, but if it bugs anyone, pick an item from the above list or the samples should move away from the CWD-relative location of device code.

@MathiasMagnus
Copy link
Copy Markdown
Collaborator Author

All samples now execute correctly from the build tree as well as the install tree in both single and multi-config generators.

Copy link
Copy Markdown
Contributor

@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.

Thanks, this is looking a lot better now!

I was able to run all of the samples save histogram on Linux, and I was able to run this one when I manually copied the kernel file to the install directory. I wasn't able to test the CL-GL sharing samples because we don't support the interop extensions on Linux, but I was able to verify that they run and do not have any missing libraries.

I'm having trouble building on Windows though. I think this is a problem on my end related to vcpkg (the OpenCL-SDK project still can't find TCLAP), but it hasn't been as smooth as Linux. I'll try more tomorrow, since we do support the interop extensions on Windows.

Copy link
Copy Markdown
Contributor

@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.

I've been able to successfully build and run all apps on both Windows and Linux - thanks @MathiasMagnus for all of your help getting this working!

For record keeping, the issues I was having on Windows were:

  1. vcpkg was defaulting to the 32-bit x86 packages instead of the 64-bit x64 packages. Fixed by explicitly specifying --triplet x64-windows for vpkg (see a64a4cb).
  2. After this, the samples built, but not all DLLs were being properly copied to the output directory. Fixed by updating my cmake version (see 9dd7c32).

Copy link
Copy Markdown
Contributor

@alycm alycm left a comment

Choose a reason for hiding this comment

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

I haven't reviewed this PR. I did build and test the samples on this branch, on an implementation with no GL interop running on an x86 CPU.

cd build
if [[ "${{ matrix.os }}" == "ubuntu-20.04" ]]; then
sudo apt update
sudo apt install -y libtclap-dev libglm-dev libglew-dev libsfml-dev libstb-dev libidn11 libx11-dev libxrandr-dev libxi-dev mesa-common-dev libgl1-mesa-dev libglu1-mesa-dev libudev-dev
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know Ubuntu isn't universal, but it might be helpful to put this in the readme. Or at least the ones that aren't there by default on a desktop install.

I had to install these to get the SDK to build:

sudo apt install libtclap-dev libstb-dev libglew-dev libsfml-dev libglm-dev

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

These packages are mentioned in the OpenCL-Guide in the Linux guide here. These are only required if one builds the samples and not otherwise.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm guessing this is only needed if the package dependencies aren't satisfied via vcpkg?

Regardless, I agree it would be worth including this as an alternative in the README. Installing via the OS package manager will likely be smoother than vcpkg (getting it working on Ubuntu wasn't trivial) and this is the mechanism we're testing in CI.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

One can install them either way. It's user preference.

-D OPENCL_ICD_LOADER_BUILD_TESTING=ON
-D BUILD_DOCS=OFF
-D BUILD_EXAMPLES=OFF
-D BUILD_TESTS=ON
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should some of these be set in the CMakeLists.txt?

This line in particular I had to set to get anything to build (I suspect that is actually a bug in OpenCL-CLHPP CMake, but it wasn't immediately clear why -- I'll look again later).

Other variables are not essential, but setting them saves building unnecessary tests.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I was thinking hard about making that also part of these PRs, but... scope creep. I really want to make the test option variables consistent. Currently only the ICD-Loader is able to selectively disable it's tests via OPENCL_ICD_LOADER_BUILD_TESTING and that's something the CLHPP repo should enable as well. One may want to test the SDK samples, but not build the few hundred header tests. I'll see to it shortly after the SDK and the base changes go through.

danieleftodi referenced this pull request in hackhy/ethminer Jan 10, 2022
Melirius and others added 27 commits January 25, 2022 14:39
* WIP

* Added error handling to context_properties

* Initial Utility and SDK lib docs

* Match definition to declaration

* Fix constraints lifetime

* Explicit narrowing conversion

* Fix lifetime

* Initial NBody sample

* Reduce NBody particle count

* NBody docs

* Add GLM as a dependency

* Don't draw after closing window

* Fix InteropWindow namespace

* Don't call CTOR explicitly

* Simplify includes

* Build SDK libs in correct place

* Hook SDK deps to the correct targerts

* Only install Util headers

* Check for TCLAP alongside other deps

* Only check for sample deps when building them

* Add TCLAP to SDK props

* Build SDK lib only with samples

* Inherit common deps from libSDK

* Modularize Stb detection for non-Vcpkg consumers

* Revamp dependency use

* Install layout overhaul

* Fix CI YAML

* Fix dynamic library dependence in install tree

* Update readme

* Don't test NBody using CTest

* Move image Dockerfiles to OpenCL-SDK

* Remove dead kernel code

* README typo fixes.

* Newline

* Update submodules to point GitHub

* Apply git-format

* CI bump

* Install deps.

* apt update

* Install stb

* Acknolwedge Linux stb install layout

* Fix build path

* Fix vcpkg paths

Co-authored-by: Ivan Siutsou <ivan@streamhpc.com>
Remove bogus full path from CMake invocation

Co-authored-by: Ben Ashbaugh <ben.ashbaugh@intel.com>
Multiline command syntax

Co-authored-by: Ben Ashbaugh <ben.ashbaugh@intel.com>
Co-authored-by: Alastair Murray <alastair.murray@codeplay.com>
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.

6 participants