Skip to content

[SYCL] Free function kernels bugfix #19535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: sycl
Choose a base branch
from

Conversation

AlexeySachkov
Copy link
Contributor

When free function kernel is a function template which in turn has arguments that are also templates, we may not print the latter properly into integration header.

Specifically, if an argument of a templated free function kernel has an enum template argument of its own then namespace qualifiers for it will be missing.

This patch fixed that.

When free function kernel is a function template which in turn has
arguments that are also templates, we may not print the latter properly
into integration header.

Specifically, if an argument of a templated free function kernel has an
`enum` template argument of its own then namespace qualifiers for it
will be missing.

This patch fixed that.
void templated_on_B(ns::nested::feature_B<T, V> Arg) {}
template void templated_on_B(ns::nested::feature_B<int, 12>);

// CHECK: template <typename T, int V> void templated_on_B(ns::nested::feature_B<T, V, ns::nested::enum_B::A, ns::enum_A::C>);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: the exact thing which was missing is ns::nested:: for enum_B::A and ns:: for enum_A::C.

@@ -267,17 +267,17 @@ namespace Testing::Tests {
// CHECK-NEXT: return (void (*)(struct ns::Arg<class ns::ns1::hasDefaultArg<struct ns::notatuple>, int, 12, struct ns::notatuple>))simple1;
// CHECK-NEXT: }

// CHECK: template <typename T> void templated(ns::Arg<T, float, 3, ns::notatuple> , T end);
// CHECK: template <typename T> void templated(ns::Arg<T, float, 3, ns::notatuple, <>>, T);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two changes here:

  • names of arguments in forward-declarations were dropped intentionally, because that's unnecessary information
  • <> is the way how empty type packs are printed. I'm not 100% sure that this is a legal C++ code, but I checked that generated integration header compiles with clang

@AlexeySachkov AlexeySachkov marked this pull request as ready for review July 21, 2025 16:12
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner July 21, 2025 16:12
@dyniols
Copy link
Contributor

dyniols commented Jul 24, 2025

@AlexeySachkov Would you mind as well removing explicit accessor parameters from this test to check if it works with defaults ?

@AlexeySachkov AlexeySachkov requested a review from a team as a code owner July 24, 2025 10:40
@AlexeySachkov
Copy link
Contributor Author

@AlexeySachkov Would you mind as well removing explicit accessor parameters from this test to check if it works with defaults ?

Thanks for the pointer. I've expanded E2E testing in 414cb2a

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants