SYCL free function namespace support #17585

dklochkov-emb · 2025-03-21T17:01:30Z

SYCL free function docs:
https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_oneapi_free_function_kernels.asciidoc

Changes should generate right forward declarations of any function(not template) and shim functions in namespace or not.

[SYCL] fix formatting [SYCL] remove unnecessary space

…namespace-support

[SYCL] include additional header in tests [SYCL] do not iterate through arguments twice

Fznamznon

Initial feedback. What about the templates though?

I left a bunch of mechanical comments, perhaps this link might be useful
https://llvm.org/docs/CodingStandards.html

You might also consider adding a release note for your change.

clang/test/CodeGenSYCL/free_function_default_template_arguments.cpp

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2025-03-27T13:20:33Z

clang/lib/Sema/SemaSYCL.cpp

+      [](raw_ostream &OS, const NamespaceDecl *NS) {}, OS, DC);
+}
+
+static bool insertFreeFunctionDeclaration(const PrintingPolicy &Policy,


Maybe a documentation comment here makes sense? The function is named insertFreeFunctionDeclaration yet it returns true if a namespace was inserted. Normally functions that are named like that return success status in a bool flag. Alternatively the bool return value can be converted to a reference parameter with a good name.

Reworked to have a separate class to emit free function and shim for that. Added comments for each method.

Fznamznon · 2025-03-27T13:22:58Z

clang/lib/Sema/SemaSYCL.cpp

+      Policy.SuppressTagKeyword = true;
+      Param->getType().print(ParmListWithNamesOstream, Policy);
+      Policy.SuppressTagKeyword = false;
+      ParmListWithNamesOstream << " " << Param->getNameAsString();


Could you please elaborate what this particular addition is trying to achieve, why the previous code did not suffice and how does it relate to namespace printing?

In general, ParamList contains only parameter types, i.e. for the function
void some_func(float a, float* b)
ParamList contains {float, float*}
It was added new list to have additional list of parameters with names to pass already existed tests for free function which checked generated header.
flag
Policy.SuppressTagKeyword = true;
forces printing without type tags, i.e. without words class and struct.

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2025-03-27T13:26:29Z

clang/lib/Sema/SemaSYCL.cpp

+      PrintNamespaces(O, FD);
+      NSInserted = true;
+    }
+    O << FD->getReturnType().getAsString() << " ";


I wonder why doing FD->print() does not suffice anymore?

FD->print() works fine for free function declared outside of namespace. If it is declared in it, print method will emit:
void NS1::NS2::some_func(...)
for function
namespace NS1::NS2{ void some_func(...){} }

The form
void NS1::NS2::some_func(...){}
works fine but forward declaration
void NS1::NS2::some_func(...) does not work. That is why it is needed to emit namespace first into intermediate header for function declaration.
P.S.
I reviewed Policy to use FD->print() but did not find.

Fznamznon · 2025-03-27T13:28:22Z

clang/test/CodeGenSYCL/free_function_default_template_arguments.cpp

 // CHECK-NEXT: }

 // CHECK: Forward declarations of kernel and its argument types:
 // CHECK: namespace ns {
 // CHECK: namespace ns1 {
 // CHECK-NEXT: template <typename A> class hasDefaultArg;
-// CHECK-NEXT: }
+// CHECK-NEXT: }}


Can we add more FE tests? Like with various combinations of namespaces around the free function kernel declaration? With inline namespace and not. Can we also test that codegen and semantic analysis is ok for free function kernels defined in a (maybe nested) namespace?

I added new e2e tests to check any possible namespaces: nested, anonymous, inline etc. Is it enough or add in these tests too? New tests do not check header directly but if something is emitted wrong, they will fail.

SYCL compiler is complicated and has a lot of components. If we only have a e2e test and it fails suddenly (for example after a pulldown), it may take a while to identify which component now has a problem.
This is one of reasons why we normally check each component separately with unit tests and everything together in e2e tests. FE-only tests are "unit" tests in this scenario. They will help more quickly to identify that the problem is in FE. They will also help people to fix any FE problems without needing to have sycl rt and device. So, I still encourage to add FE-only tests.

Good point, I did not see that these tests are units. Added new checks.

Well they are "unit" because clang is enormous itself and has its own unit tests but in terms of SYCL compiler we can consider them as unit tests.

sycl/test-e2e/Experimental/free_functions/namespace.cpp

sycl/test/e2e_test_requirements/no-unsupported-without-info.cpp

clang/lib/Sema/SemaSYCL.cpp

clang/test/CodeGenSYCL/free_function_int_header.cpp

aelovikov-intel · 2025-04-04T15:29:18Z

sycl/test-e2e/Experimental/free_functions/namespace.cpp

+concept NumericType = std::is_arithmetic_v<std::remove_reference_t<T>>;
+
+template <typename T>
+  requires NumericType<T>


I don't know if we allow unconditional C++20 in tests. I remember someone from Codeplay coming to me saying they have runners with old gcc on it. On the other hand, this is core compiler feature, not STL, so maybe nobody would complain.

I looked at the current tests before adding flag %cxx_std_optionc++20, few of them use it already. I guess it is ok.

aelovikov-intel · 2025-04-04T15:30:24Z

sycl/test-e2e/Experimental/free_functions/namespace.cpp

