Support for find_package in CMake

[codeplaysoftware]

Support for find_package in CMake

[pretyman]

This module would define COMPUTECPP_INCLUDE_DIR, COMPUTECPP_LIBRARY

Please don't. CMake is a mess by nature, but let's not advocate outdated practices. We should use targets instead and transient dependencies through the target's interface.

find_package should create a target, (ideally in a namespace) ComputeCpp::ComputeCpp, with the include dirs and libraries in its interface.

I wrote ComputeCppConfig.cmake and other CMake files which are auto generated by CMake, to mimic as if ComputeCpp package was CMake installed. I used https://github.com/forexample/package-example as a base and added the cmake files to the lib/cmake/ComputeCpp

find_package(ComputeCpp)
add_executable("my_executable" "src/main_with_kernel.cpp")
target_link_libraries("my_executable" PUBLIC ComputeCpp::ComputeCpp)
computecpp_compile("my_executable" "src/main_with_kernel.cpp")

In practice I ended up not using computecpp-sdk, I used it only as a base to understand what was going on in FindComputeCpp.cmake. This cmake file is a bit odd and seems wrong in parts, for example, it adds -include file_with_kernel.cpp.sycl file to the CMake target, but I guess you want to use -include file_with_kernel.cpp.sycl when compiling file_with_kernel.cpp, isnt that correct? Not to mention that if more than one source is passed to add_sycl_to_target, only the last one will have its -include flag added to the target.

However there are things I have not yet fixed in add_sycl_to_target, like passing the include directory to computecpp, how it is today won't work 100% because the include directory property of CMake can contain the dreaded generator expressions, so the code today will eventually break when used with other projects.

I will happily contribute the CMake scripts I am using, however right now I feel that they should be packged together with ComputeCpp, instead of this separate project "ComputeCpp-SDK"

[DuncanMcBain]

Hi @pretyman, you're right that things aren't perfect right now. If I can address your last point first: we made this repo precisely so that it was easier for people to contribute. We made a conscious decision to put the CMake code here so that we could update it independently of the main ComputeCpp releases, which are generally regular but sometimes are delayed or whatever. Personally I'd like to have each new release of ComputeCpp take the current version of FindComputeCpp.cmake and bundle that, but that requires some organisation to make sure we don't break anything, or release subtly incompatible versions of the runtime and the CMake, for example.

So, I'm therefore always interested in using more modern CMake practices (though we're not yet ready to bump the CMake minimum version requirement yet). I agree there are lots of improvements that could be made, though I was also fairly satisfied that what we have works well enough at the moment. Have you run into any cases where it doesn't work properly for you?

Finally, to describe exactly what's going on with the include files, somewhere around line 300 of FindComputeCpp.cmake: the basic functionality is to add a new command line argument to the host compiler, that being -include integration_header.cpp.sycl. This file is just "normal" C++ code, containing descriptions of the kernel, the actual kernel code and so on. Most of the time it's just added exactly like that. However, in the case of the property COMPUTECPP_INCLUDE_AFTER being specified, I had it instead compile as if the integration header were the source file, and the user code should be force included first. This is needed when the kernel uses an enumeration in its name - the integration header would have to declare but not define the enum. That is a GCC extension, not standard C++, so reordering the files in this way means that the code using enums in kernel names can still work.

By my reading of the code, the __build_spir function is called for every file in the target, so all of them should have their corresponding integration header added. It was added by another external contributor, so while it's not used in the SDK directly, I always thought it would be working for their use case.

Thanks for this feedback, it's really valuable 😃

[MathiasMagnus]

If I may add a thought… I think the most valuable contribution would be a FindSYCL.cmake that could light up both triSYCL and ComputeCpp builds, especially how triSYCL is much more debug friendly. Debug builds could execute on the host and release builds on the GPU.

Having said that, best solution were if CMake were SYCL aware, and while CUDA has large enough a user-base for that support to materialize, SYCL is not there yet, and one of the reasons is how flaky CMake is. (Just take a look at all the Windows-specific stuff that had to be added, some even specific to the MSBuild generator.) Some of this flakyness makes its way to end-users.

Current MSBuild and CMake scripts are okay and super useful, but there's still room for improvement. One such thing would be a unified FindSYCL.cmake that works both with triSYCL and ComputeCpp.

[DuncanMcBain]

The original scope of this issue was dealt with by #118. There are no plans for a combined "FindSYCL.cmake" at the moment.

(I believe some of the issues reported by pretyman have also been fixed in this PR - including the issue with -include and multiple files).