Skip to content

[clang][SYCL] Allow structs as free function kernel arguments #15334

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.

Sign up for GitHub

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

Merged
merged 19 commits into from
Sep 26, 2024
Merged

[clang][SYCL] Allow structs as free function kernel arguments #15334

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
80491c3
[clang][SYCL] Allow structs as free function kernel arguments
Fznamznon Sep 9, 2024
3fbb50c
Add AST test
Fznamznon Sep 10, 2024
3e5332e
Add test for integration header generation, extend e2e test
Fznamznon Sep 10, 2024
c8bf832
Maybe fix test on win
Fznamznon Sep 10, 2024
95cfe15
Clean up SemaSYCL
Fznamznon Sep 10, 2024
6d8a28f
Diagnose bad struct kernel parameters
Fznamznon Sep 10, 2024
7b17d82
Fix format
Fznamznon Sep 10, 2024
efcfc47
Remove obsolete TODO
Fznamznon Sep 10, 2024
4e293da
Add codegen test to see address spaces
Fznamznon Sep 10, 2024
34524f4
Incorportate review feedback
Fznamznon Sep 11, 2024
dd52ed5
Cleanup
Fznamznon Sep 12, 2024
f9a1641
Merge branch 'sycl' into free-functions-struct-params
Fznamznon Sep 13, 2024
ef3ca91
Diagnose special types in structs since they are not yet supported.
Fznamznon Sep 19, 2024
431a405
Rename createStructTemporary -> createCopyInitExpr
Fznamznon Sep 19, 2024
0b88367
Improve diagnotic messages
Fznamznon Sep 20, 2024
ce98062
Create isForwardDeclarable
Fznamznon Sep 23, 2024
9885f5a
Merge branch 'sycl' into free-functions-struct-params
Fznamznon Sep 25, 2024
2eb3d61
Resolve diagnostic concerns
Fznamznon Sep 25, 2024
9622e3e
Merge branch 'sycl' into free-functions-struct-params
Fznamznon Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12414,6 +12414,14 @@ def err_sycl_kernel_incorrectly_named : Error<
"'-fsycl-unnamed-lambda' to enable unnamed kernel lambdas"
"}0">;

// SYCL free function kernels extension.
def err_bad_free_function_kernel_param_type : Error<
"%0 cannot be used as the type of a free function kernel parameter">;
def note_free_function_kernel_param_type_not_fwd_declarable : Note<
"%0 is not forward declarable">;
def note_free_function_kernel_param_type_not_supported : Note<
"%0 is not yet supported as free function kernel parameter">;
Copy link
Contributor

@tahonermann tahonermann Sep 24, 2024

Choose a reason for hiding this comment

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

Suggested change
"%0 is not yet supported as free function kernel parameter">;
"%0 is not yet supported as a free function kernel parameter">;

Copy link
Contributor

@elizabethandrews elizabethandrews Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think having different diagnostics for free function parameter type and kernel param type is unnecessary. Other than the diagnostics you added/modified in this PR, won't other invalid types in free functions generate the old diagnostic err_bad_kernel_param_type? IMO I think we can keep the old diagnostic, passing type to it. The note diagnostic can be generated additionally for extra information where required

Copy link
Contributor

@tahonermann tahonermann Sep 24, 2024

Choose a reason for hiding this comment

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

I agree the diagnostics could be unified, but that would require changes to the existing diagnostics since their message text explicitly references the concept of a "kernel name". I'm not sure such unification would provide much benefit.

def err_nullptr_t_type_in_sycl_kernel : Error<"%0 is an invalid kernel name, "
                                        "'std::nullptr_t' is declared in the 'std' namespace ">;
def err_invalid_std_type_in_sycl_kernel : Error<"%0 is an invalid kernel name, " 
                                          "%q1 is declared in the 'std' namespace ">;
def err_sycl_kernel_incorrectly_named : Error<
  "%select{%1 is invalid; kernel name should be forward declarable "
  "at namespace scope"
  "|unscoped enum %1 requires fixed underlying type"
  "|unnamed type %1 is invalid; provide a kernel name, or use "
  "'-fsycl-unnamed-lambda' to enable unnamed kernel lambdas"
  "}0">;

Copy link
Contributor Author

@Fznamznon Fznamznon Sep 25, 2024

Choose a reason for hiding this comment

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

I used err_bad_kernel_param_type instead of a new one.

@tahonermann , I think @elizabethandrews meant another set of diagnostics.


def err_sycl_kernel_not_function_object
: Error<"kernel parameter must be a lambda or function object">;
def err_sycl_restrict : Error<
Expand Down
Loading
Loading