[SYCL-Upstreaming] Add support for host kernel launch stmt generation #51

Fznamznon · 2025-09-02T15:16:48Z

This adds generation of a call to sycl_enqueue_kernel_launch function aka "launcher" function. The launcher function can be a memeber of a class or a free function defined at namespace scope. The lookup is performed from SKEP attributed function scope. Because unqualified lookup requires Scope object present and it only exists during parsing stage and already EOLed at the point where templates instantiated, I had to move some parts of SYCLKernelCallStmt generation to earlier stages.

This adds generation of a call to sycl_enqueue_kernel_launch function aka "launcher" function. The launcher function can be a memeber of a class or a free function defined at namespace scope. The lookup is performed from SKEP attributed function scope. Because unqualified lookup requires Scope object present and it only exists during parsing stage and already EOLed at the point where templates instantiated, I had to move some parts of SYCLKernelCallStmt generation to earlier stages and now TreeTransform knows how to process SYCLKernelCallStmt. I also had to invent a new expression - UnresolvedSYCLKernelExpr which represents a string containing kernel name of a kernel that doesn't exist yet. This expression is supposed to be transformed to a StringLiteral during template instantiation phase. It should never reach AST consumers like CodeGen of constexpr evaluators. This still requires more testing and FIXME cleanups, but since it evolved into a quite complicated patch I'm pushing it for earlier feedback.

Fznamznon · 2025-09-02T15:17:29Z

Hey, @tahonermann , I'm unable to push to your fork for some reason, so here is a PR against your branch.

UnresolvedSYCLKernelNameExpr -> UnresolvedSYCLKernelLaunchExpr

clang/lib/AST/StmtPrinter.cpp

clang/include/clang/AST/RecursiveASTVisitor.h

clang/include/clang/AST/StmtSYCL.h

clang/lib/Sema/SemaExceptionSpec.cpp

clang/lib/Sema/SemaSYCL.cpp

tahonermann

I really like how this is coming together. I got about half way through adding comments. I'll have some more tomorrow.

clang/include/clang/AST/StmtSYCL.h

clang/include/clang/Sema/ScopeInfo.h

clang/lib/AST/StmtProfile.cpp

tahonermann · 2025-09-19T04:59:58Z

clang/lib/Sema/SemaDecl.cpp

+    auto *BodyCompound = dyn_cast_or_null<CompoundStmt>(Body);
+    // If Body is not a compound, that was a templated function and we don't
+    // need to build SYCLKernelCallStmt for it since it was already created by
+    // template instantiator.


I'm a little nervous about relying on a check for CompountStmt specifically here to determine what to build. I think there are three cases to consider:

A function template or templated function for which an UnresolvedSYCLKernelEntryPointStmt is needed.

An instantiated function template for which a SYCLKernelCallStmt was already instantiated.

An explicit function template specialization or a non-template function for which a SYCLKernelCallStmt is needed.

I think these can be differentiated as follows:

if FD->isTemplated() is true.

