[SYCL] reqd_work_group_size attribute is reversed by fadeeval · Pull Request #1234 · intel/llvm

fadeeval · 2020-03-03T11:58:28Z

The reqd_work_group_size attribute should follow the SYCL/C++ convention, where the rightmost index maps to the linear dimension.

Signed-off-by: Aleksander Fadeev aleksander.fadeev@intel.com

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

erichkeane

I can't make the value judgement required here, but the code changes seem innocuous enough. There are a couple of unnecessary formatting changes (see handleWorkGroupSize:2942), but I'll leave it up to @bader to see if its worth reverting that in this patch.

fadeeval · 2020-03-03T14:41:07Z

I can't make the value judgement required here, but the code changes seem innocuous enough. There are a couple of unnecessary formatting changes (see handleWorkGroupSize:2942), but I'll leave it up to @bader to see if its worth reverting that in this patch.

I make automatic formatting and this is that I got.

asavonic · 2020-03-03T14:41:44Z

clang/lib/Sema/SemaDeclAttr.cpp

-                    Existing->getYDim() == WGSize[1] &&
-                    Existing->getZDim() == WGSize[2]))
+  if (Existing &&
+      !(Existing->getXDim() == WGSize[0] && Existing->getYDim() == WGSize[1] &&


This check is not reversed. Does the warning below still work correctly?

I think it can work. Is there test for it?

If yes, then it works fine.

I think it can work. Is there test for it?

That is what I was going to ask.

If I'm reading this code correctly, we compare XDim of an existing attribute with WGSize[0], which is going to be ZDim. Shouldn't we compare existing XDim with WGSize[2] instead?

Maybe you are right

I will try to prove it.

Yes, it need to be reversed.

I suspect that the diagnostic was not tested. Can you add a test for it?

A general case was tested, see: reqd-work-group-size.cpp:23 - 31, and these diagnostics are firing anyway.
But with this patch there is a corner case introduced, that was untested, like:

[[cl::reqd_work_group_size(4, 4, 1)]] [[cl::reqd_work_group_size(1, 4, 4)]] void foo() { /*...*/ }

I add one test, that prove rightness of the implementation.

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

clang/test/SemaSYCL/reqd-work-group-size.cpp

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

fadeeval · 2020-03-04T15:32:38Z

clang/test/SemaSYCL/reqd-work-group-size.cpp

 class Functor16 {
 public:
-  [[cl::reqd_work_group_size(16, 1, 1)]] void operator()() {}
+  [[cl::reqd_work_group_size(16, 1, 1)]] [[cl::reqd_work_group_size(16, 1, 1)]] void operator()() {}


This is a new one.

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

asavonic · 2020-03-06T10:19:50Z

clang/lib/Sema/SemaDeclAttr.cpp


  if (const auto *A = D->getAttr<SYCLIntelMaxWorkGroupSizeAttr>()) {
-    if (!(WGSize[0] <= A->getXDim() && WGSize[1] <= A->getYDim() &&
+    if (S.getLangOpts().SYCLIsDevice &&


There is a lot of code duplication around S.getLangOpts().SYCLIsDevice.
Consider the following improvement:

int XDim = WGSize[0]; int YDim = WGSize[1]; int ZDim = WGSize[2]; if (S.getLangOpts().SYCLIsDevice) { std::swap(XDim, ZDim); // add a good comment here }

and then use *Dim variables instead of WGSize.

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

asavonic · 2020-03-06T15:45:02Z

clang/include/clang/Basic/Attr.td

 def SYCLIntelNumSimdWorkItems : InheritableAttr {
  let Spellings = [CXX11<"intelfpga","num_simd_work_items">];
-  let Args = [UnsignedArgument<"Number">];
+  let Args = [IntArgument<"Number">];


I guess this should be removed.

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

…e_api_test * origin/sycl: [SYCL][NFC] Fix static code analysis concerns (intel#1283) [SYCL] Fix the test/basic_tests/buffer/subbuffer.cpp (intel#1277) [SYCL][CUDA] Implement the program kernel names query (intel#1248) [SYCL] Honor the LLVM_LIBDIR_SUFFIX variable at installation time (intel#1261) [SYCL][UX] Diagnostic for undefined device functions (intel#1026) [SYCL] Reverse reqd_work_group_size attribute (intel#1234) [SYCL] Rename project to oneAPI DPC++ Compiler (intel#1249) [SYCL][XPTI] Instrumentation of SYCL runtime with XPTI (intel#1129)

…st_commit * otcshare/sycl: (469 commits) [SYCL] Implement thread-local storage restriction (intel#1281) [Driver][SYCL][FPGA] Adjust the output location for the project report (intel#1278) [SYCL][NFC] Fix static code analysis concerns (intel#1283) [SYCL] Fix the test/basic_tests/buffer/subbuffer.cpp (intel#1277) [SYCL][CUDA] Implement the program kernel names query (intel#1248) [SYCL] Honor the LLVM_LIBDIR_SUFFIX variable at installation time (intel#1261) [SYCL][UX] Diagnostic for undefined device functions (intel#1026) [SYCL] Reverse reqd_work_group_size attribute (intel#1234) [SYCL] Rename project to oneAPI DPC++ Compiler (intel#1249) [SYCL][XPTI] Instrumentation of SYCL runtime with XPTI (intel#1129) [SYCL] Add buffer dimensions restriction (intel#1147) [SYCL][NFC] Update copyright header in handler files (intel#1271) [SYCL][NFC] Format the code with clang-format [SYCL][Test] Fix SYCL library location path for LIT tests (intel#1228) [SYCL][NFC] Fix doxygen warnings (intel#1270) [SYCL][CUDA] Add the CUDA backend to the deploy-sycl-toolchain target (intel#1268) [SYCL][NFC] Fix a misleading comment regarding the SYCL flow (intel#1266) Change capability for SpecId decoration README.md: Mention retrieving llvm archive signatures travis: Restore macOS builds ...

…_accessor_refactor * origin/sycl: (454 commits) [SYCL][NFC] Fix static code analysis concerns (intel#1283) [SYCL] Fix the test/basic_tests/buffer/subbuffer.cpp (intel#1277) [SYCL][CUDA] Implement the program kernel names query (intel#1248) [SYCL] Honor the LLVM_LIBDIR_SUFFIX variable at installation time (intel#1261) [SYCL][UX] Diagnostic for undefined device functions (intel#1026) [SYCL] Reverse reqd_work_group_size attribute (intel#1234) [SYCL] Rename project to oneAPI DPC++ Compiler (intel#1249) [SYCL][XPTI] Instrumentation of SYCL runtime with XPTI (intel#1129) [SYCL] Add buffer dimensions restriction (intel#1147) [SYCL][NFC] Update copyright header in handler files (intel#1271) [SYCL][NFC] Format the code with clang-format [SYCL][Test] Fix SYCL library location path for LIT tests (intel#1228) [SYCL][NFC] Fix doxygen warnings (intel#1270) [SYCL][CUDA] Add the CUDA backend to the deploy-sycl-toolchain target (intel#1268) Change capability for SpecId decoration README.md: Mention retrieving llvm archive signatures travis: Restore macOS builds Fix DebugInfo creation after LLVM change 7a42bab Add more missing mixes Add missing fixes ...

fadeeval added 8 commits February 28, 2020 15:07

[SYCL] reqd_work_group_size attribute is reversed

2fc939b

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

[SYCL] reqd_work_group_size attribute is reversed (fix intel#2)

3582e97

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

[SYCL] reqd_work_group_size attribute is reversed (fixed intel#3)

3c6ea83

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

[SYCL] reqd_work_group_size attribute is reversed (fix intel#4)

b14013d

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

[SYCL] reqd_work_group_size attribute is reversed (fix intel#5)

1d0d751

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

[SYCL] reqd_work_group_size attribute is reversed (fix intel#6)

ff6746a

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

[SYCL] reqd_work_group_size attribute is reversed (fix intel#6)

245f323

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

[SYCL] reqd_work_group_size attribute is reversed (fix intel#8)

9125bc1

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

fadeeval requested review from MrSidims, asavonic, bader and erichkeane March 3, 2020 12:16

romanovvlad assigned asavonic Mar 3, 2020

erichkeane reviewed Mar 3, 2020

View reviewed changes

asavonic reviewed Mar 3, 2020

View reviewed changes

[SYCL] reqd_work_group_size attribute is reversed (fix intel#9)

a7eef4b

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

fadeeval commented Mar 4, 2020

View reviewed changes

clang/test/SemaSYCL/reqd-work-group-size.cpp Outdated Show resolved Hide resolved

[SYCL] reqd_work_group_size attribute is reversed (fix intel#10)

2b6df01

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

fadeeval commented Mar 4, 2020

View reviewed changes

fadeeval added 3 commits March 4, 2020 18:37

[SYCL] reqd_work_group_size attribute is reversed (fix intel#11)

5c3501f

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

[SYCL] reqd_work_group_size attribute is reversed (fix intel#12)

43f265f

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

[SYCL] reqd_work_group_size attribute is reversed (fix intel#13)

5831eb4

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

asavonic reviewed Mar 6, 2020

View reviewed changes

fadeeval added 2 commits March 6, 2020 14:36

[SYCL] reqd_work_group_size attribute is reversed (fix intel#13)

8696f6e

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

[SYCL] reqd_work_group_size attribute is reversed (fix intel#14)

dc704bf

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

fadeeval force-pushed the private/fadeeval/reqd_work_group_size branch from 5831eb4 to dc704bf Compare March 6, 2020 14:27

[SYCL] reqd_work_group_size attribute is reversed

9ab8cf7

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

asavonic reviewed Mar 6, 2020

View reviewed changes

[SYCL] reqd_work_group_size attribute is reversed (fix intel#15)

abe6d2d

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>

asavonic approved these changes Mar 10, 2020

View reviewed changes

romanovvlad merged commit 1da6fbe into intel:sycl Mar 10, 2020

fadeeval deleted the private/fadeeval/reqd_work_group_size branch March 13, 2020 12:30

fadeeval restored the private/fadeeval/reqd_work_group_size branch March 13, 2020 12:30

fadeeval deleted the private/fadeeval/reqd_work_group_size branch March 13, 2020 12:31

smanna12 mentioned this pull request Dec 21, 2020

[SYCL] [FPGA] Add template parameter support for work_group_size attributes #2906

Merged

Conversation

fadeeval commented Mar 3, 2020

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

fadeeval commented Mar 3, 2020 • 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!

fadeeval Mar 3, 2020 • 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!

MrSidims Mar 4, 2020 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fadeeval commented Mar 3, 2020 •

edited

Loading

fadeeval Mar 3, 2020 •

edited

Loading

MrSidims Mar 4, 2020 •

edited

Loading