[ESIMD] Re-work loads from globals in sycl-post-link #4718

v-klochkov · 2021-10-06T21:24:31Z

The re-work in the lowering of loads from globals was required
because the previous implementation did not allow handling
the loads from scalar globals.
Added lowering for __spirv_BuiltInSubgroupLocalInvocationId(),
which must always return 0 for ESIMD.
Added lowering for __spirv_BuiltInSubgroupSize() and __spirv_BuiltInSubgroupMaxSize()
which must always return 1 for ESIMD.

The corresponding LIT test: intel/llvm-test-suite#503

Signed-off-by: Vyacheslav N Klochkov vyacheslav.n.klochkov@intel.com

1) The re-work in the lowering of loads from globals was required because the previous implementation did not allow handling the loads from scalar globals. 2) The previous implementation generated duplicated vector loads for each of users. Fixed it here. 3) Added lowering for __spirv_BuiltInSubgroupLocalInvocationId(), which must always return 0 for ESIMD. Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

v-klochkov · 2021-10-06T21:31:58Z

It looks like I have broken something in this patch at the very last minute before uploading to re-view. I'll fix it shortly.

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

kbobrovs · 2021-10-06T22:50:21Z

Per our discussion, the new approach when {{@llvm.genx.XXX}} is generated once in the place where a load from __spirv_XXX was, rather than at every __spirv_XXX use (as it was before) looks a bit questionable. Adding @kychendev.

…f GenX calls This commit also implements loads from SubgroupSize and SubgroupMaxSize SPIRV globals. Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

…postlink

kbobrovs · 2021-10-07T23:20:43Z

llvm/lib/SYCLLowerIR/LowerESIMD.cpp

+  assert(isa<ConstantInt>(IndexV) &&
+         "Expected a const index in extract element instruction");
+  uint64_t IndexValue = cast<ConstantInt>(IndexV)->getZExtValue();
+  assert(IndexValue <= 2 &&


I suggest to rename 3 to MAX_DIMS, 2 - to MAX_DIMS-1 in such context. It is not always clear why 3 or 2 is used

Sure, will change.

kbobrovs · 2021-10-07T23:24:52Z

llvm/lib/SYCLLowerIR/LowerESIMD.cpp

+  // uint32_t __spirv_BuiltIn SubgroupId;
+
+  // Translate the loads from _scalar_ SPIRV globals in the next block.
+  // Such globals require the replacement of the load only because the users


I think the main distinguishing factor from vector intrinsics is that these scalar intrinsics always generate a constant, that's why there is no need to materialize genx intrinsic calls at every use. Otherwise there are no problems with such materialization.
I suggest to adjust the comment.

Ok, changed. Will upload additional commit soon.

kbobrovs · 2021-10-07T23:26:10Z

llvm/lib/SYCLLowerIR/LowerESIMD.cpp

-         "Extract element index should be a constant");
+  // Only loads from _vector_ SPIRV globals reach here. Replace their users now.
+  for (User *LU : LI->users()) {
+    ExtractElementInst *EEI = dyn_cast<ExtractElementInst>(LU);


simple cast can be used. It is also better, because it will crash if LU is not ExtractElementInst even in release builds.

What you mean by 'simple' cast? The cast needs to be dyn_cast as it works everywhere in llvm when instruction types are casted.
Regarding 'assert', it was copied from the previous sources, I did not change it. Do you recommend replacing it with 'llvm::report_fatal_error' ?

you use EEI = dyn_cast followed by assert(EEI), I suggest replace these two with EEI = cast<ExtractElementInst>

Ah, I forgot that cast<> included the check/assert. Thank you. Fixed in d5dbf7b

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

…postlink

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

kbobrovs · 2021-10-09T06:30:46Z

The single test failure in Jenkins is unrelated to the changes. Merging. @bader, @vladimirlaz - FYI

[2021-10-09T03:22:44.035Z] $ "env" "SYCL_DEVICE_FILTER=opencl:gpu,host" "W:\jenkins-dir\workspace\SYCL_CI\intel\Win\Test_Suite\build\SYCL\Basic\image\Output\image_max_size.cpp.tmp.out"
[2021-10-09T03:22:44.035Z] # command output:
[2021-10-09T03:22:44.035Z] Running on Intel(R) UHD Graphics 630, Driver: 30.0.100.9864
[2021-10-09T03:22:44.035Z] 2d: Max image width: 16384
[2021-10-09T03:22:44.035Z] 2d: Max image height: 16384
[2021-10-09T03:22:44.035Z] 3d: Max image width: 16384
[2021-10-09T03:22:44.035Z] 3d: Max image height: 16384
[2021-10-09T03:22:44.035Z] 3d: Max image depth: 2048
[2021-10-09T03:22:44.035Z] Starting the test with size = {16384, 2} ... Passed
[2021-10-09T03:22:44.035Z] Starting the test with size = {2, 16384} ... Passed
[2021-10-09T03:22:44.035Z] Starting the test with size = {16384, 2, 3} ... Passed
[2021-10-09T03:22:44.035Z]
[2021-10-09T03:22:44.035Z] error: command failed with exit status: 3221226505

bader · 2021-10-09T10:40:20Z

@bader, @vladimirlaz - FYI

Please, fix the issue to pass pre-commit.
Another option is to report the failure via bug tracker and disable the test while the issue is being investigated to avoid false alarms in CI.
@pvchupin, FYI.

I also noticed that this change breaks the build with disabled assertions - https://github.com/intel/llvm/runs/3845316243.
@v-klochkov, please, fix ASAP.

v-klochkov · 2021-10-09T17:15:28Z

I also noticed that this change breaks the build with disabled assertions - https://github.com/intel/llvm/runs/3845316243. @v-klochkov, please, fix ASAP.

I uploaded the fix: #4733

kbobrovs · 2021-10-09T18:59:54Z

Please, fix the issue to pass pre-commit.
Another option is to report the failure via bug tracker and disable the test while the issue is being investigated to avoid false
alarms in CI.

I'm pretty sure this issue wasn't caused by this patch, so I think the owner of the code should investigate. So option 2 is more reasonable.

v-klochkov requested review from kbobrovs and sndmitriev October 6, 2021 21:24

v-klochkov requested a review from kychendev as a code owner October 6, 2021 21:24

Minor fix to fix an error showing up only Release build mode

b04643a

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

v-klochkov added 3 commits October 7, 2021 10:53

Partially revert the initial commit in this PR: keep the duplicates o…

7d7ea96

…f GenX calls This commit also implements loads from SubgroupSize and SubgroupMaxSize SPIRV globals. Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

[NFC] - revert the comment change in LIT test

eae34f7

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

Merge remote-tracking branch 'intel_llvm/sycl' into esimd_globals_in_…

c8f4173

…postlink

kbobrovs reviewed Oct 7, 2021

View reviewed changes

v-klochkov added 3 commits October 8, 2021 18:56

Minor fixes to address reviewer's comments

d5dbf7b

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

Merge remote-tracking branch 'intel_llvm/sycl' into esimd_globals_in_…

55dc767

…postlink

clang-format fixes

4e9dcaa

Signed-off-by: Vyacheslav N Klochkov <vyacheslav.n.klochkov@intel.com>

kbobrovs approved these changes Oct 9, 2021

View reviewed changes

kbobrovs merged commit f470ec7 into intel:sycl Oct 9, 2021

v-klochkov deleted the esimd_globals_in_postlink branch October 9, 2021 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ESIMD] Re-work loads from globals in sycl-post-link #4718

[ESIMD] Re-work loads from globals in sycl-post-link #4718

Uh oh!

v-klochkov commented Oct 6, 2021 •

edited

Loading

Uh oh!

v-klochkov commented Oct 6, 2021

Uh oh!

kbobrovs commented Oct 6, 2021

Uh oh!

kbobrovs Oct 7, 2021

Uh oh!

v-klochkov Oct 8, 2021

Uh oh!

kbobrovs Oct 7, 2021

Uh oh!

v-klochkov Oct 8, 2021

Uh oh!

kbobrovs Oct 7, 2021

Uh oh!

v-klochkov Oct 8, 2021 •

edited

Loading

Uh oh!

kbobrovs Oct 8, 2021

Uh oh!

v-klochkov Oct 9, 2021

Uh oh!

kbobrovs commented Oct 9, 2021

Uh oh!

bader commented Oct 9, 2021

Uh oh!

v-klochkov commented Oct 9, 2021

Uh oh!

kbobrovs commented Oct 9, 2021

Uh oh!

Uh oh!

[ESIMD] Re-work loads from globals in sycl-post-link #4718

[ESIMD] Re-work loads from globals in sycl-post-link #4718

Uh oh!

Conversation

v-klochkov commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

v-klochkov commented Oct 6, 2021

Uh oh!

kbobrovs commented Oct 6, 2021

Uh oh!

kbobrovs Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

v-klochkov Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

kbobrovs Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

v-klochkov Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

kbobrovs Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

v-klochkov Oct 8, 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!

kbobrovs Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

v-klochkov Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

kbobrovs commented Oct 9, 2021

Uh oh!

bader commented Oct 9, 2021

Uh oh!

v-klochkov commented Oct 9, 2021

Uh oh!

kbobrovs commented Oct 9, 2021

Uh oh!

Uh oh!

v-klochkov commented Oct 6, 2021 •

edited

Loading

v-klochkov Oct 8, 2021 •

edited

Loading