[CMake] Enforce oneMath dependecy during configuration

[EwanC]

Currently when trying to build the whole project using CMake without a oneMath install on the system, CMake will configure okay but then fail to find dependencies at build time.

A better used experience would be to try detect oneMath at CMake configure time, and then skip the oneMath_gemm target with a warning if oneMath isn't found.

If we really do want oneMath to be a dependency then this should be find_package(oneMath REQUIRED), but that feels like it increases the barrier to entry for newcomers.

[steffenlarsen]

I agree that it makes sense for oneMath to be optional, given it should only affect a subset of exercises.

The other changes seem to adhere to https://github.com/uxlfoundation/oneMath/blob/develop/docs/using_onemath_with_cmake.rst.

[tomdeakin]

Have you tested where the SYCL implementation is AdaptiveCpp and an oneMKL library is found by CMake on the system? As I understand it the library might not work out the box with AdaptiveCpp. Is there a graceful exit?

[EwanC]

Have you tested where the SYCL implementation is AdaptiveCpp and an oneMKL library is found by CMake on the system? As I understand it the library might not work out the box with AdaptiveCpp. Is there a graceful exit?

So the purpose of the PR is more for enabling a development environment where oneMath isn't available rather than proving the correctness of the existing example. However, the example works because oneMath != oneMKL. oneMKL is an Intel product and one of the possible backends to oneMath, however it's not the only backend.

I built oneMath using AdaptiveCpp with the rocBLAS backend, and pointing CMAKE_PREFIX_PATH at that in a SYCL-Academy build for AdaptiveCpp works. For example, If I choose the buffer variant to show the AdaptiveCpp buffer warning in the output:

$ ./Code_Exercises/oneMath_gemm/oneMath_gemm_solution_onemath_buffer_gemm 
Problem size: c(600,2400) = a(600,1200) * b(1200,2400)
Running on AMD Instinct MI210, version: gfx90a:sramecc+:xnack-, driver version: 60443483
[AdaptiveCpp Warning] This application uses SYCL buffers; the SYCL buffer-accessor model is well-known to introduce unnecessary overheads. Please consider migrating to the SYCL2020 USM model, in particular device USM (sycl::malloc_device) combined with in-order queues for more performance. See the AdaptiveCpp performance guide for more information: 
https://github.com/AdaptiveCpp/AdaptiveCpp/blob/develop/doc/performance.md
Verify results between oneMath & serial: SUCCESS - The results are correct!

[illuhad]

I built oneMath using AdaptiveCpp with the rocBLAS backend, and pointing CMAKE_PREFIX_PATH at that in a SYCL-Academy build for AdaptiveCpp works. For example, If I choose the buffer variant to show the AdaptiveCpp buffer warning in the output:

Just to comment that it might be different for other backends. oneMath only partially supports AdaptiveCpp for some backends.

[tomdeakin]

WG discussion: can we have this example turned off by default, with an option to turn it on?

[EwanC]

Thanks for the feedback, I've pushed a commit that adds a CMake option SYCL_ACADEMY_ENABLE_ONEAPI_LESSONS to control whether this lesson will try to be built. I went for "ONEAPI" rather than "ONEMATH" in the name in case we want to add lessons for other oneAPI libraries in future and can reuse the option.

It defaults to ON for DPC++ builds, and OFF otherwise, but happy to change that default to always OFF if that's the preference.

[illuhad]

Thanks, LGTM!

It defaults to ON for DPC++ builds, and OFF otherwise, but happy to change that default to always OFF if that's the preference.

I don't have a position on this. To me it seems fine either way.

I'd suggest to merge this before the CI PR given that it already has multiple reviews and seems good to go!

[illuhad]

@tomdeakin Did we decide on a process for merging? The PR has addressed the feedback from the WG and IMO is good to merge now. Can we merge it now?

[tomdeakin]

@tomdeakin Did we decide on a process for merging? The PR has addressed the feedback from the WG and IMO is good to merge now. Can we merge it now?
Will add to the agenda.