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

[UR][CUDA] Fix compatibility with CUDA 11.x #1037

Merged
merged 3 commits into from
Nov 24, 2023

Conversation

al42and
Copy link
Contributor

@al42and al42and commented Nov 3, 2023

The code introduced in 74f42f8 uses the signature of cuGraphInstantiate from CUDA 12.x. In CUDA 11.4-11.8, the same function is called cuGraphInstantiateWithFlags, so we use it. In older CUDA, there is only the old version of cuGraphInstantiate which does not accept flags.

The code introduced in 74f42f8 uses the
signature of cuGraphInstantiate from CUDA 12.x. In CUDA 11.x, this
function has different parameters.
@al42and
Copy link
Contributor Author

al42and commented Nov 3, 2023

PR for pre-merge testing: intel/llvm#11773

Copy link
Contributor

@Bensuo Bensuo 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 catching this! Changes look good to me just one minor comment.

source/adapters/cuda/command_buffer.cpp Show resolved Hide resolved
@EwanC EwanC added the ready to merge Added to PR's which are ready to merge label Nov 9, 2023
@abagusetty
Copy link

Can we get this merged.

Fixes: #1119

@kbenzie
Copy link
Contributor

kbenzie commented Nov 24, 2023

Can we get this merged.

Fixes: #1119

About to do so.

PR for pre-merge testing: intel/llvm#11773

I'm going to close this testing PR. I'll replace it with a new intel/llvm PR that combines a number of UR PR's together with the goal of getting patches merged more quickly.

@kbenzie kbenzie merged commit e197941 into oneapi-src:adapters Nov 24, 2023
48 checks passed
@al42and al42and deleted the fix-cuda-11-8-compat branch November 24, 2023 15:31
@al42and
Copy link
Contributor Author

al42and commented Nov 24, 2023

I'm going to close this testing PR. I'll replace it with a new intel/llvm PR that combines a number of UR PR's together with the goal of getting patches merged more quickly.

Sure thing!

By the way, is the whole "draft MRs in a different repo just to test" shtick a temporary solution, or is it intended to be the long-term way of running the project? I see the benefit when the changes to IntelLLVM and UR are tightly coupled, but, with two separate repos, I got the impression that the general direction was towards untangling them and making the development more independent?

@kbenzie
Copy link
Contributor

kbenzie commented Nov 24, 2023

By the way, is the whole "draft MRs in a different repo just to test" shtick a temporary solution, or is it intended to be the long-term way of running the project? I see the benefit when the changes to IntelLLVM and UR are tightly coupled, but, with two separate repos, I got the impression that the general direction was towards untangling them and making the development more independent?

I agree this is an unusual setup and it's primarily due to historical reasons. UR was born out of the Plugin Interface (PI) in intel/llvm which we are still in the process of fully migrating in the SYCL runtime.

We are still working towards decoupling UR from intel/llvm further but it's a time consuming process and must be balanced with our other commitments. The main area which needs improved is our level of testing, both API coverage and the OS/hardware support matrix.

Having now completed the move of all the adapter implementations to this repo the maintenance workload has increased quite a bit for me personally as a result of the current process. So I am personally invested in streamlining things for everyone involved in the project.

@al42and
Copy link
Contributor Author

al42and commented Nov 24, 2023

Thanks for the explanation, @kbenzie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants