Skip to content
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

[SYCL][clang] Emit default template arguments in integration header #16005

Merged
merged 10 commits into from
Nov 18, 2024

Conversation

Fznamznon
Copy link
Contributor

@Fznamznon Fznamznon commented Nov 6, 2024

For free function kernels support clang forward declares the kernel itself as well as its parameter types. In case a free function kernel has a parameter that is templated and has a default template argument, all template arguments including arguments that match default arguments must be printed in kernel's forward declarations, for example

template <typename T, typename = int> struct Arg {
  T val;
};

// For the kernel
SYCL_EXT_ONEAPI_FUNCTION_PROPERTY(
    (ext::oneapi::experimental::nd_range_kernel<1>)) void foo(Arg<int> arg) {
  arg.val = 42;
}

// Integration header must contain
void foo(Arg<int, int> arg);

Unfortunately, even though integration header emission already has extensive support for forward declarations priting, some modifications to clang's type printing are still required, since neither of existing PrintingPolicy flags help to reach the correct result.
Using SuppressDefaultTemplateArgs = true doesn't help without printing canonical types, printing canonical types for the case like

template <typename T>
SYCL_EXT_ONEAPI_FUNCTION_PROPERTY(
    (ext::oneapi::experimental::nd_range_kernel<1>)) void foo(Arg<T> arg) {
  arg.val = 42;
}
// Printing canonical types is causing the following integration header
template <typename T>
void foo(Arg<type-parameter-0-0, int> arg);

Using SkipCanonicalizationOfTemplateTypeParms field of printing policy doesn't help here since at the one point where it is checked we take canonical type of Arg, not its parameters and it will contain template argument types in canonical type after that.

For free function kernels support clang forward declares the kernel
itself as well as its parameter types. In case a free function kernel has a
parameter that is templated and has a default template argument, all
template arguments including arguments that match default arguments must
be printed in kernel's forward declarations, for example

```
template <typename T, typename = int> struct Arg {
  T val;
};

// For the kernel
SYCL_EXT_ONEAPI_FUNCTION_PROPERTY(
    (ext::oneapi::experimental::nd_range_kernel<1>)) void foo(Arg<int> arg) {
  arg.val = 42;
}

// Integration header must contain
void foo(Arg<int, int> arg);
```

Unfortunately, even though integration header emission already has
extensive support for forward declarations priting,
some modifications to clang's type printing are still required.
infrastructure, since neither of existing PrintingPolicy flags help to
reach the correct result.
Using `SuppressDefaultTemplateArgs = true` doesn't help without printing
canonical types, printing canonical types for the case like
```
template <typename T>
SYCL_EXT_ONEAPI_FUNCTION_PROPERTY(
    (ext::oneapi::experimental::nd_range_kernel<1>)) void foo(Arg<T> arg) {
  arg.val = 42;
}
// Printing canonical types is causing the following integration header
template <typename T>
void foo(Arg<type-parameter-0-0, int> arg);
```

Using `SkipCanonicalizationOfTemplateTypeParms` field of printing policy
doesn't help here since at the one point where it is checked we take
canonical type of `Arg`, not its parameters and it will contain template
argument types in canonical type after that.
@Fznamznon Fznamznon marked this pull request as ready for review November 11, 2024 17:02
@Fznamznon Fznamznon requested a review from a team as a code owner November 11, 2024 17:02
Copy link
Contributor

@lbushi25 lbushi25 left a comment

Choose a reason for hiding this comment

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

The tests for work group memory arguments used with free function kernels are now passing with these changes so for me this looks good.

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
Policy.FullyQualifiedName = true;
Policy.EnforceScopeForElaboratedTypes = true;

// SuppressDefaultTemplateArguments is a downstream addition that suppresses
Copy link
Contributor

Choose a reason for hiding this comment

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

I think an example in the comments would be helpful here. Probably the same one in PR description. Just reading these is very confusing without context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, thanks!

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved
@Fznamznon
Copy link
Contributor Author

@elizabethandrews , are you ok with the change?

@Fznamznon
Copy link
Contributor Author

@intel/llvm-gatekeepers , the PR is ready for submit. Please merge.

@sommerlukas sommerlukas merged commit 08a2edc into intel:sycl Nov 18, 2024
13 checks passed
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