[SYCL] Add warning for attributes applied to non-kernel functions #15154

dyniols · 2024-08-21T15:37:12Z

According to 5.8.1. Kernel attributes, kernel attributes can only be applied to SYCL kernel functions, but not to a regular device functions.

Based on this information, this pull request introduces a warning diagnostic whenever specified attributes are applied to regular device functions.

…functions

dyniols · 2024-08-26T14:46:07Z

@intel/dpcpp-cfe-reviewers Can you review?

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/include/clang/Sema/SemaSYCL.h

clang/lib/Sema/Sema.cpp

clang/lib/Sema/SemaSYCL.cpp

…functions

dyniols · 2024-08-28T09:45:13Z

@premanandrao I did requested changes can you look over them? Thanks

clang/lib/Sema/SemaSYCL.cpp

clang/test/SemaSYCL/sycl-attr-warn-non-kernel.cpp

clang/test/SemaSYCL/check-work-group-size-hint-device.cpp

elizabethandrews

I am not sure if my suggestion of looking into deferred diagnostics was the right one here. What happens when you modify collectSYCLAttributes and emit the warning when DirectlyCalled >1

clang/lib/Sema/SemaSYCL.cpp

…functions

dyniols · 2024-09-02T09:37:20Z

@elizabethandrews Do you know how to check if function is a kernel in CollectSyclExternalFuncs?

…functions

dyniols · 2024-09-04T12:11:09Z

Okay, I investigated it, and it seems that dependency checks on attributes were unnecessary.

dyniols · 2024-09-09T14:54:04Z

@elizabethandrews Could you look at my changes?

clang/lib/Sema/SemaSYCL.cpp

clang/include/clang/Sema/SemaSYCL.h

clang/test/SemaSYCL/sycl-attr-warn-non-kernel.cpp

elizabethandrews · 2024-09-10T19:07:03Z

clang/test/SemaSYCL/check-work-group-size-hint-device.cpp

@@ -17,7 +17,8 @@

 // Produce a conflicting attribute warning when the args are different.
 [[sycl::work_group_size_hint(4, 1, 1)]] void f3();    // expected-note {{previous attribute is here}}
-[[sycl::work_group_size_hint(1, 1, 32)]] void f3() {} // expected-warning {{attribute 'work_group_size_hint' is already applied with different arguments}}


The additional warnings in all these tests makes the tests a bit messy IMO. I would just remove (if we already test that functionality elsewhere) or rewrite the incorrect tests to not generate this warning. But this might just be me nit-picking. @premanandrao @smanna12 what do you think?

I think in this specific case, I need to remove
[[sycl::work_group_size_hint(1, 1, 32)]] void f3() {}
because it can only be a host or device function, but not a kernel. As a result, applying this attribute generates a warning.

I understand. My point was that these test cases are no longer really relevant in this context. For example in this case, if we already have other tests testing work_group_size_hint incorrectly applied on the kernel, we don't need to be testing it on device/host functions since we are already warning they are incorrect as per your new warning anyway. It is not a big deal keeping these tests. It just makes the test cases a bit messy IMO.

I am ok submitting this PR as-is and cleaning up the tests in a separate PR if the other reviewers @premanandrao and @smanna12 want these tests cleaned up as well.

I would like the tests cleaned up, but not necessarily as part of this PR. Are you okay with that @elizabethandrews?

I would like the tests cleaned up, but not necessarily as part of this PR

I have same opinion. Agree with @elizabethandrews and @premanandrao

Are you okay with that @elizabethandrews?

Works for me. @DYNIO-INTEL please clean up the tests in a follow up PR

It works for me too. I will clean up the tests in a follow up PR.

…functions

elizabethandrews · 2024-09-13T17:18:00Z

clang/test/SemaSYCL/check-work-group-size-hint-device.cpp

@@ -17,7 +17,8 @@

 // Produce a conflicting attribute warning when the args are different.
 [[sycl::work_group_size_hint(4, 1, 1)]] void f3();    // expected-note {{previous attribute is here}}
-[[sycl::work_group_size_hint(1, 1, 32)]] void f3() {} // expected-warning {{attribute 'work_group_size_hint' is already applied with different arguments}}


I understand. My point was that these test cases are no longer really relevant in this context. For example in this case, if we already have other tests testing work_group_size_hint incorrectly applied on the kernel, we don't need to be testing it on device/host functions since we are already warning they are incorrect as per your new warning anyway. It is not a big deal keeping these tests. It just makes the test cases a bit messy IMO.

I am ok submitting this PR as-is and cleaning up the tests in a separate PR if the other reviewers @premanandrao and @smanna12 want these tests cleaned up as well.

clang/test/SemaSYCL/sycl-attr-warn-non-kernel.cpp

…functions

smanna12

LGTM. Thank you

steffenlarsen · 2024-09-17T11:32:29Z