+    const float expected = 3.14f + static_cast<float>(i);
+    assert(ptr[i] == expected &&


Will it work with fast-math (or downstream)?

Yes, it works with fast math

dklochkov-emb added 6 commits March 7, 2025 07:40

[SYCL] add e2e test for namespace

b305e94

[SYCL] implement namespace support for free function

5ff7b2c

[SYCL] fix test issues after adding namespace support

12df05e

[SYCL] fix formatting [SYCL] remove unnecessary space

Merge remote-tracking branch 'upstream/sycl' into sycl-free-function-…

bd15ef7

…namespace-support

[SYCL] do not use macros in tests

65ef84f

[SYCL] include additional header in tests [SYCL] do not iterate through arguments twice

[SYCL] fix tests

117e97a

dklochkov-emb self-assigned this Mar 21, 2025

dklochkov-emb requested review from a team as code owners March 21, 2025 17:01

dklochkov-emb requested a review from KseniyaTikhomirova March 21, 2025 17:01

dklochkov-emb temporarily deployed to WindowsCILock March 21, 2025 17:01 — with GitHub Actions Inactive

dklochkov-emb marked this pull request as draft March 21, 2025 17:01

dklochkov-emb temporarily deployed to WindowsCILock March 21, 2025 17:29 — with GitHub Actions Inactive

[SYCL] exclude cuda from free function tests

c53312a

dklochkov-emb temporarily deployed to WindowsCILock March 25, 2025 10:46 — with GitHub Actions Inactive

dklochkov-emb had a problem deploying to WindowsCILock March 25, 2025 11:26 — with GitHub Actions Error

[SYCL] update unsupported list of tests

190ac32

dklochkov-emb temporarily deployed to WindowsCILock March 25, 2025 11:44 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock March 25, 2025 12:08 — with GitHub Actions Inactive

dklochkov-emb marked this pull request as ready for review March 26, 2025 09:49

[SYCL] fix typo

0858e70

dklochkov-emb temporarily deployed to WindowsCILock March 26, 2025 09:53 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock March 26, 2025 10:36 — with GitHub Actions Inactive

Fznamznon reviewed Mar 27, 2025

View reviewed changes

dklochkov-emb added 2 commits March 27, 2025 18:23

[SYCL] rework free functions to use a separate entity

32113db

[SYCL] Update code style

2183658

dklochkov-emb had a problem deploying to WindowsCILock April 1, 2025 09:37 — with GitHub Actions Error

[SYCL][E2E] fix formatting

157b39a

dklochkov-emb had a problem deploying to WindowsCILock April 1, 2025 09:48 — with GitHub Actions Failure

dklochkov-emb had a problem deploying to WindowsCILock April 1, 2025 10:09 — with GitHub Actions Error

dklochkov-emb temporarily deployed to WindowsCILock April 1, 2025 10:09 — with GitHub Actions Inactive

[SYCL][E2E] update free function integration header test

e3ff53a

dklochkov-emb temporarily deployed to WindowsCILock April 1, 2025 10:53 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock April 1, 2025 11:12 — with GitHub Actions Inactive

dklochkov-emb requested review from Fznamznon and aelovikov-intel April 1, 2025 14:19

aelovikov-intel reviewed Apr 1, 2025

View reviewed changes

sycl/test-e2e/Experimental/free_functions/namespace.cpp Show resolved Hide resolved

aelovikov-intel reviewed Apr 1, 2025

View reviewed changes

sycl/test/e2e_test_requirements/no-unsupported-without-info.cpp Outdated Show resolved Hide resolved

dklochkov-emb had a problem deploying to WindowsCILock April 1, 2025 15:30 — with GitHub Actions Error

[SYCL][E2E] do not update no-unsupported test

bdb3967

dklochkov-emb force-pushed the sycl-free-function-namespace-support branch from de6c1a4 to bdb3967 Compare April 1, 2025 15:31

dklochkov-emb temporarily deployed to WindowsCILock April 1, 2025 15:31 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock April 1, 2025 16:06 — with GitHub Actions Inactive

Fznamznon reviewed Apr 2, 2025

View reviewed changes

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

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

clang/test/CodeGenSYCL/free_function_int_header.cpp Show resolved Hide resolved

dklochkov-emb added 3 commits April 4, 2025 12:17

[SYCL] add new tests to cover free fuction namespace support

cae876a

[SYCL] remove unused variable

afb8b59

[SYCL][E2E] add new tests for free functions namespace support

09c7786

dklochkov-emb temporarily deployed to WindowsCILock April 4, 2025 12:27 — with GitHub Actions Inactive

dklochkov-emb temporarily deployed to WindowsCILock April 4, 2025 12:47 — with GitHub Actions Inactive

dklochkov-emb requested review from aelovikov-intel and Fznamznon April 4, 2025 13:17

aelovikov-intel reviewed Apr 4, 2025

View reviewed changes

aelovikov-intel approved these changes Apr 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SYCL free function namespace support #17585

SYCL free function namespace support #17585

dklochkov-emb commented Mar 21, 2025

Fznamznon left a comment

Fznamznon Mar 27, 2025

dklochkov-emb Mar 28, 2025

Fznamznon Mar 27, 2025

dklochkov-emb Mar 28, 2025

Fznamznon Mar 27, 2025

dklochkov-emb Mar 28, 2025

Fznamznon Mar 27, 2025

dklochkov-emb Mar 28, 2025

Fznamznon Mar 28, 2025

dklochkov-emb Mar 28, 2025

Fznamznon Mar 28, 2025

aelovikov-intel Apr 4, 2025

dklochkov-emb Apr 4, 2025

aelovikov-intel Apr 4, 2025

dklochkov-emb Apr 4, 2025

		const float expected = 3.14f + static_cast<float>(i);
		assert(ptr[i] == expected &&

SYCL free function namespace support #17585

Are you sure you want to change the base?

SYCL free function namespace support #17585

Conversation

dklochkov-emb commented Mar 21, 2025

Fznamznon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment