fix merge after readding syclkernel attr #20363

sarnex · 2025-10-14T21:42:46Z

fix merge after llvm/llvm-project#162868

check-clang passes with only 3 unrelated failures that were failing before the breaking change

Signed-off-by: Sarnie, Nick <[email protected]>

sarnex · 2025-10-14T21:51:23Z

FYI @tahonermann

tahonermann

I started looking at this, but it is going to take more time than I have available tonight. I'll resume tomorrow.

tahonermann · 2025-10-14T22:28:55Z

clang/include/clang/AST/GlobalDecl.h

  static KernelReferenceKind getDefaultKernelReference(const FunctionDecl *D) {
-    return (D->hasAttr<DeviceKernelAttr>() || D->getLangOpts().CUDAIsDevice)
+    return (D->hasAttr<DeviceKernelAttr>() || D->getLangOpts().CUDAIsDevice ||
+            D->hasAttr<SYCLKernelAttr>())


I'm not sure that checking for SYCLKernelAttr is correct here. We didn't check for it prior to llvm/llvm-project#137882 landing.

Right, upstream didnt, but we did in syclos before we pulled down that commit, see here https://github.com/intel/llvm/blob/nightly-2025-07-18/clang/include/clang/AST/GlobalDecl.h#L168

Thanks, yes, added by commit 3b170e3 in May, the pulldown happened in commit fcd95a9, and that change was still present in the parent commit 6b2006a (from the previous pulldown). Isn't git archeology a joy.

clang/lib/AST/Decl.cpp

jsji · 2025-10-15T16:40:15Z

Please prioritize the review, we will need to merge this later today to unblock pulldown. Thanks.

tahonermann

I've done what I can to review. I find git archeology in the presence of pull down merges really difficult, so I wouldn't be surprised if I've missed things. There is one bit of code that I think is incorrect and at least one other one that I'm unsure of. Otherwise, it looks like @sarnex has done his homework on this quite well.

tahonermann · 2025-10-15T17:48:54Z

clang/include/clang/AST/GlobalDecl.h

  static KernelReferenceKind getDefaultKernelReference(const FunctionDecl *D) {
-    return (D->hasAttr<DeviceKernelAttr>() || D->getLangOpts().CUDAIsDevice)
+    return (D->hasAttr<DeviceKernelAttr>() || D->getLangOpts().CUDAIsDevice ||
+            D->hasAttr<SYCLKernelAttr>())


Thanks, yes, added by commit 3b170e3 in May, the pulldown happened in commit fcd95a9, and that change was still present in the parent commit 6b2006a (from the previous pulldown). Isn't git archeology a joy.

clang/lib/AST/Decl.cpp

tahonermann · 2025-10-15T18:45:34Z

clang/lib/Sema/SemaDeclAttr.cpp

  if (S.getASTContext().getTargetInfo().getTriple().isNVPTX()) {
->>>>>>> 1db148cc946eb95fefd5399766e379fc030eef78
    handleGlobalAttr(S, D, AL);


I'm not sure this is right. Prior to llvm/llvm-project#137882, processing of NVPTXKernelAttr and CUDAGlobalAttr were not dependent on an NVPTX target. I'm not sure if this has ramifications for the SYCL-CUDA compat mode.

For NVPTXKernelAttr, it looks like it is target dependent in the attr definition:

https://github.com/llvm/llvm-project/blob/87f0227cb60147a26a1eeb4fb06e3b505e9c7261/clang/include/clang/Basic/Attr.td#L1470

def NVPTXKernel : InheritableAttr, **TargetSpecificAttr<TargetNVPTX>** {

so if the target doesn't match the the attr is never added, the check for the target agains the attr is in ParsedAttr, before we create that NVPTXKernel object

and this function can't be hit for CUDAGlobalAttr because it's guarded by a check for DeviceKernelAttr.

so I think the cases handleGlobalAttr sees NVPTXKernelAttr or CUDAGlobalAttr are the exact same before and after

Ok, I think I agree with that, but shouldn't there also be a check for isNVPTXSpelling()? If the spelling is "__kernel" or "kernel", I would expect the else branch to be the desired control flow.

upstream Joseph was really against target-specific behavior, and even if seems confusing he would prefer for all the remaining spellings of DeviceKernelAttr to be aliases, expect for OpenCL because it's very particular, and if we do make a change like that i would expect it to be upstream

I understand that he would like to have one attribute for kernels that are strongly tied to a particular target (though I don't necessarily agree with him). With this code as is though, wouldn't use of the OpenCL spellings end up going down the wrong branch when targeting NVPTX?

Yeah I think you're right, but it doesn't seem to be causing any problems either upstream or here.
Is it okay if we move forward with this PR in syclos and I make a PR upstream to add a check so we don't go down this branch for OpenCL spellings?

Sure, I think that is reasonable.

llvm/llvm-project#163859

Co-authored-by: Tom Honermann <[email protected]>

tahonermann

I'm going to go ahead and improve with the understanding that one outstanding issue will be addressed by a separate PR that targets upstream. All of my other concerns have been addressed.

sarnex · 2025-10-15T20:41:19Z

Thanks, will work on the upstream PR immedately.

@jsji @premanandrao Please merge this when you are ready

premanandrao · 2025-10-15T20:49:11Z

Thank you @tahonermann and @sarnex

jsji · 2025-10-15T21:08:49Z

clang/lib/AST/Decl.cpp

 bool FunctionDecl::isReferenceableKernel() const {
  return hasAttr<CUDAGlobalAttr>() ||
-         DeviceKernelAttr::isOpenCLSpelling(getAttr<DeviceKernelAttr>()) || hasAttr<DeviceKernelAttr>();
+  return hasAttr<CUDAGlobalAttr>() ||


Duplicate lines:
return hasAttr() ||

It is OK, I will remove it in the amend commit.

ah my bad, github suggestion UI 👎

fix merge after readding syclkernel attr

9940ddb

Signed-off-by: Sarnie, Nick <[email protected]>

sarnex marked this pull request as ready for review October 14, 2025 21:50

sarnex requested a review from a team as a code owner October 14, 2025 21:50

sarnex requested review from Fznamznon and jsji October 14, 2025 21:51

tahonermann reviewed Oct 14, 2025

View reviewed changes

jsji requested a review from tahonermann October 15, 2025 16:36

tahonermann reviewed Oct 15, 2025

View reviewed changes

remove extra DeviceKernelAttr check

9a28bce

Co-authored-by: Tom Honermann <[email protected]>

tahonermann approved these changes Oct 15, 2025

View reviewed changes

premanandrao merged commit 3ff4b20 into sycl-web-to-resolve Oct 15, 2025

jsji reviewed Oct 15, 2025

View reviewed changes

bader deleted the sarnex/syclweb_fix branch October 16, 2025 21:51

fix merge after readding syclkernel attr #20363

fix merge after readding syclkernel attr #20363

Uh oh!

Conversation

sarnex commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarnex commented Oct 14, 2025

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarnex Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsji commented Oct 15, 2025

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarnex Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarnex Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarnex Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

sarnex commented Oct 15, 2025

Uh oh!

premanandrao commented Oct 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sarnex commented Oct 14, 2025 •

edited

Loading

sarnex Oct 15, 2025 •

edited

Loading

sarnex Oct 15, 2025 •

edited

Loading

sarnex Oct 15, 2025 •

edited

Loading

sarnex Oct 15, 2025 •

edited

Loading