-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[clang] Ensure we don't process OpenCL kernels as CUDA kernels #163859
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sarnie, Nick <[email protected]>
@llvm/pr-subscribers-clang Author: Nick Sarnie (sarnex) ChangesWe've decided the OpenCL spelling of the I wasn't able to come up with a test with any actual difference in behavior. It's possible this is NFC but I'm not sure so I'm not adding the tag. Full diff: https://github.com/llvm/llvm-project/pull/163859.diff 1 Files Affected:
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index e6f8748db7644..6978299734ece 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -5206,7 +5206,8 @@ static void handleCallConvAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
static void handleDeviceKernelAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
const auto *FD = dyn_cast_or_null<FunctionDecl>(D);
bool IsFunctionTemplate = FD && FD->getDescribedFunctionTemplate();
- if (S.getASTContext().getTargetInfo().getTriple().isNVPTX()) {
+ if (S.getASTContext().getTargetInfo().getTriple().isNVPTX() &&
+ !DeviceKernelAttr::isOpenCLSpelling(AL)) {
handleGlobalAttr(S, D, AL);
} else {
// OpenCL C++ will throw a more specific error.
|
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 the conditions for each branch can be expressed more cleanly per my inline comment.
clang/lib/Sema/SemaDeclAttr.cpp
Outdated
if (S.getASTContext().getTargetInfo().getTriple().isNVPTX()) { | ||
if (S.getASTContext().getTargetInfo().getTriple().isNVPTX() && | ||
!DeviceKernelAttr::isOpenCLSpelling(AL)) { | ||
handleGlobalAttr(S, D, AL); | ||
} else { |
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 can be made easier to read and maintain by reversing the order of the if/else branches.
if (DeviceKernelAttr::isOpenCLSpelling(AL)) {
// Do OpenCL things...
} else if (S.getASTContext().getTargetInfo().getTriple().isNVPTX()) {
// Do presumed CUDA/HIP kernel things, but only on NVPTX for some reason...
}
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.
Thanks for the review. We go into the else
branch for OpenCL spelling or non-NVPTX, so I don't think we can only check for the OpenCL spelling if we swap that condition to be the if
. I made an attempt in my latest commit, please take a look when you have a chance.
Signed-off-by: Sarnie, Nick <[email protected]>
We've decided the OpenCL spelling of the
DeviceKernel
attr needs separate handling, and add a case to that special handing for NVPTX target.I wasn't able to come up with a test with any actual difference in behavior.
It's possible this is NFC but I'm not sure so I'm not adding the tag.