[SYCL][FPGA][NFC] Fix num_simd_work_items and reqd_work_group_size argument check #3337

smanna12 · 2021-03-10T15:43:48Z

From Bug: #3252
The order of the arguments to reqd_work_group_size in SYCL and OpenCL are reversed.
In the OpenCL syntax, reqd_work_group_size(X,Y,Z), X is the index that increments the fastest,
followed by Y, then Z. If we preserve that definition of X, Y, and Z, then in SYCL the attribute
is specified as reqd_work_group_size(Z,Y,X). This gets more complicated in the case where fewer than
three arguments are provided. In such a case it's not the "X" argument that is omitted
(by the definitions of X, Y, and Z above) it's the Z dimension that's omitted. So the two argument
version has the syntax reqd_work_group_size(Y,X). Similarly, the one argument variant has syntax
reqd_work_group_size(X).

This patches fixes the problem on #3275 where we checked the wrong argument.
In SYCL, the num_simd_work_items must evenly divide the last argument to reqd_work_group_size regardless of how many arguments req_work_group_size has.

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

…gument check The order of the arguments to reqd_work_group_size in SYCL and OpenCL are reversed. In the OpenCL syntax, reqd_work_group_size(X,Y,Z), X is the index that increments the fastest, followed by Y, then Z. If we preserve that definition of X, Y, and Z, then in SYCL the attribute is specified as reqd_work_group_size(Z,Y,X). This gets more complicated in the case where fewer than three arguments are provided. In such a case it's not the "X" argument that is omitted (by the definitions of X, Y, and Z above) it's the Z dimension that's omitted. So the two argument version has the syntax reqd_work_group_size(Y,X). Similarly, the one argument variant has syntax reqd_work_group_size(X). This patches fixes the problem on intel#3275 where we checked the wrong argument. In SYCL, the num_simd_work_items must evenly divide the last argument to reqd_work_group_size regardless of how many arguments req_work_group_size has. Signed-off-by: Soumi Manna <[email protected]>

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

AaronBallman · 2021-03-11T13:06:41Z

clang/lib/Sema/SemaDeclAttr.cpp

