[SYCL] Add support for [[intel::reqd_sub_group_… #2137

smanna12 · 2020-07-20T06:39:58Z

…size()]]

This patch adds new spelling of IntelReqdSubGroupSize attribute
and generates a diagnostic which will tell that previous spelling
of the attribute is deprecated.

Signed-off-by: Soumi Manna [email protected]

…size()]] This patch adds new spelling of IntelReqdSubGroupSize attribute and generates a diagnostic which will tell that previous spelling of the attribute is deprecated. Signed-off-by: Soumi Manna <[email protected]>

Signed-off-by: Soumi Manna <[email protected]>

erichkeane · 2020-07-20T13:13:53Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -10969,6 +10969,9 @@ def err_ivdep_declrefexpr_arg : Error<
 def warn_ivdep_redundant : Warning <"ignoring redundant Intel FPGA loop "
  "attribute 'ivdep': safelen %select{INF|%1}0 >= safelen %select{INF|%3}2">,
  InGroup<IgnoredAttributes>;
+def warn_attribute_spelling_deprecated : Warning<
+  "previous spelling of attribute %0 is deprecated">,


User's aren't particularly knowledgeable about the fact that the same attribute can be spelled in 2 different ways. They tend to treat them as different attributes with similar/same behavior.

Additionally, we likely want to suggest

This should probably be:
"attribute %0 is deprecated
Then a note of: <note_function_suggestion > "did you mean %1?"

Additionally, See if you can find if there is a good note that we can use to suggest

User's aren't particularly knowledgeable about the fact that the same attribute can be spelled in 2 different ways. They tend to treat them as different attributes with similar/same behavior.

Additionally, we likely want to suggest

This should probably be:
"attribute %0 is deprecated
Then a note of: <note_function_suggestion > "did you mean %1?"

Additionally, See if you can find if there is a good note that we can use to suggest

Thanks @erichkeane for the suggestion. I will look at this and update the diagnostic.

Signed-off-by: Soumi Manna <[email protected]>

erichkeane · 2020-07-21T14:36:02Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -10969,6 +10969,9 @@ def err_ivdep_declrefexpr_arg : Error<
 def warn_ivdep_redundant : Warning <"ignoring redundant Intel FPGA loop "
  "attribute 'ivdep': safelen %select{INF|%1}0 >= safelen %select{INF|%3}2">,
  InGroup<IgnoredAttributes>;
+def warn_attribute_spelling_deprecated : Warning<
+  "attribute %0 is deprecated, did you mean to use 'reqd_sub_group_size' instead?">,


I had suggested making this a warning + note (using something like note_function_suggestion) as it seems to fit how diagnostics work better, and still have a preference for it. But @elizabethandrews @Fznamznon and @premanandrao can make the final call here.

I personally prefer adding a note. However I see instances of 'did you mean' in error/warning message itself in DiagnosticsSemaKinds.td. @erichkeane what does clang community recommend? If they recommend using a separate note, @smanna12 would you mind adding a note? There are examples in DiagnosticsSemaKinds.td

I suspect we are consistently inconsistent :) Clang typically doesn't have its warnings be prescriptive, but I can see that we do in a bunch of cases. I'm not sure we have official guidance.

That said, if we leave it alone, i think the suggested replacement needs to be a replacement as well. Finally, don't we mean:

'did you mean to use 'intel::reqd_sub_group_size' instead?"

Thanks @elizabethandrews and @erichkeane for the comments.

However I see instances of 'did you mean' in error/warning message itself in DiagnosticsSemaKinds.td.
Yes, i saw several examples that used and so added in the warning message.

I have added warning+ note and updated all tests with it.

Signed-off-by: Soumi Manna <[email protected]>

erichkeane · 2020-07-21T17:16:55Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

  InGroup<DeprecatedAttributes>;
+def note_spelling_suggestion : Note<
+  "did you mean to use 'intel::reqd_sub_group_size' instead?">;


Make intel::reqd_sub_group_size a %0 instead, then fill it in with your Diag call.

I was actually surprised a general 'did you mean' like this one did not exist already.

@elizabethandrews : the only one I found was note_function_suggestion, which is textually identical to what I'd like, but is function-specific. I'm tempted to update the name in the upstream to make it work for anything...

Make intel::reqd_sub_group_size a %0 instead, then fill it in with your Diag call.

Done.

Signed-off-by: Soumi Manna <[email protected]>

clang/lib/Sema/SemaDeclAttr.cpp

elizabethandrews · 2020-07-21T18:34:55Z

clang/test/SemaSYCL/sycl-device-reqd-sub-group-size-template.cpp

@@ -5,7 +5,9 @@
 template <int SIZE>
 class KernelFunctor {
 public:


It feels weird to me that existing tests all throw warnings now. I am not sure what is usually done when spellings are changed though. Personally I would just prefer updating these tests to use the new spelling (except where this functionality is actually being checked). @erichkeane thoughts?

I could go either way. There aren't that many tests to just 'fix' the spelling of here, so I'd probably suggest just fixing all the spellings instead.

If this was more widespread (that is, the tests were a pain to change), or a situation where there was a difference in the implementation, I'd consider just using the -Wno flag in the tests. In this case, I think just changing the spelling in the tests is better than adding the warning/note to ALL of them.

That said, probably leave at least 1 to validate the deprecated warning :)

