[clang][SYCL] Add sycl_external attribute and restrict emitting device code #140282

schittir · 2025-05-16T17:04:30Z

This patch is part of the upstreaming effort for supporting SYCL language front end.
It makes the following changes:

Adds sycl_external attribute for functions with external linkage.
Adds checks to avoid emitting device code when sycl_external and sycl_kernel_entry_point attributes are not enabled
Fixes test failures caused by the above changes

This patch is missing diagnostics for the following diagnostics listed in the SYCL 2020 specification's section 5.10.1, which will be addressed in a subsequent PR:
Functions that are declared using SYCL_EXTERNAL have the following additional restrictions beyond those imposed on other device functions:

If the SYCL backend does not support the generic address space then the function cannot use raw pointers as parameter or return types. Explicit pointer classes must be used instead;
The function cannot call group::parallel_for_work_item;
The function cannot be called from a parallel_for_work_group scope.

tahonermann

Thanks for working on this @schittir! I completed an initial pass of all of the code, but still need to look more closely at the documentation updates.

clang/lib/AST/ASTContext.cpp

clang/include/clang/Sema/SemaSYCL.h

clang/test/CodeGenSYCL/kernel-caller-entry-point.cpp

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/include/clang/Basic/Attr.td

clang/test/SemaSYCL/sycl-external-attribute.cpp

clang/lib/AST/ASTContext.cpp

schittir · 2025-05-19T15:27:54Z

Thanks for working on this @schittir! I completed an initial pass of all of the code, but still need to look more closely at the documentation updates.

Thank you for the initial pass review, Tom!

github-actions · 2025-05-26T22:45:36Z

✅ With the latest revision this PR passed the C/C++ code formatter.

clang/lib/AST/ASTContext.cpp

Support for functions is sufficient for SYCL 2020 spec conformance.

clang/include/clang/Basic/AttrDocs.td

clang/lib/Sema/SemaDeclAttr.cpp

clang/include/clang/Basic/AttrDocs.td

clang/test/CodeGenSYCL/amd-address-space-conversions.cpp

clang/test/CodeGenSYCL/function-attrs.cpp

clang/test/CodeGenSYCL/unique_stable_name_windows_diff.cpp

clang/test/SemaSYCL/sycl-external-attr-grammar.cpp

clang/test/SemaSYCL/sycl-external-attr.cpp

Co-authored-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

tahonermann

Thanks @schittir, here are my initial review comments. I expect to review the newly added tests more closely once these comments are all addressed.

clang/include/clang/Basic/Attr.td

clang/include/clang/Basic/AttrDocs.td

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/lib/AST/ASTContext.cpp

clang/test/SemaSYCL/sycl-external-attr.cpp

clang/test/SemaSYCL/sycl-external-attr-ignore.cpp

clang/test/SemaSYCL/sycl-external-attr-grammar.cpp

clang/test/SemaSYCL/sycl-external-attr.cpp

clang/test/SemaSYCL/sycl-external-attr-grammar.cpp

Fznamznon · 2025-07-10T09:08:37Z

Oops new tests are failing.

tahonermann

This is looking good @schittir! I added some comments for minor issues. I still need to review the tests more closely.

clang/include/clang/Basic/Attr.td

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/lib/Sema/SemaDecl.cpp

clang/test/CodeGenSYCL/kernel-caller-entry-point.cpp

tahonermann

A few more comments. I still haven't finished my review of sycl-external-attr.cpp, but will try to later today or over the weekend.

clang/test/SemaSYCL/sycl-external-attr-grammar.cpp

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/test/SemaSYCL/sycl-external-attr-ignored.cpp

clang/test/SemaSYCL/sycl-external-attr.cpp

tahonermann

I finished an initial pass through all of the tests. I'll do another round once you've had a chance to catch up on the comments so far.

clang/test/SemaSYCL/sycl-external-attr.cpp

clang/include/clang/Basic/AttrDocs.td

tahonermann

This is looking great @schittir! I just realized that, since we made the change to allow the attribute to be present during host compilation that we should no longer restrict diagnostics to device compilation and that tests should exercise host compilation as well.

I suggested a few FIXME comments, but other than that, I think this is good to go!

clang/lib/Sema/SemaDecl.cpp

clang/test/SemaSYCL/sycl-external-attr-appertainment.cpp

clang/test/SemaSYCL/sycl-external-attr-grammar.cpp

clang/test/SemaSYCL/sycl-external-attr.cpp

schittir · 2025-07-18T15:47:21Z

This is looking great @schittir! I just realized that, since we made the change to allow the attribute to be present during host compilation that we should no longer restrict diagnostics to device compilation and that tests should exercise host compilation as well.

I suggested a few FIXME comments, but other than that, I think this is good to go!

Thank you for the review, Tom!

tahonermann

I love it @schittir, this looks great!

