-
Notifications
You must be signed in to change notification settings - Fork 798
Resolve sycl-web test failures caused by https://github.com/llvm/llvm-project/pull/140282 #19895
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
clang/test/CodeGenSPIRV/Builtins/generic_cast_to_ptr_explicit.c
Outdated
Show resolved
Hide resolved
@@ -3,7 +3,7 @@ | |||
// RUN: %clang_cc1 -O1 -triple spirv32 -cl-std=CL3.0 -x cl %s -emit-llvm -o - | FileCheck %s | |||
|
|||
#ifdef __SYCL_DEVICE_ONLY__ |
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.
Something seems off here. Why would __SYCL_DEVICE_ONLY__
be defined for the runs that don't include -fsycl-is-device
? -x cl
doesn't somehow imply SYCL, does it?
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.
The very first line runs clang with -fsycl-is-device.
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.
Yes, and this makes sense for that RUN line. My question is about the other two RUN lines that pass -x cl
and don't pass -fsycl-is-device
. The code presumes that __SYCL_DEVICE_ONLY__
is defined for those RUN lines as well and I don't understand why that would be the case. I also find it odd that an attribute named sycl_device
is expected to be used in a non-SYCL OpenCL compilation. I would expect an attribute named opencl_device
or similar instead.
clang/test/CodeGenSPIRV/Builtins/generic_cast_to_ptr_explicit.c
Outdated
Show resolved
Hide resolved
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 looks good, but the latest changes should make it unnecessary to add __attribute__((sycl_device))
. If that isn't the case, then I'm confused (again).
clang/lib/CodeGen/CodeGenModule.cpp
Outdated
!D->hasAttr<SYCLDeviceAttr>() && | ||
!(D->hasAttr<SYCLDeviceAttr>() || D->hasAttr<SYCLExternalAttr>()) && | ||
!SemaSYCL::isTypeDecoratedWithDeclAttribute<SYCLDeviceGlobalAttr>( | ||
D->getType())) | ||
return getLangOpts().GPURelocatableDeviceCode |
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.
We'll need to do something similar to this in upstream as part of fixing emit of inline functions in device code.
clang/test/CodeGenSPIRV/Builtins/generic_cast_to_ptr_explicit.c
Outdated
Show resolved
Hide resolved
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.
This looks perfect, thank you @schittir!
Thank you for reviewing this, @tahonermann @Fznamznon @premanandrao ! |
This patch fixes test failures caused by the upstream patch that adds sycl_external attribute and restricts emitting device code.