@mdtoguchi - Driver/sycl.c on Windows has been failing in post-commit as of yesterday for unrelated changes (#15297) and this PR is the first case I've seen it fail in pre-commit. Do you have any idea of how to address it? Can we disable it temporarily?

mdtoguchi · 2024-09-17T13:12:17Z

@mdtoguchi - Driver/sycl.c on Windows has been failing in post-commit as of yesterday for unrelated changes (#15297) and this PR is the first case I've seen it fail in pre-commit. Do you have any idea of how to address it? Can we disable it temporarily?

@steffenlarsen, it should be OK to disable temporarily (maybe just use REQUIRES: system-linux) until I can figure out what is going on.

steffenlarsen · 2024-09-17T15:03:46Z

Failure in Driver/sycl.c for Windows is known and will be disabled in #15416.

dyniols added 2 commits August 21, 2024 08:28

issue warning for attributes that are applied to non-kernel functions

b4d5226

fix formatting

d129a4b

dyniols temporarily deployed to WindowsCILock August 21, 2024 15:38 — with GitHub Actions Inactive

dyniols marked this pull request as ready for review August 21, 2024 15:43

dyniols requested a review from a team as a code owner August 21, 2024 15:43

dyniols temporarily deployed to WindowsCILock August 21, 2024 16:37 — with GitHub Actions Inactive

dyniols added 2 commits August 22, 2024 04:07

Merge remote-tracking branch 'intel/sycl' into attributes_non_kernel_…

20c6f73

…functions

remove device_has because it allowed to use in non kernel functions

148c2c5

dyniols temporarily deployed to WindowsCILock August 22, 2024 11:32 — with GitHub Actions Inactive

dyniols temporarily deployed to WindowsCILock August 22, 2024 12:06 — with GitHub Actions Inactive

premanandrao reviewed Aug 27, 2024

View reviewed changes

dyniols added 2 commits August 28, 2024 10:54

Merge remote-tracking branch 'intel/sycl' into attributes_non_kernel_…

c0dc57a

…functions

review suggestion changes

9d1b33b

dyniols had a problem deploying to WindowsCILock August 28, 2024 09:41 — with GitHub Actions Error

fixing formatting

b05cade

dyniols temporarily deployed to WindowsCILock August 28, 2024 09:53 — with GitHub Actions Inactive

dyniols temporarily deployed to WindowsCILock August 28, 2024 10:28 — with GitHub Actions Inactive

elizabethandrews reviewed Aug 30, 2024

View reviewed changes

smanna12 reviewed Aug 30, 2024

View reviewed changes

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

smanna12 reviewed Aug 30, 2024

View reviewed changes

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

Merge remote-tracking branch 'intel/sycl' into attributes_non_kernel_…

89f7b6c

…functions

dyniols added 3 commits September 3, 2024 11:34

Merge remote-tracking branch 'intel/sycl' into attributes_non_kernel_…

e6f059f

…functions

review suggestion changes v2

35361dc

formatting fix

8ab3a14

dyniols requested review from elizabethandrews and smanna12 September 3, 2024 09:49

fixed formatting

b03fafa

dyniols temporarily deployed to WindowsCILock September 4, 2024 12:11 — with GitHub Actions Inactive

dyniols requested a review from premanandrao September 4, 2024 12:11

dyniols temporarily deployed to WindowsCILock September 4, 2024 13:23 — with GitHub Actions Inactive

elizabethandrews reviewed Sep 10, 2024

View reviewed changes

dyniols added 2 commits September 11, 2024 09:55

Merge remote-tracking branch 'intel/sycl' into attributes_non_kernel_…

3aebde5

…functions

naming update and added new test cases

e1f9800

dyniols temporarily deployed to WindowsCILock September 11, 2024 09:20 — with GitHub Actions Inactive

dyniols requested a review from elizabethandrews September 11, 2024 09:20

dyniols temporarily deployed to WindowsCILock September 11, 2024 10:03 — with GitHub Actions Inactive

elizabethandrews reviewed Sep 13, 2024

View reviewed changes

dyniols added 2 commits September 16, 2024 14:36

Merge remote-tracking branch 'intel/sycl' into attributes_non_kernel_…

1d1f1e6

…functions

adding_test_case_for_out_of_line_definition

a4448c5

dyniols requested a review from elizabethandrews September 16, 2024 13:28

dyniols had a problem deploying to WindowsCILock September 16, 2024 13:29 — with GitHub Actions Failure

dyniols had a problem deploying to WindowsCILock September 16, 2024 14:30 — with GitHub Actions Error

dyniols had a problem deploying to WindowsCILock September 16, 2024 15:02 — with GitHub Actions Failure

elizabethandrews approved these changes Sep 16, 2024

View reviewed changes

smanna12 approved these changes Sep 16, 2024

View reviewed changes

dyniols temporarily deployed to WindowsCILock September 16, 2024 16:03 — with GitHub Actions Inactive

dyniols had a problem deploying to WindowsCILock September 17, 2024 10:46 — with GitHub Actions Failure

dyniols temporarily deployed to WindowsCILock September 17, 2024 11:27 — with GitHub Actions Inactive

premanandrao approved these changes Sep 17, 2024

View reviewed changes

steffenlarsen merged commit 227af47 into intel:sycl Sep 17, 2024
11 of 12 checks passed

dyniols mentioned this pull request Sep 19, 2024

[SYCL] Tests cleanup for warnings issued due to attributes applied to non-kernel functions #15440

Closed

[SYCL] Add warning for attributes applied to non-kernel functions #15154

[SYCL] Add warning for attributes applied to non-kernel functions #15154

Uh oh!

Conversation

dyniols commented Aug 21, 2024

Uh oh!

dyniols commented Aug 26, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dyniols commented Aug 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elizabethandrews 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!

Uh oh!

Uh oh!

dyniols commented Sep 2, 2024

Uh oh!

dyniols commented Sep 4, 2024

Uh oh!

dyniols commented Sep 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elizabethandrews Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

dyniols Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Sep 13, 2024 • 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!

premanandrao Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

smanna12 Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

dyniols Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Sep 13, 2024 • 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!

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Sep 17, 2024

Uh oh!

mdtoguchi commented Sep 17, 2024

Uh oh!

steffenlarsen commented Sep 17, 2024

Uh oh!

Uh oh!

Uh oh!

elizabethandrews left a comment •

edited

Loading

elizabethandrews Sep 13, 2024 •

edited

Loading

elizabethandrews Sep 13, 2024 •

edited

Loading