[sycl-post-link] Split SYCL and ESIMD kernels into separate modules #3044

DenisBakhvalov · 2021-01-16T01:27:46Z

For now, this change doesn't have any effect on existing programs since we don't allow mixing SYCL and ESIMD kernels in one source or in one program.
But this is an essential step towards allowing such a mix since ESIMD kernels require specific processing as opposed to usual SYCL kernels.

PLEASE, only review commit: 147eb5f

Patch (1/3): #3042
Patch (2/3): #3043
Patch (3/3): this PR

This patch is a preparation for spliting ESIMD and SYCL kernels.

For now this change doesn't have any effect on existing programs since we don't allow mixing SYCL and ESIMD kernels in one source or in one program. But this is an essential step towards this goal since ESIMD kernels require specific processing as opposed to usual SYCL kernels.

AlexeySachkov · 2021-01-19T16:36:22Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+  std::string RetStr = Dir + sys::path::stem(OutputFilename).str() + "_";
+  if (IsEsimd)
+    RetStr += "esimd_";
+  return RetStr + std::to_string(I) + Ext.str();


Am I right that this filename change was only introduced to simplify debugging, i.e. there is no functional need for it?

I have a small concern that a number of functions were changed just to have this prefix added and it is kind of a non-generic solution (like what if we decided to add one more level of device code split for some other feature?)

Before my change the code was split into modules and saved to files with the name derived from OutputFilename plus sequential index, i.e. /path/to/file_01.bc, /path/to/file_02.bc, etc.

Now, when we split ESIMD modules we cannot overwrite already processed SYCL files, this is why we need to change the name of the ESIMD files. We could have:

`/path/to/file_01.bc` // sycl `/path/to/file_02.bc` // sycl `/path/to/file_esimd_01.bc` // esimd `/path/to/file_esimd_02.bc` // esimd

or

`/path/to/file_01.bc` // sycl `/path/to/file_02.bc` // sycl `/path/to/file_03.bc` // esimd `/path/to/file_04.bc` // esimd

I choose the first scheme, since, as you said, it will be easier to find ESIMD modules, and the names of the files do not affect functionality. But I'm OK to change if you think the second is better. However, I think we need to propagate the IsEsimd flag (or other parameter) regardless of the naming scheme.

We can introduce a class that will keep the state. Then all the propagation will go away. But we can do that later in further patches.

I choose the first scheme, since, as you said, it will be easier to find ESIMD modules, and the names of the files do not affect functionality. But I'm OK to change if you think the second is better.

I don't have a strong opinion here, so, the proposed way looks fine to me. We can hear from @kbobrovs here to have another opinion

However, I think we need to propagate the IsEsimd flag (or other parameter) regardless of the naming scheme.

Right, but from functional point of view it is only needed in processOneModule

We can introduce a class that will keep the state. Then all the propagation will go away. But we can do that later in further patches.

Personally, I'm not a fun of such states, because it makes it harder to understand where particular value is came from and how the call graph looks like

Current scheme with _esimd suffix seems clearer to me. Users don't care, but devs will appreciate it.

AlexeySachkov

This change should probably be updated to ToT because of the conflict, but in general I don't have any objections, LGTM

llvm/test/tools/sycl-post-link/sycl-esimd/basic-sycl-esimd-split.ll

llvm/test/tools/sycl-post-link/sycl-esimd/sycl-esimd-split-per-kernel.ll

llvm/test/tools/sycl-post-link/sycl-esimd/sycl-esimd-split-per-source.ll

llvm/test/tools/sycl-post-link/sycl-esimd/sycl-esimd-split-symbols.ll

llvm/tools/sycl-post-link/sycl-post-link.cpp

kbobrovs · 2021-01-19T23:27:41Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+  std::string RetStr = Dir + sys::path::stem(OutputFilename).str() + "_";
+  if (IsEsimd)
+    RetStr += "esimd_";
+  return RetStr + std::to_string(I) + Ext.str();


Current scheme with _esimd suffix seems clearer to me. Users don't care, but devs will appreciate it.

llvm/tools/sycl-post-link/sycl-post-link.cpp

llvm/test/tools/sycl-post-link/sycl-esimd/sycl-esimd-split-per-kernel.ll

llvm/tools/sycl-post-link/sycl-post-link.cpp

bader · 2021-01-20T12:45:42Z

