[SYCL][ESIMD] Setup compilation pipeline for ESIMD #2134

kbobrovs · 2020-07-17T11:01:27Z

Add passes in BackendUtil to enable ESIMD back-end code generation.
Add more tests some of which are dependent on the new compilation pipeline for ESIMD
on-device tests are to be added later once CI infrastructure is ready

Please review commits starting from
7f525d5 [SYCL][ESIMD] Enable LLVM passes for ESIMD.
Earlier commits are parts of other PRs.

bader

LLVM changes looks good to me, but I would expect LLVM LIT test updates covering them.

bader · 2020-07-17T12:35:37Z

llvm/lib/SYCLLowerIR/LowerESIMD.cpp

+      Value *TranslatedV =
+        TranslateFunc(*CI, SpirvIntrName.substr(1, 1));


Could you add a regression test for this change, please?

It is added - sycl/test/esimd/spirv_intrins_trans.cpp

I mean LLVM LIT test, not SYCL LIT test.

Oh, I guess you mean IR->IR test. OK, will add. Even though functionality is covered by sycl/test/esimd/spirv_intrins_trans.cpp.

alexbatashev

Contents of sycl/ LGTM.

sycl/test/esimd/glob.cpp

sycl/test/basic_tests/esimd/global_var.cpp

kbobrovs · 2020-07-20T13:39:32Z

@bader, added a new test case into llvm/test/SYCLLowerIR/esimd_lower_intrins.ll - see
declare dso_local spir_func i64 @_Z27__spirv_LocalInvocationId_xv()

kbobrovs · 2020-07-20T13:40:27Z

had to rebase to fix 2 failing tests and force-push.

alexbatashev

SYCL part still LGTM

bader · 2020-07-20T14:20:20Z

llvm/test/SYCLLowerIR/esimd_lower_intrins.ll

+  %call = call spir_func i64 @_Z27__spirv_LocalInvocationId_xv()
+; CHECK-NEXT: %call.esimd = call <3 x i32> @llvm.genx.local.id.v3i32()
+; CHECK-NEXT: %local_id.x = extractelement <3 x i32> %call.esimd, i32 0
+; CHECK-NEXT: %local_id.x.cast.ty = zext i32 %local_id.x to i64


Please, consider following existing design for lowering functions like SPIR-V built-ins to device specific intrinsics by linking corresponding device library with SPIR-V functions implementation (see NVPTX implementation for details).
This will allow you to remove most of LowerESIMD pass (including the buggy part).

The problem with NVPTX approach as well as with using existing clang built-in infra is that it is hard to support the "C++" intrinsics mechanism with those. Once C++ intrinsics is supported in clang built-in infra - yes, this pass will simplify.

What buggy part do you refer to?

The problem with NVPTX approach as well as with using existing clang built-in infra is that it is hard to support the "C++" intrinsics mechanism with those.

I'm not sure why this mechanism is needed. translateSpirvIntrinsic doesn't require it.

BTW, why do we need this in the first place? According to my understanding these SPIR-V built-ins are lowered to valid SPIR-V, which should be supported by the back-end.

Once C++ intrinsics is supported in clang built-in infra - yes, this pass will simplify.

What buggy part do you refer to?

The one fixed by this patch.

I see. You refer to SPIRV translation part. Agree - this could be done similar to NVPTX.

According to my understanding these SPIR-V built-ins are lowered to valid SPIR-V, which should be supported by the back-end.

Not quite. The vector BE does not support SPIRV intrinsics, as it is not a SIMT BE. But with subgroup size = 1 restriction it is doable really. I thought about this as a future step.

According to my understanding these SPIR-V built-ins are lowered to valid SPIR-V, which should be supported by the back-end.

Not quite. The vector BE does not support SPIRV intrinsics, as it is not a SIMT BE. But with subgroup size = 1 restriction it is doable really. I thought about this as a future step.

To be precise, this functions are represented in SPIR-V as a variable with decoration and I think it must be supported by any back-end. AFAIK, there are a lot of ESIMD specific patches landed to SPIR-V translator and it seems to the right place to lower this functionality.
SPIR-V consumer by design is supposed to lower standard SPIR-V instructions to HW specific intructions/intrinsics.
If I understand it correctly ESIMD extension implementation includes these two parts:

