-
Notifications
You must be signed in to change notification settings - Fork 802
[SYCL][CUDA] Fetch the adapter source from the UR repo #11342
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
Conversation
666b39a to
f4b0ac3
Compare
f4b0ac3 to
e6b009d
Compare
sycl/plugins/cuda/CMakeLists.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does cmake need 10.1 available when dpc++ is not usually built with 10.1? Can this be removed?:
find_package(CUDA 10.1 REQUIRED)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means that a version of 10.1 or higher is required to build. The Cmake in UR repo is asking for the same version.
Maybe there is an argument that 10.1 is no longer enough? In that case we should decide which version is required and update this cmake and the one in UR (but that is probably out of scope for this PR).
I don't think this can be removed because include(FindCUDACupti) relies on variables set by find_package(CUDA 10.1 REQUIRED) such as CUDA_TOOLKIT_ROOT_DIR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, yeah it probably makes sense to leave it as it is then. People might still want to use 10.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the cuda check moved inside if (SYCL_ENABLE_XPTI_TRACING) intentionally? Shouldn't it check there is a cuda version available even when not using XPTI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the code reached this point, find_package(CUDA) has already been invoked from unified-runtime and the library cudadrv already exists. The only reason to invoke it here is to get the env variables. I moved it inside the if intentionally because it's the only place where it is needed.
This implicit dependency from unified-runtime CmakeLists is not very intuitive. But PI will be removed soon, so I think there is no need to refactor this too much.
sycl/plugins/cuda/CMakeLists.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure of the significance of this part, but should/have you checked whether the removal of this code affects windows builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we officially support windows builds in CUDA? CI doesn't seem to run it.
This code was moved to unified-runtime repo: https://github.com/oneapi-src/unified-runtime/blob/adapters/source/adapters/cuda/CMakeLists.txt
It was causing issues because cudadrv was being imported twice. I think the behaviour should stay the same after the move.
For what it is worth, the windows job passed with this changes: https://github.com/intel/llvm/actions/runs/6340282334
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows support is experimental for cuda backend but I don't think it is tested by any CI. You should be able to build and run most tests on it though. Would be good to just check if still builds if the CMake changes could affect it. Codeplay has a windows machine available for this. If you are sure it won't affect windows build then of course this won't be worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think since the code is moved to UR, and linux CI cuda testing works, it is expected to still work on windows too.
e6b009d to
7e7cc2b
Compare
|
@intel/bindless-images-reviewers / @intel/dpcpp-l0-pi-reviewers , when possible, could someone have a look at this PR? |
The CUDA adapter source files have been moved to the unified runtime repository at https://github.com/oneapi-src/unified-runtime This commit removes the sources files from intel/llvm and updates cmake to fetch them directly from the unified runtime repository.
7e7cc2b to
345a913
Compare
|
@intel/dpcpp-l0-pi-reviewers please review ASAP, this is blocking all CUDA adapter changes. |
The CUDA adapter source files have been moved to the unified runtime repository at https://github.com/oneapi-src/unified-runtime
This commit removes the sources files from intel/llvm and updates cmake to fetch them directly from the unified runtime repository.