@DenisBakhvalov, please, resolve merge conflicts.

1. Check call graph deps in all added tests. 2. Changed signature of makeResultFileName: replaced `bool IsEsimd` flag with `StringRef suffix`. 3. Added -split-esimd option to control SYCL-ESIMD splitting. 4. The new -split-esimd options is not compatible with -ir-output-only. 5. Added a test that checks that SYCL-ESIMD splitting doesn't happen when no '-split-esimd' option provided.

DenisBakhvalov · 2021-01-20T19:03:08Z

@DenisBakhvalov, please, resolve merge conflicts.

Done.

DenisBakhvalov · 2021-01-20T19:08:01Z

@kbobrovs, thanks for the review. I fixed your comments in 0b6c9c7. Please review.

kbobrovs · 2021-01-20T19:54:05Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

@@ -712,13 +793,19 @@ int main(int argc, char **argv) {
      "This is a collection of utilities run on device code's LLVM IR before\n"
      "handing off to back-end for further compilation or emitting SPIRV.\n"
      "The utilities are:\n"
+      "- SYCL and ESIMD kernels can be split into separate modules with\n"
+      "  '-split-esimd' option. The option has no effect when there is only\n"
+      "  one type of kernels in the input module.\n"


Suggest to add:
Functions unreachable from any kernel (including those marked as with "explicit SIMD" metadata) are dropped from the resulting module(s).

kbobrovs

LGTM

kbobrovs · 2021-01-21T22:29:55Z

This change should probably be updated to ToT because of the conflict, but in general I don't have any objections, LGTM

treating this as approval from @AlexeySachkov. No other objections - merging...

DenisBakhvalov requested review from AlexeySachkov, Fznamznon and kbobrovs January 16, 2021 01:27

DenisBakhvalov requested a review from bader as a code owner January 16, 2021 01:27

This was referenced Jan 16, 2021

[sycl-post-link][NFC] Extracted the code into a subroutine #3042

Merged

[sycl-post-link][NFC] Refactored writing to the output table #3043

Merged

DenisBakhvalov added 3 commits January 17, 2021 12:23

[sycl-post-link][NFC] Extracted the code into a subroutine

d60195b

This patch is a preparation for spliting ESIMD and SYCL kernels.

[sycl-post-link][NFC] Refactored writing to the output table

0fde11d

This patch is a preparation for spliting ESIMD and SYCL kernels.

DenisBakhvalov force-pushed the mix-sycl-esimd-split-3 branch from 4f79b5d to 147eb5f Compare January 18, 2021 04:07

AlexeySachkov reviewed Jan 19, 2021

View reviewed changes

AlexeySachkov previously approved these changes Jan 19, 2021

View reviewed changes

kbobrovs reviewed Jan 19, 2021

View reviewed changes

Merge remote-tracking branch 'sycl' into mix-sycl-esimd-split-3

016bcc5

DenisBakhvalov dismissed AlexeySachkov’s stale review via 016bcc5 January 20, 2021 00:15

DenisBakhvalov added 2 commits January 20, 2021 10:51

Merge remote-tracking branch 'upstream/sycl' into mix-sycl-esimd-split-3

b6160c7

kbobrovs reviewed Jan 20, 2021

View reviewed changes

Added a comment about unreachable functions

8d71009

DenisBakhvalov requested a review from kbobrovs January 20, 2021 21:40

kbobrovs approved these changes Jan 21, 2021

View reviewed changes

kbobrovs requested a review from AlexeySachkov January 21, 2021 20:46

kbobrovs merged commit a599c44 into intel:sycl Jan 21, 2021

DenisBakhvalov mentioned this pull request Jan 25, 2021

[SYCL][Driver] Enable splitting of ESIMD code in sycl-post-link #3086

Merged

[sycl-post-link] Split SYCL and ESIMD kernels into separate modules #3044

[sycl-post-link] Split SYCL and ESIMD kernels into separate modules #3044

Uh oh!

Conversation

DenisBakhvalov commented Jan 16, 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!

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!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bader commented Jan 20, 2021

Uh oh!

DenisBakhvalov commented Jan 20, 2021

Uh oh!

DenisBakhvalov commented Jan 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

kbobrovs commented Jan 21, 2021

Uh oh!

Uh oh!

DenisBakhvalov commented Jan 16, 2021 •

edited

Loading