LLVM pass replaces SPIR-V built-in functions to GEN specific intrinsics in FE/ME.

SPIR-V translator is enhanced to preserve undefined GEN specific intrinsics in SPIR-V format.

According to my understanding these are not required if we move lowering of SPIR-V standard instructions to SPIR-V consumer.

NOTE: I'm talking about standard SPIR-V functionality only.

SPIR-V consumer by design is supposed to lower standard SPIR-V instructions to HW specific intructions/intrinsics.

That is true. But ESIMD back-end by design wasn't a "SPIRV consumer" and glue passes are still required to feed it LLVM IR resulting from SPIRV translation. Also ESIMD BE can't consume arbitrary SPIRV with Kernel capability - there are a number of restrictions. E.g. ESIMD BE does not have a concept of workitem, a central one in SIMT world. My point is that there are still design options which need to be discussed with the ESIMD BE devs.

But ESIMD back-end by design wasn't a "SPIRV consumer" and glue passes are still required to feed it LLVM IR resulting from SPIRV translation. Also ESIMD BE can't consume arbitrary SPIRV with Kernel capability - there are a number of restrictions. E.g. ESIMD BE does not have a concept of workitem, a central one in SIMT world. My point is that there are still design options which need to be discussed with the ESIMD BE devs.

If SPIR-V consumption is not designed, what is the point in using SPIR-V then?
It looks like using LLVM IR as exchange format in ESIMD mode would be much easier and doesn't require so much hacks.

If SPIR-V consumption is not designed, what is the point in using SPIR-V then?

IR stability.

doesn't require so much hacks.

There can be parts which can be improved, but I don't see how this justifies calling it hacks.

bader

