[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 <[email protected]>

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

There seems to be an issue with the use of sycl_kernel_entry_point attribute vs __builtin_unique_stable_name

clang/test/CodeGenSYCL/unique_stable_name_windows_diff.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!

tahonermann · 2025-07-17T19:52:40Z

clang/lib/Sema/SemaDecl.cpp

+    if (LangOpts.SYCLIsDevice) {
+      const SYCLExternalAttr *SEA = New->getAttr<SYCLExternalAttr>();
+      if (SEA && !Old->hasAttr<SYCLExternalAttr>()) {
+        Diag(SEA->getLocation(), diag::err_attribute_missing_on_first_decl)
+            << SEA;
+        Diag(Old->getLocation(), diag::note_previous_declaration);
+        New->dropAttr<SYCLExternalAttr>();
+      }


Since we have decided to allow the attribute to be present during host compilation, we don't need to, and shouldn't, check for device compilation; diagnostics should be emitted during host compilation too.

Suggested change

if (LangOpts.SYCLIsDevice) {

const SYCLExternalAttr *SEA = New->getAttr<SYCLExternalAttr>();

if (SEA && !Old->hasAttr<SYCLExternalAttr>()) {

Diag(SEA->getLocation(), diag::err_attribute_missing_on_first_decl)

<< SEA;

Diag(Old->getLocation(), diag::note_previous_declaration);

New->dropAttr<SYCLExternalAttr>();

}

const SYCLExternalAttr *SEA = New->getAttr<SYCLExternalAttr>();

if (SEA && !Old->hasAttr<SYCLExternalAttr>()) {

Diag(SEA->getLocation(), diag::err_attribute_missing_on_first_decl)

<< SEA;

Diag(Old->getLocation(), diag::note_previous_declaration);

New->dropAttr<SYCLExternalAttr>();

tahonermann · 2025-07-17T19:54:04Z

clang/lib/Sema/SemaDecl.cpp

+  if (getLangOpts().SYCLIsDevice) {
+    if (FD->hasAttr<SYCLExternalAttr>()) {
+      Diag(FD->getLocation(), diag::err_sycl_attribute_invalid_main);
+      FD->setInvalidDecl();
+      return;
+    }
+  }


Diagnostics are desired for host compilation as well.

Suggested change

if (getLangOpts().SYCLIsDevice) {

if (FD->hasAttr<SYCLExternalAttr>()) {

Diag(FD->getLocation(), diag::err_sycl_attribute_invalid_main);

FD->setInvalidDecl();

return;

}

}

if (FD->hasAttr<SYCLExternalAttr>()) {

Diag(FD->getLocation(), diag::err_sycl_attribute_invalid_main);

FD->setInvalidDecl();

return;

}

tahonermann · 2025-07-17T20:04:21Z

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

+// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -std=c++17 -verify %s
+// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -std=c++20 -verify %s
+// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -std=c++23 -verify %s


Validate diagnostics for host compilation too.

Suggested change

// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -std=c++17 -verify %s

// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -std=c++20 -verify %s

// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -std=c++23 -verify %s

// RUN: %clang_cc1 -fsycl-is-host -fsyntax-only -std=c++17 -verify %s

// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -std=c++17 -verify %s

// RUN: %clang_cc1 -fsycl-is-host -fsyntax-only -std=c++20 -verify %s

// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -std=c++20 -verify %s

// RUN: %clang_cc1 -fsycl-is-host -fsyntax-only -std=c++23 -verify %s

// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -std=c++23 -verify %s

tahonermann · 2025-07-17T20:05:58Z

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

@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -verify %s


Validate diagnostics for host compilation too.

Suggested change

// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -verify %s

// RUN: %clang_cc1 -fsycl-is-host -fsyntax-only -verify %s

// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -verify %s

tahonermann · 2025-07-17T20:09:23Z

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

+// FIXME: this case should be diagnosed.
+// This attribute takes no arguments.
+[[clang::sycl_external()]] void bad1();


Just to make the desired intent a little more clear.

Suggested change

// FIXME: this case should be diagnosed.

// This attribute takes no arguments.

[[clang::sycl_external()]] void bad1();

// FIXME-expected-error@+1{{'clang::sycl_external' attribute takes no arguments}}

[[clang::sycl_external()]] void bad1();

tahonermann · 2025-07-17T20:10:38Z

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

+// RUN: %clang_cc1 -fsycl-is-device -std=c++17 -fsyntax-only -verify -DCPP17 %s
+// RUN: %clang_cc1 -fsycl-is-device -std=c++20 -fsyntax-only -verify -DCPP20 %s


Validate diagnostics for host compilation too.

Suggested change

// RUN: %clang_cc1 -fsycl-is-device -std=c++17 -fsyntax-only -verify -DCPP17 %s

// RUN: %clang_cc1 -fsycl-is-device -std=c++20 -fsyntax-only -verify -DCPP20 %s

// RUN: %clang_cc1 -fsycl-is-host -std=c++17 -fsyntax-only -verify -DCPP17 %s

// RUN: %clang_cc1 -fsycl-is-device -std=c++17 -fsyntax-only -verify -DCPP17 %s

// RUN: %clang_cc1 -fsycl-is-host -std=c++20 -fsyntax-only -verify -DCPP20 %s

// RUN: %clang_cc1 -fsycl-is-device -std=c++20 -fsyntax-only -verify -DCPP20 %s

tahonermann · 2025-07-17T20:11:28Z

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

+
+  struct UnnX {};


This type is no longer used.

Suggested change

struct UnnX {};

tahonermann · 2025-07-17T20:14:35Z

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

+// expected-error@+3 2{{'sycl_external' can only be applied to functions with external linkage}}
+namespace { struct S7 {}; }
+template<typename>
+[[clang::sycl_external]] void func7();
+template<> void func7<S7>() {}
+// expected-note@-1{{in instantiation of function template specialization 'func7<(anonymous namespace)::S7>' requested here}}


This case should not be diagnosed.

Suggested change

// expected-error@+3 2{{'sycl_external' can only be applied to functions with external linkage}}

namespace { struct S7 {}; }

template<typename>

[[clang::sycl_external]] void func7();

template<> void func7<S7>() {}

// expected-note@-1{{in instantiation of function template specialization 'func7<(anonymous namespace)::S7>' requested here}}

// FIXME: C++23 [temp.expl.spec]p12 states:

// ... Similarly, attributes appearing in the declaration of a template

// have no effect on an explicit specialization of that template.

// Clang currently instantiates and propagates attributes from a function

// template to its explicit specializations resulting in the following

// spurious error.

// expected-error@+3 2{{'sycl_external' can only be applied to functions with external linkage}}

namespace { struct S7 {}; }

template<typename>

[[clang::sycl_external]] void func7();

template<> void func7<S7>() {}

// expected-note@-1{{in instantiation of function template specialization 'func7<(anonymous namespace)::S7>' requested here}}

tahonermann · 2025-07-17T20:16:57Z

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

+// FIXME: This case is currently being diagnosed as an error because clang implements
+// default inheritance of attribute and explicit instantiation declaration names the
+// symbol that causes the instantiated specialization to have internal linkage.


This case is correctly diagnosed. Explicit function template instantiations inherit attributes from the function template. Explicit function template specializations are the ones that shouldn't inherit from the function template.

Suggested change

// FIXME: This case is currently being diagnosed as an error because clang implements

// default inheritance of attribute and explicit instantiation declaration names the

// symbol that causes the instantiated specialization to have internal linkage.

tahonermann · 2025-07-17T20:23:22Z

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

+namespace { struct S8 {}; }
+template<typename>
+void func8();
+template<> [[clang::sycl_external]] void func8<S8>() {}
+// expected-error@-1{{'clang::sycl_external' attribute does not appear on the first declaration}}
+// expected-note@-2{{previous declaration is here}}


Let's add a FIXME comment for this case. It is a weird corner case, but I have encountered programmers wanting to do this kind of thing recently. I think it should be well-formed, but I don't think it is worth trying to address now.

Suggested change

namespace { struct S8 {}; }

template<typename>

void func8();

template<> [[clang::sycl_external]] void func8<S8>() {}

// expected-error@-1{{'clang::sycl_external' attribute does not appear on the first declaration}}

// expected-note@-2{{previous declaration is here}}

// FIXME: The explicit function template specialization appears to trigger

// instantiation of a declaration from the primary template without the

// attribute leading to a spurious diagnostic that the sycl_external

// attribute is not present on the first declaration.

namespace { struct S8 {}; }

template<typename>

void func8();

template<> [[clang::sycl_external]] void func8<S8>() {}

// expected-error@-1{{'clang::sycl_external' attribute does not appear on the first declaration}}

// expected-note@-2{{previous declaration is here}}

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 <[email protected]>

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

schittir added 6 commits July 1, 2025 19:17

Merge branch 'main' into sycl_external

7893e90

Change the second RUN line to use -sycl-is-host

7e76afd

Switch to using sycl_external attr to pass the failing test

e8d26a2

There seems to be an issue with the use of sycl_kernel_entry_point attribute vs __builtin_unique_stable_name

Change diagnostic messages

82fa98a

Revert RUN line to -fsycl-is-device

e4d15eb

Revert test change

b38e578

Fznamznon reviewed Jul 7, 2025

View reviewed changes

clang/test/CodeGenSYCL/unique_stable_name_windows_diff.cpp Outdated Show resolved Hide resolved

schittir added 6 commits July 8, 2025 11:02

Merge branch 'main' into sycl_external

1d82fc1

Fix conflict resolution errors.

d751b43

Remove changes introduced from downstream.

2b22ed2

Update diagnostic messages in tests

1b3a198

Undo more downstream changes

568b569

Ungroup diagnostics and add test cases

0ab9ac5

schittir requested review from Fznamznon and tahonermann July 10, 2025 04:24

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

		@@ -0,0 +1,15 @@
		// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -verify %s

	// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -verify %s
	// RUN: %clang_cc1 -fsycl-is-host -fsyntax-only -verify %s
	// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -verify %s

		// RUN: %clang_cc1 -fsycl-is-device -std=c++17 -fsyntax-only -verify -DCPP17 %s
		// RUN: %clang_cc1 -fsycl-is-device -std=c++20 -fsyntax-only -verify -DCPP20 %s

	// FIXME: This case is currently being diagnosed as an error because clang implements
	// default inheritance of attribute and explicit instantiation declaration names the
	// symbol that causes the instantiated specialization to have internal linkage.

[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!

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

schittir commented May 16, 2025 •

edited

Loading

github-actions bot commented May 26, 2025 •

edited

Loading