-
Notifications
You must be signed in to change notification settings - Fork 760
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][Driver][HIP] Do not define __CUDA_ARCH__ for HIP-AMD targets #15443
Conversation
Requires #15521 to get merged to rebase on those changes for the |
95a29e6
to
7062717
Compare
84bff0a
to
d31852f
Compare
d31852f
to
45b86ef
Compare
6c5e501
to
bf72790
Compare
@@ -309,7 +309,6 @@ void AMDGPUTargetInfo::getTargetDefines(const LangOptions &Opts, | |||
Twine("\"") + Twine(CanonName) + Twine("\"")); | |||
Builder.defineMacro("__amdgcn_target_id__", | |||
Twine("\"") + Twine(*getTargetID()) + Twine("\"")); | |||
Builder.defineMacro("__CUDA_ARCH__", "0"); |
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.
NOTE:
This was previously added to avoid setting it to a non-zero value but is not needed anymore after the changes in this PR completely disable the definition of the macro for SYCL offload. Additionally, the upstream llvm-project doesn't have this line https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/AMDGPU.cpp#L311.
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.
FE LGTM
The Update: Merged latest sycl branch that includes the above fix so all is passing now. |
@intel/llvm-gatekeepers This should be good merge now. Thanks! |
@GeorgeWeb The AMD runner is not doing too hot so we won't have E2E HIP testing for a bit (I'm working on it ASAP), are you confident this is sufficiently tested in compile-time tests? Thanks |
Good reminder. I forgot that was the reason I didn't ping for merging on Friday. I've run local testing last week but I'd rather it pass the precommit here. It's not a patch of high importance at the moment. |
@GeorgeWeb We don't have plans do to HIP testing in precommit for a while because the runner is in such bad shape, but we will have it in post commit when I can fix the runner. If you tested locally and are confident it should work I'm happy to merge the patch and wait and see, just wanted to check your confidence level. |
I fixed the HIP AMD runner so I'll merge this now |
This commit updates NVIDIA CCCL to version 2.7.0 and starts following the official repository rather than my fork. In order to make this work, I had to incorporate a temporary workaround for intel/llvm#15544 until we start using a release with intel/llvm#15443. Closes acts-project#660.
Fixes #15544