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

[SYCL] kernel_compiler support of OpenCL queries #12888

Merged
merged 25 commits into from
Mar 7, 2024

Conversation

cperkinsintel
Copy link
Contributor

This PR both includes and realizes the new specification here originally drafted here: #11994

gmlueck and others added 14 commits November 23, 2023 08:27
Add additional APIs to the OpenCL kernel compiler specification, which
allow the application to query the OpenCL features and extensions that
are supported.
…sonal aside, can I once again take a moment to say how much I hate clang-format? OMG it uglifies the code terribly. People like it because it destroys/uglifies code EQUALLY, like some sort of Harrison Bergeron nightmare - clown nosed and spectacled, killed by a shotgun. What a waste.
…ioning the exceptions I've unilaterally decided to throw
@densamoilov
Copy link

densamoilov commented Mar 1, 2024

The new API to query OpenCL extensions/features requires a minimum version of ocloc and hence a minimum version of the GPU driver. The documentation says that the latest GPU driver should be used for development, which I find confusing as the latest GPU driver would also be required for running an application that uses the new API.

I also, couldn't find any requirements for the GPU driver for the open-source DPC++ compiler.

The fact that the API may or may not work depending on a version of the GPU driver is confusing so we need to make an effort to let the users know what minimum version of the GPU driver they have to use.

@bader
Copy link
Contributor

bader commented Mar 1, 2024

couldn't find any requirements for the GPU driver for the open-source DPC++ compiler.

FYI: https://github.com/intel/llvm/blob/sycl/devops/dependencies.json

@cperkinsintel cperkinsintel marked this pull request as ready for review March 2, 2024 01:45
@cperkinsintel cperkinsintel requested review from a team as code owners March 2, 2024 01:45
//===----------------------------------------------------------------------===//

// Run on CUDA, HIP, and other devices that don't support the kernel_compiler
// REQUIRES: !ocloc
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test looks good, but I don't understand how this REQUIIRES selects a CUDA or HIP device. Does it?

What does REQUIRES: ocloc really mean? Does it just mean that ocloc is present on the PATH? If that is the case, you could easily imagine a case where ocloc is on the PATH, but we are still running on a CUDA device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this to // REQUIRES: !ocloc && !opencl && !level_zero and the others to ocloc && (opencl || level_zero)

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't understand the exact meaning of REQUIRES: ocloc. Does this mean that ocloc is on the PATH?

If so, wouldn't we want the test to fail if ocloc is not on the PATH when running on Level Zero or OpenCL? Isn't it a bug if ocloc is not available in that case?

Copy link
Contributor Author

@cperkinsintel cperkinsintel Mar 6, 2024

Choose a reason for hiding this comment

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

Yes, // REQUIRES: ocloc means ocloc is on the PATH. And // REQUIRES: !ocloc means that it is affirmed to NOT be on the PATH.

As it now with my latest change, if the situation of "L0 but no ocloc" or "CUDA but with ocloc" comes up then the test will be skipped as UNSUPPORTED. If I were to go back to just requiring ocloc (or not) then the test would fail in those situations - which might arguably be better because it would alert us to a change or misconfiguration.

As to what it means if we have L0 but not ocloc, that I can't say. You have said that ocloc is "an implementation detail". And that's not unreasonable. The drivers could potentially run perfectly fine without ocloc being exposed on the path. In those cases the kernel_compiler will simply not work for opencl language ( it throws exceptions / returns false depending on the call in that situation), and if that configuration appears then these tests will be skipped over as UNSUPPORTED.

But if it means that it's a bug, then, yes, I should return these //REQUIRES: directives to just focus on ocloc or not and we'll get failures in the future if that happens. Arguably, if it's a bug (not merely an implementation detail) then perhaps these opencl queries should throw exceptions if ocloc isn't present, rather than returning false.

Let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we do the following:

  • Change the positive test to REQUIRES: (opencl || level_zero)
  • Change the negative test to REQUIRES: !opencl && !level_zero

The positive test expects d.ext_oneapi_can_compile(syclex::source_language::opencl) to be true. If ocloc is missing for some reason, that query will return false and the test will fail. That makes sense because ocloc is required part of the implementation on these backends, so we want the test to fail if it is somehow missing.

Copy link
Contributor Author

@cperkinsintel cperkinsintel Mar 6, 2024

Choose a reason for hiding this comment

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

One last wrinkle I overlooked: opencl:fpga . I'll add an unsupported line for that (simplest)

sycl/test-e2e/KernelCompiler/opencl_queries.cpp Outdated Show resolved Hide resolved
@steffenlarsen steffenlarsen merged commit 6344ead into intel:sycl Mar 7, 2024
13 checks passed
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