de14866 - looks good me, assuming that other changes to llvm/* sources will be reviewed and merged separately (i.e. before this PR).

elizabethandrews

The only commit I see here with FE change not covered by other PRs is 2689ac1. This commit LGTM. Prior commits with FE changes have been reviewed, with changes requested, in their corresponding PRs.

kbobrovs · 2020-07-22T06:53:18Z

Just for the record: this patch can be merged only after those:
#2133, #2097, #2096
some conflict resolution might be required

kbobrovs · 2020-07-22T19:57:07Z

Had to resolve conflicts, rebase, fix test and force-push. The only changes are
[SQUASH] Fixed esimd_metadata2.cpp to prevent inlining. b28d14e264128b7800b215fde74f64ccea0b8718
[SQUASH] Applied clang format to ESIMD tests. 14dce981f37943719dea406a5f8ca6e5ea9c6732

@bader, please take a look at the two new above changes and re-approve.

Also new base commit covered by separate PR #2161 added.

kbobrovs · 2020-07-22T19:59:16Z

Updated list of active PRs this one depends on:
#2161 #2097

bader · 2020-07-23T10:34:13Z

clang/lib/CodeGen/BackendUtil.cpp

+  if (LangOpts.SYCLIsDevice && LangOpts.SYCLExplicitSIMD) {
+    PerModulePasses.add(createSYCLLowerESIMDPass());
+  }
+
  CreatePasses(PerModulePasses, PerFunctionPasses);

+  if (LangOpts.SYCLIsDevice && LangOpts.SYCLExplicitSIMD &&


I can't find review for this part, so I'll ask here.

Origianal code uses CreatePasses to populate PerModulePasses pass manager. It seems reasonable to move new code adding ESIMD passes to this function. Right?

Good point, will do.

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Authors: Konstantin S Bobrovsky <[email protected]> Denis Bakhvalov <[email protected]> Gang Chen <[email protected]> Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Authors: Wei Pan, Konstantin S Bobrovsky, Denis Bakhvalov, Shahab Layeghi. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Co-authored-by: Alexander Batashev <[email protected]>

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Co-authored-by: Mariya Podchishchaeva <[email protected]>

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

…e.ll. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

kbobrovs · 2020-07-29T09:50:12Z

Had to force-push after resolving conflicts, rebasing.

kbobrovs · 2020-07-29T09:59:02Z

@bader, please take another look (essential change - fixed test for the vecarg pass)

bader · 2020-07-29T13:57:28Z

@romanovvlad, could you take a look at clang-format-check. It looks like this sort of formatting suggestions should be suppressed. Right?
NOTE: users are free to set the comment prefixes for FileCheck, so I'm sure if there is a reliable regex to filter these comments. IIRC, you proposed to ignore all the comments. Can we apply this proposal?

…rogram * upstream/sycl: (609 commits) [SYCL] Fix fail in the post commit testing (intel#2210) [SYCL] Materialize shadow local variables for byval arguments before use (intel#2200) [SYCL] Support lambda functions passed to reduction (intel#2190) [SYCL][USM] Improve USM Allocator. (intel#2026) [SYCL] Disallow mutable lambdas (intel#1785) [SYCL][ESIMD] Setup compilation pipeline for ESIMD (intel#2134) [SYCL] Fix not found kernel due to empty kernel name when using set_arg(s) (intel#2181) [SYCL] Fixed check for set_arg (intel#2203) Refactor indirect access calls to minimize invocations. (intel#2185) [SYCL][NFC] Fix potential null-pointer access (intel#2197) [SYCL] Propagate attributes from transitive calls to kernel (intel#1878) [SYCL] Fix warnings from static analysis tool (intel#2193) [SYCL][NFC] Fix ac_float test for compilation with FE optimizations (intel#2184) [GitHub Actions] Uplift clang-format version to 10 (intel#2194) [SYCL][ESIMD] Pass to replace simd* parameters with native llvm vectors. (intel#2097) [SYCL][NFC] Fixed SYCL_PI_TRACE output while selecting a device. (intel#2192) [SYCL][FPGA] New spec for controlling load-store units in FPGAs (intel#2158) [SYCL][Doc] Clarify reqd_sub_group_size (intel#2103) [SYCL] Remove noreturn function attribute (intel#2165) [SYCL] Aligned set_arg behaviour with SYCL specification (intel#2159) ...

This completes the initial bulk conversion of tests using the migration script. There are more tests to be converted; however they will need manual fixups, so leave them out of this bulk conversion. Original commit: KhronosGroup/SPIRV-LLVM-Translator@2c9f60f

kbobrovs requested a review from DenisBakhvalov July 17, 2020 11:01

kbobrovs requested review from bader, elizabethandrews, Fznamznon, premanandrao and a team as code owners July 17, 2020 11:01

kbobrovs requested a review from alexbatashev July 17, 2020 11:01

kbobrovs added the esimd Explicit SIMD feature label Jul 17, 2020

bader previously approved these changes Jul 17, 2020

View reviewed changes

alexbatashev previously approved these changes Jul 17, 2020

View reviewed changes

sycl/test/esimd/glob.cpp Outdated Show resolved Hide resolved

sycl/test/basic_tests/esimd/global_var.cpp Outdated Show resolved Hide resolved

kbobrovs dismissed stale reviews from alexbatashev and bader via 6f58901 July 20, 2020 05:49

kbobrovs force-pushed the esimd1-be-util branch from 6f58901 to 99603ae Compare July 20, 2020 13:39

alexbatashev previously approved these changes Jul 20, 2020

View reviewed changes

bader reviewed Jul 20, 2020

View reviewed changes

bader previously approved these changes Jul 20, 2020

View reviewed changes

elizabethandrews previously approved these changes Jul 20, 2020

View reviewed changes

kbobrovs dismissed stale reviews from elizabethandrews, bader, and alexbatashev via 2e70056 July 22, 2020 12:52

kbobrovs force-pushed the esimd1-be-util branch 2 times, most recently from 2e70056 to 14dce98 Compare July 22, 2020 19:51

bader reviewed Jul 23, 2020

View reviewed changes

kbobrovs force-pushed the esimd1-be-util branch 2 times, most recently from ece45f5 to 2b0ee47 Compare July 23, 2020 12:07

kbobrovs and others added 15 commits July 28, 2020 21:45

[SYCL][ESIMD] Fix SPIRV intrinsics translation.

380a344

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

[SYCL][ESIMD] Add ESIMD intrinsics/IR translation tests.

59242b6

Authors: Konstantin S Bobrovsky <[email protected]> Denis Bakhvalov <[email protected]> Gang Chen <[email protected]> Signed-off-by: Konstantin S Bobrovsky <[email protected]>

[SYCL][ESIMD] Add ESIMD tests executed on host.

8b18175

Authors: Wei Pan, Konstantin S Bobrovsky, Denis Bakhvalov, Shahab Layeghi. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

[SQUASH] Applied clang-format

e076a8a

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Update sycl/test/esimd/glob.cpp

af1d73b

Co-authored-by: Alexander Batashev <[email protected]>

[SYCL][ESIMD] Fix esimd-private-global.cpp test.

2ef6447

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

[SQUASH] Fixed esimd_metadata2.cpp to prevent inlining.

1fbcad5

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

[SQUASH] Applied clang format to ESIMD tests.

abdc4a1

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

[SQUASH] Updated attribute names in ESIMD globals test.

a08bf0f

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

[SQUASH] Move LLVM passes customization for ESIMD under CreatePasses.

05a8351

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

[SQUASH] Fix code formatting.

c9ff9eb

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Update clang/lib/CodeGen/BackendUtil.cpp

3cd2b40

Co-authored-by: Mariya Podchishchaeva <[email protected]>

[SQUASH] Make SYCLLowerESIMDPass run regardless of optimizer options.

17472ad

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

[SQUASH] Remove -O0 - not needed after ESIMD opt passes move.

1197ae8

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

[SYCL][ESIMD] Match 'align' formal param attribute in esimd_subroutin…

97ce032

…e.ll. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

kbobrovs dismissed Fznamznon’s stale review via 97ce032 July 29, 2020 09:49

kbobrovs force-pushed the esimd1-be-util branch from 49a9bdd to 97ce032 Compare July 29, 2020 09:49

kbobrovs requested a review from Fznamznon July 29, 2020 09:50

Fznamznon approved these changes Jul 29, 2020

View reviewed changes

bader approved these changes Jul 29, 2020

View reviewed changes

bader changed the title ~~[SYCL][ESIMD] Setup compilation pipeline for ESIMD, add tests.~~ [SYCL][ESIMD] Setup compilation pipeline for ESIMD Jul 29, 2020

bader requested a review from alexbatashev July 29, 2020 14:08

bader merged commit 44136bd into intel:sycl Jul 29, 2020

alexbatashev approved these changes Jul 29, 2020

View reviewed changes

kbobrovs deleted the esimd1-be-util branch July 30, 2020 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][ESIMD] Setup compilation pipeline for ESIMD #2134

[SYCL][ESIMD] Setup compilation pipeline for ESIMD #2134

kbobrovs commented Jul 17, 2020

bader left a comment

bader Jul 17, 2020

kbobrovs Jul 17, 2020

bader Jul 17, 2020

kbobrovs Jul 17, 2020

alexbatashev left a comment

kbobrovs commented Jul 20, 2020

kbobrovs commented Jul 20, 2020

alexbatashev left a comment

bader Jul 20, 2020

kbobrovs Jul 20, 2020

bader Jul 20, 2020

kbobrovs Jul 21, 2020

bader Jul 21, 2020

kbobrovs Jul 22, 2020

bader Jul 22, 2020

kbobrovs Jul 22, 2020 •

edited

Loading

bader left a comment

elizabethandrews left a comment

kbobrovs commented Jul 22, 2020

kbobrovs commented Jul 22, 2020

kbobrovs commented Jul 22, 2020

bader Jul 23, 2020

kbobrovs Jul 23, 2020

kbobrovs commented Jul 29, 2020

kbobrovs commented Jul 29, 2020

bader commented Jul 29, 2020

		Value *TranslatedV =
		TranslateFunc(*CI, SpirvIntrName.substr(1, 1));

[SYCL][ESIMD] Setup compilation pipeline for ESIMD #2134

[SYCL][ESIMD] Setup compilation pipeline for ESIMD #2134

Conversation

kbobrovs commented Jul 17, 2020

bader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbatashev left a comment

Choose a reason for hiding this comment

kbobrovs commented Jul 20, 2020

kbobrovs commented Jul 20, 2020

alexbatashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbobrovs Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

bader left a comment

Choose a reason for hiding this comment

elizabethandrews left a comment

Choose a reason for hiding this comment

kbobrovs commented Jul 22, 2020

kbobrovs commented Jul 22, 2020

kbobrovs commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbobrovs commented Jul 29, 2020

kbobrovs commented Jul 29, 2020

bader commented Jul 29, 2020

kbobrovs Jul 22, 2020 •

edited

Loading