@@ -3151,11 +3151,14 @@ static void handleWorkGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) {
      return;
    ZDimExpr = ZDim.get();

+    // If the declaration has an [[intel::num_simd_work_items()]] attribute,
+    // check to see if the last argument can be evenly divided by the
+    // num_simd_work_items attribute


Suggested change

// num_simd_work_items attribute

// num_simd_work_items attribute.

AaronBallman · 2021-03-11T13:14:00Z

clang/lib/Sema/SemaDeclAttr.cpp

    if (const auto *A = D->getAttr<SYCLIntelNumSimdWorkItemsAttr>()) {
      int64_t NumSimdWorkItems =
          A->getValue()->getIntegerConstantExpr(Ctx)->getSExtValue();

-      if (XDimVal.getZExtValue() % NumSimdWorkItems != 0) {
+      if (ZDimVal.getZExtValue() % NumSimdWorkItems != 0) {


This check is called for __attribute__((reqd_work_group_size)) as well as [[intel::reqd_work_group_size]] -- aren't X and Z only swapped for the intel attribute and not the OpenCL ones?

@AaronBallman,
I have added a check to verify SYCL vs OpenCL spelling. I did not add any test for __attribute__((reqd_work_group_size)) since we can not use [[intel::num_simd_work_items]] and _attribute__((reqd_work_group_size)) together here

clang/lib/Sema/SemaDeclAttr.cpp

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

premanandrao · 2021-03-12T17:13:49Z

clang/test/SemaSYCL/num_simd_work_items_device.cpp

 // can be evenly divided by the num_simd_work_items attribute.
 struct TRIFuncObjBad1 {
  [[intel::num_simd_work_items(3)]]        // expected-error{{'num_simd_work_items' attribute must evenly divide the work-group size for the 'reqd_work_group_size' attribute}}
-  [[intel::reqd_work_group_size(5, 3, 3)]] // expected-note{{conflicting attribute is here}}
+  [[intel::reqd_work_group_size(5, 3, 5)]] // expected-note{{conflicting attribute is here}}


Could you please change the examples to something like (3, 3, 5) so we know which value the complaint is about? With (5, 3, 5) it is ambiguous as to which '5' the '3' does not divide. This comment applies to many of the cases in this test.

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

premanandrao

I am okay with the changes. @elizabethandrews ?

AaronBallman · 2021-03-12T19:19:50Z

clang/include/clang/Basic/AttrDocs.td

@@ -2421,7 +2421,7 @@ device kernel, the attribute is not ignored and it is propagated to the kernel.
 If the`` intel::reqd_work_group_size`` or ``cl::reqd_work_group_size``
 attribute is specified on a declaration along with a
 intel::num_simd_work_items attribute, the work group size attribute
-argument (the first argument) must be evenly divisible by the argument specified
+argument (the last argument) must be evenly divisible by the argument specified


This documentation also needs to be updated for the OpenCL vs SYCL confusion. This would be a great place for a big warning to users because they're definitely going to be surprised by this SYCL design choice.

AaronBallman · 2021-03-12T19:22:51Z

clang/lib/Sema/SemaDeclAttr.cpp

    if (const auto *A = D->getAttr<SYCLIntelNumSimdWorkItemsAttr>()) {
      int64_t NumSimdWorkItems =
          A->getValue()->getIntegerConstantExpr(Ctx)->getSExtValue();

-      if (XDimVal.getZExtValue() % NumSimdWorkItems != 0) {
+      unsigned checkWorkGroupSize =
+          AL.getSyntax() == AttributeCommonInfo::AS_GNU


We should document clearly that the GNU spelling of this attribute maps to the OpenCL semantics (because otherwise it's extremely unclear as to which semantics you get since there's no vendor information).

Also, this code misses the [[cl::reqd_work_group_size]] spelling. We should be sure to add tests for both the GNU and CXX spellings so we don't accidentally regress this sort of thing in the future.

We should be sure to add tests for both the GNU and CXX spellings so we don't accidentally regress this sort of thing in the future.

@AaronBallman , just want to check with you that how can we add tests for GNU spelling here? Am i missing anything?

we can not use [[intel::num_simd_work_items]] and __attribute__((reqd_work_group_size)) together here to verify workgroup size (first argument) divides by num_simd_work_items.

We have __attribute__((reqd_work_group_size)) and __attribute__((num_simd_work_items)) in downstream as OpenCL spelling

I'm sorry, I'm not certain I understand the questions. You're already testing for the GNU spelling? Did you mean the [[cl:reqd_work_group_size]] spelling? You can do something like (AL.getSyntax() == AttributeCommonInfo::AS_CXX11 || AL.getSyntax() == AttributeCommonInfo::AS_C2x) && AL.hasScope() && AL.getScopeName()->isStr("cl") to get you there.

Sorry i meant to say __attribute__((reqd_work_group_size)) - another GNU spelling.
[[cl:reqd_work_group_size]] - this is not the problem

I think the tests we need are:

// Merging cases. [[intel::num_simd_work_items(2)]] void func1(); __attribute__((reqd_work_group_size(3,4,4))) void func1(); // Should diagnose [[intel::num_simd_work_items(2)]] void func2(); [[cl::reqd_work_group_size(3, 4, 4)]] void func2(); // Should diagnose [[intel::num_simd_work_items(2)]] void func3(); [[intel::reqd_work_group_size(3, 4, 4)]] void func3(); // Shockingly OK! // Direct cases. [[intel::num_simd_work_items(2)]] __attribute__((reqd_work_group_size(3,4,4))) void func1(); // Should diagnose [[intel::num_simd_work_items(2)]] [[cl::reqd_work_group_size(3, 4, 4)]] void func2(); // Should diagnose [[intel::num_simd_work_items(2)]] [[intel::reqd_work_group_size(3, 4, 4)]] void func3(); // Shockingly OK! // It'd probably be good to add template cases along these same lines as well.

Or am I misunderstanding still? (Sorry, it's been a long week and I'm feeling a bit braindead.)

I confused myself with the syntax of reqd_work_group_size attributes
kernel __attribute__((reqd_work_group_size(1,1,2))) void kernel13(){}

am I misunderstanding still?

No, you understood correctly. You answered my questions. Thanks @AaronBallman

We should be sure to add tests for both the GNU and CXX spellings so we don't accidentally regress this sort of thing in the future.

Added test cases, Done.

AaronBallman · 2021-03-12T19:23:13Z

clang/lib/Sema/SemaDeclAttr.cpp

    if (const auto *A = D->getAttr<SYCLIntelNumSimdWorkItemsAttr>()) {
      int64_t NumSimdWorkItems =
          A->getValue()->getIntegerConstantExpr(Ctx)->getSExtValue();

-      if (XDimVal.getZExtValue() % NumSimdWorkItems != 0) {
+      unsigned checkWorkGroupSize =


Suggested change

unsigned checkWorkGroupSize =

unsigned CheckWorkGroupSize =

per usual coding conventions.

AaronBallman · 2021-03-12T19:27:17Z

clang/lib/Sema/SemaDeclAttr.cpp

+      Optional<llvm::APSInt> ZDimVal = DeclAttr->getZDimVal(Context);
+
+      llvm::APSInt WorkGroupSize =
+          DeclAttr->getSyntax() == AttributeCommonInfo::AS_GNU ? *XDimVal


This would be better done by adding an Accessor to the attribute in Attr.td. e.g.,

let Accessors = [Accessor<"usesOpenCLArgOrdering", [GNU<"reqd_work_group_size">, CXX11<"cl","reqd_work_group_size">]>];

then you can do DeclAttr->usesOpenCLArgOrdering() here. That will also fix the issue with the C++ OpenCL spelling.

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

AaronBallman · 2021-03-15T12:05:21Z

clang/include/clang/Basic/AttrDocs.td

@@ -2418,26 +2418,44 @@ device kernel, the attribute is not ignored and it is propagated to the kernel.
    [[intel::num_simd_work_items(N)]] void operator()() const {}
  };

-If the`` intel::reqd_work_group_size`` or ``cl::reqd_work_group_size``


I think it would be more clear if we rearranged the changes to the docs. My recommendation is:

The docs for reqd_work_group_size is where most of these details should live. Those docs should be updated to explain what the three arguments are and that the argument which specifies the "work group size" differs depending on which spelling of the attribute is used. If possible, we should put an anchor in the docs for the work group size so that later links can direct users to that specific anchor.

The documentation that refers to reqd_work_group_size (like here) can then say "If the reqd_work_group_size attribute is specified on a declaration along with a intel::num_simd_work_items attribute, the requested work group size argument must be evenly divisible by the argument specified in the num_simd_work_items attribute.

This way, we keep all the "we made bad decisions about the argument ordering" words are all in one place (instead of repeated) and can refer to the oddity from whatever other attributes have this sort of interaction with the work group size.

AaronBallman · 2021-03-15T12:16:29Z

clang/lib/Sema/SemaDeclAttr.cpp


-      if (*XDimVal % ArgVal != 0) {
+      llvm::APSInt WorkGroupSize =


We might have a testing hole here with a bug hiding in it -- won't this wind up with ZDimVal being llvm::None because the attribute has not yet been instantiated?

template <int X, int Y, int Z> [[intel::reqd_work_group_size(X, Y, Z)]] void func(); template <int X, int Y, int Z> [[intel::num_simd_work_items(3)]] void func(); // When we go to process this attribute.

AaronBallman · 2021-03-15T12:26:18Z

clang/lib/Sema/SemaDeclAttr.cpp

+    const auto *DeclXExpr = dyn_cast<ConstantExpr>(DeclAttr->getXDim());
+    const auto *DeclZExpr = dyn_cast<ConstantExpr>(DeclAttr->getZDim());
+
+    llvm::APSInt WorkGroupSize = DeclAttr->usesOpenCLArgOrdering()


Similar here -- I think this could crash because the work group size or the merge expression hasn't been evaluated yet (so we'd dereference a null pointer). I think on something like this:

template <int X, int Y, int Z, int N> [[intel::reqd_work_group_size(X, Y, Z)]] void func(); template <int X, int Y, int Z, int N> [[intel::num_simd_work_items(N)]] void func();

I tried couple of examples below, no crash happens with current patch:

template <int X, int Y, int Z, int N> [[intel::reqd_work_group_size(X, Y, Z)]] void func(); template <int X, int Y, int Z, int N> [[intel::num_simd_work_items(N)]] void func(); int main() { func<3, 3, 5, 3>(); } This does not crash. test.cpp:5:3: error: 'num_simd_work_items' attribute must evenly divide the work-group size for the 'reqd_work_group_size' attribute [[intel::num_simd_work_items(N)]] void func(); ^ test.cpp:8:3: note: in instantiation of function template specialization 'func<3, 3, 5, 3>' requested here func<3, 3, 5, 3>(); ^ test.cpp:2:3: note: conflicting attribute is here [[intel::reqd_work_group_size(X, Y, Z)]] void func(); ^ 1 error generated. Same for other test case. No crash happens with current patch: template <int X, int Y, int Z> [[intel::reqd_work_group_size(X, Y, Z)]] void func(); template <int X, int Y, int Z> [[intel::num_simd_work_items(3)]] void func(); // When we go to process this attribute. int main() { func<3, 3, 5>(); } test1.cpp:6:3: error: 'num_simd_work_items' attribute must evenly divide the work-group size for the 'reqd_work_group_size' attribute [[intel::num_simd_work_items(3)]] void func(); // When we go to process this attribute. ^ test1.cpp:9:3: note: in instantiation of function template specialization 'func<3, 3, 5>' requested here func<3, 3, 5>(); ^ test1.cpp:2:3: note: conflicting attribute is here [[intel::reqd_work_group_size(X, Y, Z)]] ^ 1 error generated.

Am i missing something here? Did you mean to test something different examples here?

This would crash. I think you meant to test this example. The crash happens here because the work group size or the merge expression hasn't been evaluated yet for existing implementation of reqd_work_group_size attribute( plan to refactor this attribute later - outside of this PR)

template <int X, int Y, int Z>
[[intel::num_simd_work_items(3)]] void func();

template <int X, int Y, int Z>
[[intel::reqd_work_group_size(X, Y, Z)]]
void func();

int main() {
func<3, 3, 5>();
}

It's possible my test cases weren't quite right. The cases I'm worried about are when either the arg expression from num_simd_work_items is null (because it's not a constant expression we've evaluated yet) or the work group size argument from reqd_work_group_size is null (for the same reason).

(Note how we're checking for a null MergeExpr a few lines higher in this same function.)

keryell · 2021-03-16T05:35:15Z

clang/include/clang/Basic/AttrDocs.td

+
+  struct func1 {
+    [[intel::num_simd_work_items(4)]]
+    [[cl::reqd_work_group_size(4, 5, 5)]]


When possible, do not pick 2 numbers the same.
I know in the SYCL spec there are a lot of such bad examples... :-(
What about 3 different prime numbers?

smanna12 changed the title ~~[SYCL][FPGA][NFC] Fix num_simd_work_items and reqd_work_group_size ar…~~ [SYCL][FPGA][NFC] Fix num_simd_work_items and reqd_work_group_size argument check Mar 10, 2021

smanna12 requested a review from AaronBallman March 10, 2021 15:47

smanna12 marked this pull request as ready for review March 10, 2021 15:47

smanna12 requested review from elizabethandrews and premanandrao as code owners March 10, 2021 15:47

fix test

00deaee

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

AaronBallman reviewed Mar 11, 2021

View reviewed changes

smanna12 added 2 commits March 12, 2021 06:24

Address review comments

e5d1479

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

Fix errors

602ef39

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

smanna12 requested a review from AaronBallman March 12, 2021 14:43

premanandrao reviewed Mar 12, 2021

View reviewed changes

address review comments

bf958c6

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

smanna12 requested a review from premanandrao March 12, 2021 18:11

premanandrao reviewed Mar 12, 2021

View reviewed changes

AaronBallman reviewed Mar 12, 2021

View reviewed changes

smanna12 added 3 commits March 13, 2021 17:47

Address review comments

ca86bb5

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

Fix errors

5dd9786

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

update var name

895e3bc

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

smanna12 requested review from AaronBallman and premanandrao March 14, 2021 02:25

Merge remote-tracking branch 'my_remote/sycl' into FixNumSimdReqdAttrBug

579ad6f

AaronBallman reviewed Mar 15, 2021

View reviewed changes

smanna12 mentioned this pull request Mar 15, 2021

reqd_work_group_size and max_work_group_size argument checking #3361

Closed

keryell reviewed Mar 16, 2021

View reviewed changes

smanna12 closed this May 12, 2021

	// num_simd_work_items attribute
	// num_simd_work_items attribute.

[SYCL][FPGA][NFC] Fix num_simd_work_items and reqd_work_group_size argument check #3337

[SYCL][FPGA][NFC] Fix num_simd_work_items and reqd_work_group_size argument check #3337

Uh oh!

Conversation

smanna12 commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smanna12 Mar 12, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

premanandrao left a comment

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!

smanna12 Mar 12, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

smanna12 Mar 12, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

smanna12 Mar 14, 2021 • 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!

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!

smanna12 Mar 15, 2021 • 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!

smanna12 Mar 15, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smanna12 commented Mar 10, 2021 •

edited

Loading

smanna12 Mar 12, 2021 •

edited

Loading

smanna12 Mar 12, 2021 •

edited

Loading

smanna12 Mar 12, 2021 •

edited

Loading

smanna12 Mar 14, 2021 •

edited

Loading

smanna12 Mar 15, 2021 •

edited

Loading

smanna12 Mar 15, 2021 •

edited

Loading