otherwise, if FD->getTemplateSpecializationInfo() is non-null and isTemplateInstantiation(FD->getTemplateSpecializationKind())` is true.

everything else.

Updated. FD->getTemplateSpecializationInfo() && isTemplateInstantiation(FD->getTemplateSpecializationKind()) seems to be breaking some tests. I used FD->isTemplateInstantiation() to detect the case instead.

Ok. I added the FD->getTemplateSpecializationInfo() condition because I wasn't sure what FD->getTemplateSpecializationKind() would return for a non-template function. But if it works as expected, then great.

There is a comment in the FunctionDecl::isTemplateInstantiation() definition that I don't really know what to make of. My best guess is that it relates to a possible ambiguous classification of an instantiated dependent explicit specialization. It could also be that the FIXME comment is old/stale; I didn't dig into it. The isTemplateInstantiation() name certainly suggests it is what we want here.

bool FunctionDecl::isTemplateInstantiation() const { // FIXME: Remove this, it's not clear what it means. (Which template // specialization kind?) return clang::isTemplateInstantiation(getTemplateSpecializationKind()); }

tahonermann · 2025-09-19T05:27:13Z

clang/test/SemaSYCL/sycl-host-kernel-launch.cpp

+}
+
+
+


Some other interesting cases to test. If it is easy to get these to work, great. My main concern is ensuring a reasonable diagnostic is issued if someone tries things like this.

Use of CRTP. I would expect this case to work with no additional effort.

template<typename> struct base_handler { template<typename KNT, typename... Ts> void sycl_enqueue_kernel_launch(const char*, Ts...) {} }; struct derived_handler : base_handler<derived_handler> { template<typename KNT, typename KT> [[clang::sycl_kernel_entry_point(KNT)]] void entry(KT k) { k(); } };

Use of CRTP with a dependent base class. I would expect this case to fail because, in ordinary C++, explicit qualification is required to access a member of a dependent base class.

template<typename> struct base_handler { template<typename KNT, typename... Ts> void sycl_enqueue_kernel_launch(const char*, Ts...) {} }; template<int> struct derived_handler : base_handler<derived_handler<N>> { template<typename KNT, typename KT> [[clang::sycl_kernel_entry_point(KNT)]] void entry(KT k) { k(); } };

Use of a variable template as the kernel launch function. I would be surprised if this worked without additional effort required. I can potentially see some interest in this working because it avoids ADL concerns.

template<typename KNT> struct kernel_launcher { template<typename... Ts> void operator()(const char*, Ts...) const {} }; template<typename KNT> kernel_launcher<KNT> sycl_enqueue_kernel_launch; struct handler { template<typename KNT, typename KT> [[clang::sycl_kernel_entry_point(KNT)]] void entry(KT k) { k(); } };

Use of CRTP. I would expect this case to work with no additional effort.

Works.

Use of CRTP with a dependent base class. I would expect this case to fail because, in ordinary C++, explicit qualification is required to access a member of a dependent base class.

Doesn't work for that exact reason. I'll see what can be done on Monday.

Use of a variable template as the kernel launch function. I would be surprised if this worked without additional effort required. I can potentially see some interest in this working because it avoids ADL concerns.

Works.

Doesn't work for that exact reason. I'll see what can be done on Monday.

I think we should probably let this fail since we don't have a way for the programmer to express an intent to look in dependent base classes. Perhaps the diagnostic could be improved though. Or a warning issued if overload resolution could have selected it (if the call had explicit qualification) but found another candidate. I don't think much effort is warranted for this though.

That's cool that the variable template case just worked without additional effort!

Perhaps the diagnostic could be improved though. Or a warning issued if overload resolution could have selected it (if the call had explicit qualification) but found another candidate. I don't think much effort is warranted for this though.

Could you please be more specific?
Right now for the following test

template<int, int = 0> struct KN; template<typename> struct base_handler { template<typename KNT, typename... Ts> void sycl_enqueue_kernel_launch(const char*, Ts...) {} }; template<int N> struct derived_handler_t : base_handler<derived_handler_t<N>> { template<typename KNT, typename KT> [[clang::sycl_kernel_entry_point(KNT)]] void entry(KT k) { k(); } }; void bar() { derived_handler_t<13> H6; H6.entry<KN<13>>([](){}); }

the diagnostics emitted are

$ clang -cc1 -fsycl-is-host t39.cpp t39.cpp:14:18: error: unable to find suitable 'sycl_enqueue_kernel_launch' function for host code synthesis 14 | void entry(KT k) { k(); } | ^ t39.cpp:14:18: note: define 'sycl_enqueue_kernel_launch' function template to fix this problem 1 error generated.

What else could we potentially say?

We could do something like below. I don't know if this would be difficult or not.

$ clang -cc1 -fsycl-is-host t39.cpp t39.cpp:14:18: error: unable to find suitable 'sycl_enqueue_kernel_launch' function for host code synthesis 14 | void entry(KT k) { k(); } | ^ t39.cpp:5:8: note: explicit qualification required to use member 'sycl_enqueue_kernel_launch' from dependent base class; define 'sycl_enqueue_kernel_launch' in the derived class and forward arguments to the base class member. 1 error generated.

From reviewing other diagnostics, I see that lookup in dependent base classes might work as a Microsoft extension. Does that case work if -fms-extensions is passed?

Does that case work if -fms-extensions is passed?

No

Ah, the extension isn't tied to -fms-extensions or -fms-compatibility, but rather to the target. See https://godbolt.org/z/qeEGh7qaG. I suspect the test will pass if the target is set to x86_64-windows-msvc. MSVC accepts the code in the link in C++17 or earlier modes or when proper two phase lookup is disabled with /ZctwoPhase- for C++20 or later.

@tahonermann , do we want to accept this or do we want to diagnose this? It seems it is accepted for MSVC due to additional hacks somewhere. I see that for normal C++ path and MSVC we actually come to the CallExpr generation during template instantiation with an empty LookupResult object and built some kind of "recovery" CallExpr.

It seems both diagnostics - the error and the msvc extension warning are emitted by Sema::DiagnoseEmptyLookup which then calls DiagnoseDependentMemberLookup. What I don't like here is that it walks DeclContexts and "emulates" the lookup. I definitely don't want to write that. I could adapt Sema::DiagnoseEmptyLookup for SYCL but that would need additional flags to control its behavior for that special SYCL case which I'm not sure is worth it.

I'm not inclined to do anything special for this case. How about we add RUN lines that exercise the x86_64-windows-msvc target and conditionalize any differences in expected diagnostics accordingly? I would expect only the dependent CRTP case to be affected.

My earlier investigation of the MSVC compatibility was incomplete. I remember being frustrated at the time that I couldn't get the ext_found_in_dependent_base diagnostic to be issued. It seems that diagnostic is only issued if the derived class template is instantiated and my test didn't do that. https://godbolt.org/z/E1ToqrdKb demonstrates the extension warning and shows that, contrary to my expectations, MSVC does actually reject such use (in C++20 mode; it accepts it in C++17 mode; clang doesn't differentiate those cases which is fine). Your test already does this, so once additional RUN lines for Windows are added, that diagnostic should be captured by the test. I think this suffices to address this corner case. For anyone that wants this to work, they can add an inline sycl_kernel_launch() member to the derived class template that forwards to the dependent base class.

Added the Windows RUN line.

Co-authored-by: Tom Honermann <tom@honermann.net>

clang/include/clang/AST/StmtSYCL.h

Co-authored-by: Tom Honermann <tom@honermann.net>

tahonermann

A couple more minor comments. I'm inclined to merge this once we finish with these.

clang/lib/Sema/SemaSYCL.cpp

tahonermann

Thank you @Fznamznon, merging...

aelovikov-intel

I think this needs a doc/<something> that would provide the snippet of the code generated by the FE, unless destination branch already has that.

tahonermann · 2025-09-29T20:37:59Z

I think this needs a doc/<something> that would provide the snippet of the code generated by the FE, unless destination branch already has that.

@aelovikov-intel, the destination branch has that documentation here. This includes some of my own changes as well as Mariya's; see the full PR here. Feel free to review and comment on that PR!

Fznamznon added 8 commits September 3, 2025 07:40

Remove a fixme from SemaSYCL

e0bb6e9

Do not crash if original body was invalid

ad0065b

Add AST test for skep-attributed member

145e850

Fix a warning

9c91fce

Extend codegen test a bit

07967ea

Find and replace

520a4e6

UnresolvedSYCLKernelNameExpr -> UnresolvedSYCLKernelLaunchExpr

Implement the thing

319caa5

One more find and replace

2c89c01

Fznamznon commented Sep 12, 2025

View reviewed changes

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

Fznamznon added 3 commits September 12, 2025 06:09

I don't know how it looks like

aa02c24

Find and replace again

c3d3035

Switch to UnresolvedSYCLKernelEntryPointStmt

cb314fe

Fznamznon commented Sep 17, 2025

View reviewed changes

clang/include/clang/AST/RecursiveASTVisitor.h Outdated Show resolved Hide resolved

Fznamznon commented Sep 17, 2025

View reviewed changes

clang/include/clang/AST/StmtSYCL.h Outdated Show resolved Hide resolved

Fznamznon commented Sep 17, 2025

View reviewed changes

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

Fznamznon commented Sep 17, 2025

View reviewed changes

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

Fznamznon added 3 commits September 18, 2025 12:58

Apply suggestions from code review

76d904b

Remove log.txt

6f2541b

Implement visiting

db002bb

tahonermann reviewed Sep 19, 2025

View reviewed changes

Fznamznon and others added 5 commits September 19, 2025 05:43

Add tests

345e7b7

Apply suggestions from code review

2c155ef

Co-authored-by: Tom Honermann <tom@honermann.net>

IdExpr -> KernelLaunchIdExpr

6d7e4c1

Don't rely on compound

7e3a0bf

UnresolvedSYCLKernelEntryPointStmt -> UnresolvedSYCLKernelCall

b48996e

Fznamznon commented Sep 19, 2025

View reviewed changes

clang/include/clang/AST/StmtSYCL.h Outdated Show resolved Hide resolved

Fznamznon added 2 commits September 24, 2025 05:55

Fix warnings

1193734

Rename sycl_enqueue_kernel_launch -> sycl_kernel_launch

c83509b

Apply suggestions from code review

cb21a34

Co-authored-by: Tom Honermann <tom@honermann.net>

tahonermann reviewed Sep 25, 2025

View reviewed changes

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

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

Fznamznon added 2 commits September 25, 2025 02:47

Remove array decay

07402a9

Add windows run line to the sema test

1b2d1dd

tahonermann approved these changes Sep 29, 2025

View reviewed changes

tahonermann merged commit dd8f6cf into tahonermann:sycl-upstream-fe-sycl_kernel_entry_point-host Sep 29, 2025

aelovikov-intel reviewed Sep 29, 2025

View reviewed changes

[SYCL-Upstreaming] Add support for host kernel launch stmt generation #51

[SYCL-Upstreaming] Add support for host kernel launch stmt generation #51

Uh oh!

Conversation

Fznamznon commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fznamznon commented Sep 2, 2025

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!

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!

Choose a reason for hiding this comment

Uh oh!

Fznamznon Sep 19, 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!

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!

Fznamznon Sep 24, 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!

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tahonermann left a comment

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel left a comment • 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!

tahonermann commented Sep 29, 2025

Uh oh!

Uh oh!

Fznamznon commented Sep 2, 2025 •

edited

Loading

Fznamznon Sep 19, 2025 •

edited

Loading

Fznamznon Sep 24, 2025 •

edited

Loading

aelovikov-intel left a comment •

edited

Loading