@erichkeane, @bader, please review and lend your approval. Note that some of the changes to the tests are to synchronize them with continued evolution that happened in DPC++ but was not previously upstreamed.

bader

LGTM in general. I have a few minor comments related to the code style and one related to CodeGen testing.

clang/test/SemaSYCL/sycl-external-attr.cpp

clang/test/CodeGenSYCL/unique_stable_name.cpp

clang/lib/Sema/SemaSYCL.cpp

clang/lib/AST/ASTContext.cpp

clang/lib/Sema/SemaSYCL.cpp

erichkeane

Ah shoot, apparently I forgot to click 'submit' when I reviewed this last week. Sorry about that. Here are my pending comments.

clang/include/clang/Basic/AttrDocs.td

erichkeane · 2025-07-18T20:33:42Z

clang/include/clang/Basic/Attr.td

@@ -1641,6 +1641,13 @@ def DeviceKernel : DeclOrTypeAttr {
 }];
 }

+def SYCLExternal : InheritableAttr {
+  let Spellings = [Clang<"sycl_external">];


Suggested change

let Spellings = [Clang<"sycl_external">];

let Spellings = [CXX11<"sycl_external">];

Alternatively:

Suggested change

let Spellings = [Clang<"sycl_external">];

let Spellings = [CXX11<"sycl_external">, C23<"sycl_external">];

We use the Clang spelling for sycl_kernel_entry_point, what would be the reason for doing differently here? The attribute is an implementation detail used to provide the SYCL_EXTERNAL functionality, so shouldn't be directly written by SYCL programmers. Are you suggesting excluding the GNU spelling?

SYCL is only relevant for C++, so the C23 spelling wouldn't be desired.

If we switch to (only) the CXX11 spelling, I think the clang namespace should be retained.

Suggested change

let Spellings = [Clang<"sycl_external">];

let Spellings = [CXX11<"clang", "sycl_external">];

We use the Clang spelling for sycl_kernel_entry_point, what would be the reason for doing differently here? The attribute is an implementation detail used to provide the SYCL_EXTERNAL functionality, so shouldn't be directly written by SYCL programmers. Are you suggesting excluding the GNU spelling?

SYCL is only relevant for C++, so the C23 spelling wouldn't be desired.

If we switch to (only) the CXX11 spelling, I think the clang namespace should be retained.

Ah! Yes, I was suggesting to dump the GNU spelling. Keeping the clang namespace is also the right thing to do, so your suggestion is the right one.

Ok, I'm fine with dropping the GNU spelling. But in that case, we should also change the sycl_kernel_entry_point attribute to match (either in this PR or in a separate one).

Ok, I'm fine with dropping the GNU spelling. But in that case, we should also change the sycl_kernel_entry_point attribute to match (either in this PR or in a separate one).

Agreed. SLIGHT preference for separate PR (as I feel like we're almost done with this one, so for the sake of expedience).

Separate PR it is!
I'll follow this spelling: let Spellings = [CXX11<"clang", "sycl_external">];

This change fails two tests; will fix.
Clang :: CodeGenSPIRV/Builtins/generic_cast_to_ptr_explicit.c
Clang :: CodeGenSPIRV/Builtins/ids_and_ranges.c

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/lib/AST/ASTContext.cpp

erichkeane · 2025-07-18T20:36:04Z

clang/lib/Sema/SemaDecl.cpp

+    //   Redeclarations of the function in the same translation unit may
+    //   optionally use SYCL_EXTERNAL, but this is not required.
+    const SYCLExternalAttr *SEA = New->getAttr<SYCLExternalAttr>();
+    if (SEA && !Old->hasAttr<SYCLExternalAttr>()) {


To be consistent with the rest of our attributes, I'd probably suggest we support adding it up until definition. We can then do a conformance warning instead of error, but still let it work.

That might be reasonable. I think we might still have to make it an error if there was an ODR-use before the definition though. I'd have to think about that more.

@tahonermann - do you have any additional thoughts here?
Thank you!

clang/lib/Sema/SemaDecl.cpp

erichkeane · 2025-07-18T20:38:32Z

clang/lib/Sema/SemaDecl.cpp

@@ -12251,6 +12264,9 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
  if (NewFD->hasAttr<SYCLKernelEntryPointAttr>())
    SYCL().CheckSYCLEntryPointFunctionDecl(NewFD);

+  if (NewFD->hasAttr<SYCLExternalAttr>())
+    SYCL().CheckSYCLExternalFunctionDecl(NewFD);


Hmm... this seems like it should be handled when doing the attribute 'visiting', why is it here instead of there?

The checks include checking for linkage which I don't think is necessarily computed at the time the attribute is visited.

Can you confirm that linkage isn't computed at that point? I would expect us to (since the entire declaration is read before we handle attributes) have it there, so it is a little surprising.

Also, I didn't see instantiation of this attribute, do we prevent it on function templates?

Linkage, or rather, external visibility, which is what we actually check, can depend on other attributes like VisibilityAttr. Checking here ensures that all attributes have been processed and therefore avoids visitation ordering issues.

The attribute is allowed on function templates and is automatically inherited by (implicit and explicit) instantiations (and explicit specializations which is incorrect according to the C++ standard). I don't think there is anything to do to handle instantiation.

We do have a testing gap to address yet though. We have good tests for diagnostics, but are missing a test to validate which symbols are actually emitted. We'll ensure that test exercises function templates.

Linkage, or rather, external visibility, which is what we actually check, can depend on other attributes like VisibilityAttr. Checking here ensures that all attributes have been processed and therefore avoids visitation ordering issues.

The attribute is allowed on function templates and is automatically inherited by (implicit and explicit) instantiations (and explicit specializations which is incorrect according to the C++ standard). I don't think there is anything to do to handle instantiation.

We do have a testing gap to address yet though. We have good tests for diagnostics, but are missing a test to validate which symbols are actually emitted. We'll ensure that test exercises function templates.

Ah, I see, the visibility attribute makes sense here, thank you for looking into that.

I don't think there is anything to do to handle instantiation.

We've had to do some work in the past for attribute instantiation, though simple ones might be automatic. Can you make sure that specializations/partial specializations are properly tested? And diagnose if linkage isn't right?

We'll add tests to make sure the attribute has the proper affect with regard to actually emitting symbols. Tests for diagnostics are already in place in clang/test/SemaSYCL/sycl-external-attr.cpp.

Though, hmm, I think we're missing a test for implicit instantiation; I don't think we should diagnose cases like this:

namespace { struct S9 {}; } struct T9 { using type = S9; }; template<typename> [[clang::sycl_external]] void func9() {} template<typename T> [[clang::sycl_external]] void test_func9() { func9<typename T::type>(); } template void test_func9<T9>(); // Ok; don't diagnose implicit instantiation of func9<S9>().

namespace { struct S9 {}; } struct T9 { using type = S9; }; template<typename> [[clang::sycl_external]] void func9() {} // error here : {{'sycl_external' can only be applied to functions with external linkage}} template<typename T> [[clang::sycl_external]] void test_func9() { func9<typename T::type>(); // note here: {{in instantiation of function template specialization 'func9<(anonymous namespace)::S9>' requested here}} } template void test_func9<T9>(); // note here: {{in instantiation of function template specialization 'test_func9<T9>' requested here}}

This case is being diagnosed at present. Will fix it.

To test for implicit instantiation, call FunctionDecl::getTemplateSpecializationInfo(). If it returns non-null, call getSpecializationKind() via the returned pointer. If that returns TSK_ImplicitInstantiation, then skip the diagnostic.

The suggested change to avoid diagnosing the abovementioned case affects cases even with explicit template specializations, such as the two preceding it in the test file. Using the canonical decls doesn't solve the problem. Further investigation is needed. I will leave the FIXME as it is for this case for follow up in a subsequent PR.

tahonermann

Thanks for the updates, @schittir. A few minor requests.

clang/lib/AST/ASTContext.cpp

clang/test/CodeGenSYCL/sycl-external-attr.cpp

tahonermann · 2025-07-28T17:03:31Z

clang/test/CodeGenSYCL/sycl-external-attr.cpp

+  [[clang::sycl_external]] virtual void BFunc1WithAttr() { int i = 1; }
+// CHECK: define linkonce_odr spir_func void @_ZN1B14BFunc1WithAttrEv


This might not be the result we want. Virtual function definitions have particular rules for when they are emitted and those rules are different for the Itanium and MSVC ABIs. In this case, the virtual function is defined as an inline function and emitting it is ok, but I think is also unnecessary. I think we should do the following:

Skip requiring a symbol for an unused inline function declared with sycl_external.

Add a test for a virtual function that is defined outside the class (a symbol should be emitted).

Add a test for an unused virtual function that is defined as inline outside the class (a symbol should not be emitted).

clang/test/CodeGenSYCL/sycl-external-attr.cpp

schittir added 2 commits May 16, 2025 10:03

Add sycl_external attribute

abdbf89

Fix test and remove space

f631d7a

tahonermann requested changes May 17, 2025

View reviewed changes

Address review comments #1

128ab1b

Fznamznon reviewed May 27, 2025

View reviewed changes

clang/lib/AST/ASTContext.cpp Outdated Show resolved Hide resolved

schittir added 4 commits May 29, 2025 12:31

Fix conditional and failing tests

118656c

Fix the remaining six failing tests

7c592a4

Fix formatting

90ead01

Merge branch 'main' into sycl_external

195a3cc

Fznamznon mentioned this pull request Jun 6, 2025

lGeneric cast integration for SYCL intel/llvm#18841

Merged

schittir added 3 commits June 9, 2025 13:17

Remove sycl_external attribute support to variables.

a0071d1

Support for functions is sufficient for SYCL 2020 spec conformance.

Rename test file

d20382c

Add tests for sycl_external attribute

65262ba

schittir changed the title ~~Add sycl_external attribute~~ [SYCL] Add sycl_external attribute Jun 10, 2025

Add code examples to sycl_external documentation

770c65e

schittir requested review from tahonermann and Fznamznon June 10, 2025 16:16

schittir marked this pull request as ready for review June 10, 2025 16:16

schittir changed the title ~~[SYCL] Add sycl_external attribute~~ [clang][SYCL Upstreaming] Add sycl_external attribute Jun 10, 2025

Fznamznon added the SYCL https://registry.khronos.org/SYCL label Jun 10, 2025

schittir changed the title ~~[clang][SYCL Upstreaming] Add sycl_external attribute~~ [clang][SYCL Upstreaming] Add sycl_external attribute and restrict emitting code Jun 10, 2025

schittir changed the title ~~[clang][SYCL Upstreaming] Add sycl_external attribute and restrict emitting code~~ [clang][SYCL Upstreaming] Add sycl_external attribute and restrict emitting device code Jun 10, 2025

Fznamznon reviewed Jun 10, 2025

View reviewed changes

schittir and others added 3 commits June 10, 2025 14:48

Merge branch 'main' into sycl_external

328d242

Update clang/lib/Sema/SemaDeclAttr.cpp

aab6f7d

Co-authored-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Address review comments -2

385ea37

Fznamznon changed the title ~~[clang][SYCL Upstreaming] Add sycl_external attribute and restrict emitting device code~~ [clang][SYCL] Add sycl_external attribute and restrict emitting device code Jun 11, 2025

tahonermann requested changes Jun 16, 2025

View reviewed changes

Merge branch 'main' into sycl_external

19d1660

schittir added 2 commits July 10, 2025 05:51

Fix newly failing tests by adding sycl_external attribute

a70e2df

Add constexpr and consteval test cases

45f7b09

tahonermann requested changes Jul 11, 2025

View reviewed changes

Use existing diagnostic and address other minor comments

4db4101

tahonermann requested changes Jul 11, 2025

View reviewed changes

tahonermann requested changes Jul 12, 2025

View reviewed changes

Add additional test cases and address review comments

931fd76

tahonermann reviewed Jul 15, 2025

View reviewed changes

clang/include/clang/Basic/AttrDocs.td Outdated Show resolved Hide resolved

Add test cases

e34f2a6

schittir requested review from tahonermann, erichkeane and bader July 16, 2025 19:18

tahonermann requested changes Jul 17, 2025

View reviewed changes

Add FIXME comments, enable diagnostics for host, remove a needless decl

13a68d5

Merge branch 'main' into sycl_external

2c6cf7f

schittir requested a review from tahonermann July 18, 2025 15:49

tahonermann approved these changes Jul 18, 2025

View reviewed changes

bader reviewed Jul 22, 2025

View reviewed changes

tahonermann reviewed Jul 23, 2025

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

bader approved these changes Jul 23, 2025

View reviewed changes

erichkeane reviewed Jul 23, 2025

View reviewed changes

schittir added 3 commits July 25, 2025 21:26

Add test cases and address comments

394fc32

Add test case where device code is not emitted without the attribute

96fcd60

Merge branch 'main' into sycl_external

8e42487

tahonermann requested changes Jul 28, 2025

View reviewed changes

Addressing further comments

1981b57

tahonermann mentioned this pull request Jul 30, 2025

[SYCL] Restrict the sycl_kernel_entry_point attribute spelling to C++11 style. #151405

Merged

	let Spellings = [Clang<"sycl_external">];
	let Spellings = [CXX11<"sycl_external">];

	let Spellings = [Clang<"sycl_external">];
	let Spellings = [CXX11<"sycl_external">, C23<"sycl_external">];

	let Spellings = [Clang<"sycl_external">];
	let Spellings = [CXX11<"clang", "sycl_external">];

		[[clang::sycl_external]] virtual void BFunc1WithAttr() { int i = 1; }
		// CHECK: define linkonce_odr spir_func void @_ZN1B14BFunc1WithAttrEv

[clang][SYCL] Add sycl_external attribute and restrict emitting device code #140282

Are you sure you want to change the base?

[clang][SYCL] Add sycl_external attribute and restrict emitting device code #140282

Uh oh!

Conversation

schittir commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

schittir commented May 19, 2025

Uh oh!

github-actions bot commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fznamznon commented Jul 10, 2025

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

schittir commented May 16, 2025 •

edited

Loading

github-actions bot commented May 26, 2025 •

edited

Loading