Yea. That's what I meant by 'except where this functionality is actually being checked' :)

@smanna12 sorry but can you make this change? I think it's just cleaner this way.

Yea. That's what I meant by 'except where this functionality is actually being checked' :)

@smanna12 sorry but can you make this change? I think it's just cleaner this way.

Sure. I will do this.

I think you still need to leave ONE example (perhaps add a new example) to validate the new warning/note.

Done

yes, right. By mistake, I changed all spellings.

Sorry for the multiple submission.

Signed-off-by: Soumi Manna <[email protected]>

elizabethandrews

LGTM

smanna12 · 2020-07-21T20:56:35Z

Thanks for the review.

clang/test/SemaSYCL/reqd-sub-group-size-device.cpp

clang/include/clang/Basic/AttrDocs.td

Signed-off-by: Soumi Manna <[email protected]>

MrSidims

@smanna12 thanks for clarification and sharing plans! The patch is LGTM with an exception of one unresolved comment from @erichkeane

Original commit: KhronosGroup/SPIRV-LLVM-Translator@5d4ebd1

smanna12 added 2 commits July 19, 2020 23:34

Fix Clang-format issues

a655790

Signed-off-by: Soumi Manna <[email protected]>

smanna12 changed the title ~~[WIP] [DO NOT REVIEW] [SYCL] Add support for [[intel::reqd_sub_group_…~~ [WIP] [SYCL] Add support for [[intel::reqd_sub_group_… Jul 20, 2020

smanna12 changed the title ~~[WIP] [SYCL] Add support for [[intel::reqd_sub_group_…~~ [SYCL] Add support for [[intel::reqd_sub_group_… Jul 20, 2020

smanna12 marked this pull request as ready for review July 20, 2020 12:29

smanna12 requested review from elizabethandrews, Fznamznon and premanandrao as code owners July 20, 2020 12:29

smanna12 requested review from AlexeySachkov and erichkeane July 20, 2020 12:32

erichkeane reviewed Jul 20, 2020

View reviewed changes

Update diagnostic message

65b3634

Signed-off-by: Soumi Manna <[email protected]>

erichkeane reviewed Jul 21, 2020

View reviewed changes

Add note

f3cdca1

Signed-off-by: Soumi Manna <[email protected]>

erichkeane reviewed Jul 21, 2020

View reviewed changes

smanna12 added 2 commits July 21, 2020 11:21

update note based on review comments

9eb186f

Signed-off-by: Soumi Manna <[email protected]>

Fix clang-format issue

804f147

Signed-off-by: Soumi Manna <[email protected]>

erichkeane reviewed Jul 21, 2020

View reviewed changes

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

elizabethandrews reviewed Jul 21, 2020

View reviewed changes

smanna12 added 3 commits July 21, 2020 12:04

Update all tests with new attribute spelling

8d2c340

Signed-off-by: Soumi Manna <[email protected]>

Update test and fix clang-format error

07aa988

Signed-off-by: Soumi Manna <[email protected]>

Fix clang-format error

59dde52

Signed-off-by: Soumi Manna <[email protected]>

smanna12 requested review from elizabethandrews and erichkeane July 21, 2020 20:03

elizabethandrews previously approved these changes Jul 21, 2020

View reviewed changes

erichkeane previously approved these changes Jul 21, 2020

View reviewed changes

MrSidims reviewed Jul 21, 2020

View reviewed changes

clang/test/SemaSYCL/reqd-sub-group-size-device.cpp Show resolved Hide resolved

MrSidims reviewed Jul 21, 2020

View reviewed changes

clang/include/clang/Basic/AttrDocs.td Outdated Show resolved Hide resolved

Correct Doc

7c4204e

Signed-off-by: Soumi Manna <[email protected]>

smanna12 dismissed stale reviews from erichkeane and elizabethandrews via 7c4204e July 22, 2020 02:45

smanna12 requested review from MrSidims and elizabethandrews July 22, 2020 03:05

MrSidims approved these changes Jul 22, 2020

View reviewed changes

premanandrao approved these changes Jul 22, 2020

View reviewed changes

bader merged commit b2da2c8 into sycl Jul 22, 2020

bader deleted the NewSpellingOfReqdSubGroupAttr branch July 22, 2020 15:28

jsji pushed a commit that referenced this pull request Aug 31, 2023

Test conversion instructions for TypeCooperativeMatrixKHR (#2137)

582f653

Original commit: KhronosGroup/SPIRV-LLVM-Translator@5d4ebd1

[SYCL] Add support for [[intel::reqd_sub_group_… #2137

[SYCL] Add support for [[intel::reqd_sub_group_… #2137

Uh oh!

Conversation

smanna12 commented Jul 20, 2020

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!

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!

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!

erichkeane Jul 21, 2020 • edited by smanna12 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!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

smanna12 commented Jul 21, 2020

Uh oh!

Uh oh!

Uh oh!

MrSidims left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erichkeane Jul 21, 2020 •

edited by smanna12

Loading