[SYCL] sycl-post-link changes to support invoke_simd. #6160

kbobrovs · 2022-05-17T07:16:24Z

Split into SYCL and ESIMD callgraph is now obligatory, as these kinds
of functions undergo different set of IR t-forms. If no SYCL/ESIMD
splitting was requested on the command line, then the tool clones
functions shared between the two callgraphs and then links them back
together. This is future default for the compiler driver, as for
invoke_simd to be correctly handled by the BE, both kinds of code
must be in the same module.
Logic of the output file table generation refactored to be more
straightforward.
ModuleDesc extended to accomodate properties and support entry point
(Function object) replacement with new entry with the same name.
Make --split-esimd a first-class action of sycl-post-link - if passed
alone, the tool will no longer complain that no actions are requested.
Fixed bug in ModuleSplitter.cpp/extractSubModule where no actual
replacement of entry point Functions with cloned ones happenned.

kbobrovs · 2022-05-18T20:59:50Z

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

  return EntryPointGroups;
 }

-// For device global variables with the 'device_image_scope' property,


This function was just moved out of the anonymous namespace into ModuleSplitterBase to make it callable from the sycl-post-link.cpp. It was also renamed to verifyNoCrossModuleDeviceGlobalUsage to reflect the purpose more closely.

kbobrovs · 2022-05-18T21:00:45Z

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

@@ -424,3 +366,213 @@ std::unique_ptr<ModuleSplitterBase> module_split::getSplitterByMode(
  else
    return std::make_unique<ModuleCopier>(std::move(M), std::move(Groups));
 }
+
+void ModuleSplitterBase::verifyNoCrossModuleDeviceGlobalUsage() {


This is a rename+move of checkImageScopedDeviceGlobals function.

kbobrovs · 2022-05-18T21:01:39Z

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

@@ -675,8 +806,10 @@ int main(int argc, char **argv) {
      "  '-split=auto' mode automatically selects the best way of splitting\n"
      "  kernels into modules based on some heuristic.\n"
      "  The '-split' option is compatible with '-split-esimd'. In this case,\n"
-      "  first input module will be split into SYCL and ESIMD modules. Then\n"
-      "  both modules will be further split according to the '-split' option.\n"
+      "  first input module will be split according to the '-split' option\n"


-split is now applied before -split-esimd, hence the update

kbobrovs · 2022-05-18T21:02:34Z

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

@@ -706,14 +839,16 @@ int main(int argc, char **argv) {

  bool DoSplit = SplitMode.getNumOccurrences() > 0;
  bool DoSplitEsimd = SplitEsimd.getNumOccurrences() > 0;
+  bool DoLowerEsimd = LowerEsimd.getNumOccurrences() > 0;


-lower-esimd is now a first-class action - i.e. it can be the only action requested from sycl-post-link, w/o causing option processing error.

kbobrovs · 2022-05-18T21:03:41Z

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

+    OutputFilename = (Twine(sys::path::stem(InputFilename)) + S).str();
+  }
+
+  std::unique_ptr<util::SimpleTable> Table = processInputModule(std::move(M));


Table generation was "homogenised" and consolidated in processInputModule

kbobrovs · 2022-05-18T21:04:48Z

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

+  // Do invoke_simd processing before splitting because this:
+  // - saves processing time (the pass is run once, even though on larger IR)
+  // - doing it before SYCL/ESIMD splitting is required for correctness
+  Modified |= lowerInvokeSimd(*M);


This is a new addition for invoke_simd processing. This t-form was dormant in the code base until now.

kbobrovs · 2022-05-18T21:06:25Z

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

+  Modified |= Splitter->totalSplits() > 1;
+
+  if (DeviceGlobals)
+    Splitter->verifyNoCrossModuleDeviceGlobalUsage();


This check was done internally in the splitter, which required unnecessary DeviceGloabls option propagation, now it is moved here.

kbobrovs · 2022-05-18T21:08:21Z

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


-TableFiles processOneModule(std::unique_ptr<Module> M, bool IsEsimd,


this was refactored and in-lined into processInputModule for better clarity:

table generation was consolidated and simplified

CanReuseInputModule and IROutputOnly processing logic also simplified

kbobrovs · 2022-05-18T21:10:12Z

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

+    Splitter->verifyNoCrossModuleDeviceGlobalUsage();
+
+  // Proceed with top-level splitting.
+  while (Splitter->hasMoreSplits()) {


processInputModule now has two nested splitters - SYCL/ESIMD is nested within scoped one.

kbobrovs · 2022-05-18T21:24:04Z

llvm/test/tools/sycl-post-link/auto-module-split-1.ll

@@ -10,27 +10,27 @@ target triple = "spir64-unknown-linux"

 $_Z3barIiET_S0_ = comdat any

-; CHECK-TU0-NOT: @{{.*}}GV{{.*}}
-; CHECK-TU1: @{{.*}}GV{{.*}} = internal addrspace(1) constant [1 x i32] [i32 42], align 4
+; CHECK-TU1-NOT: @{{.*}}GV{{.*}}


all test changes are related to re-numeration of output files - E.g. ESIMD kernels appeared in files generated earlier (with smaller numeric index), now they will appear in files generated later, etc.

v-klochkov · 2022-05-19T17:13:59Z

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

+#ifdef _NDEBUG
+#define DUMP_ENTRY_POINTS(args...)
+#else
+constexpr int DebugPostLink = 0;
+
+#define DUMP_ENTRY_POINTS(...)                                                 \
+  if (DebugPostLink > 0) {                                                     \
+    llvm::module_split::dumpEntryPoints(__VA_ARGS__);                          \
+  }
+#endif
+


I have 2 suggestions for this code.

If someone needs to see debug output, the one have to change "DebugPostLink = 0" with "DebugPostLink = 1;" and re-build. When don't need it anymore, need to cancel change and re-build again.
Wouldn't it be more convenient to have a macro?

Suggested change

#ifdef _NDEBUG

#define DUMP_ENTRY_POINTS(args...)

#else

constexpr int DebugPostLink = 0;

#define DUMP_ENTRY_POINTS(...) \

if (DebugPostLink > 0) { \

llvm::module_split::dumpEntryPoints(__VA_ARGS__); \

}

#endif

#if defined(_NDEBUG) || !defined(DEBUG_POST_LINK)

#define DUMP_ENTRY_POINTS(args...)

#else

#define DUMP_ENTRY_POINTS(...) \

llvm::module_split::dumpEntryPoints(__VA_ARGS__); \

#endif

Why not to have a normal C++ function, something like this?

Suggested change

#ifdef _NDEBUG

#define DUMP_ENTRY_POINTS(args...)

#else

constexpr int DebugPostLink = 0;

#define DUMP_ENTRY_POINTS(...) \

if (DebugPostLink > 0) { \

llvm::module_split::dumpEntryPoints(__VA_ARGS__); \

}

#endif

template <typename... T>

inline void debugEntryPoints(T... Args) {

#if !defined(_NDEBUG) && defined(DEBUG_POST_LINK)

llvm::module_split::dumpEntryPoints(Args...);

#endif

}

Wouldn't it be more convenient to have a macro?

I don't think it is more convenient. Adding a macro to the LLVM build requires re-configuring - this seems a lot more disturbing.

Why not to have a normal C++ function, something like this?

I don't see advantages in having this as a function. DUMP_ENTRY_POINTS in all caps is intuitively understandable that this is a macro call, and has the same efforts on the call site as debugEntryPoints.

had to re-base to re-test with fresh top-of-trunk

- Split into SYCL and ESIMD callgraph is now obligatory, as these kinds of functions undergo different set of IR t-forms. If no SYCL/ESIMD splitting was requested on the command line, then the tool clones functions shared between the two callgraphs and then links them back together. This is future default for the compiler driver, as for invoke_simd to be correctly handled by the BE, both kinds of code must be in the same module. - Logic of the output file table generation refactored to be more straightforward. - ModuleDesc extended to accomodate properties and support entry point (Function object) replacement with new entry with the same name. - Make --split-esimd a first-class action of sycl-post-link - if passed alone, the tool will no longer complain that no actions are requested. - Fixed bug in ModuleSplitter.cpp/extractSubModule where no actual replacement of entry point Functions with cloned ones happenned.

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

v-klochkov · 2022-05-24T18:05:47Z

llvm/tools/sycl-post-link/ModuleSplitter.h

 };

 using EntryPointGroupVec = std::vector<EntryPointGroup>;

-struct ModuleDesc {
+enum class ESIMDStatus { SYCL_ONLY, ESIMD_ONLY, SYCL_AND_ESIMD };


Minor comment: IMO, the name of this enum is a bit confusing. Wouldn't it be more clear to name it something like SplitModuleStatus or ModuleStatus?

Agree. I'm thinking about something like SyclEsimdSplitStatus

v-klochkov · 2022-05-24T18:08:38Z

llvm/tools/sycl-post-link/ModuleSplitter.h

+  }
+
+  bool isESIMD() const { return Props.HasEsimd == ESIMDStatus::ESIMD_ONLY; }
+  bool isSYCL() const { return Props.HasEsimd == ESIMDStatus::SYCL_ONLY; }


'HasEsimd == ESIMDStatus::SYCL_ONLY' is a bit awkward, wouldn't it be better to say something like:
'Props.ModuleFeatures == ModuleStatus::SYCL_ONLY' ?

v-klochkov · 2022-05-24T18:08:38Z

llvm/tools/sycl-post-link/ModuleSplitter.h

+  }
+
+  bool isESIMD() const { return Props.HasEsimd == ESIMDStatus::ESIMD_ONLY; }
+  bool isSYCL() const { return Props.HasEsimd == ESIMDStatus::SYCL_ONLY; }


'HasEsimd == ESIMDStatus::SYCL_ONLY' is a bit awkward, wouldn't it be better to say something like:
'Props.ModuleFeatures == ModuleStatus::SYCL_ONLY' ?

I agree about ESIMDStatus "awkwardness". "ModuleFeatures" is too general IMO.

llvm/tools/sycl-post-link/ModuleSplitter.h

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

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

asudarsa · 2022-05-26T02:19:17Z

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

+
+// @param MD Module descriptor to save
+// @param IRFilename filename of already avaialble IR component. If not empty,
+//   IR component saving is skipped, and this file name as recorded as such in


typo: is recorded as

asudarsa · 2022-05-26T02:33:58Z

LGTM overall. Some minor changes proposed. Thanks

v-klochkov · 2022-05-24T18:05:47Z

llvm/tools/sycl-post-link/ModuleSplitter.h

 };

 using EntryPointGroupVec = std::vector<EntryPointGroup>;

-struct ModuleDesc {
+enum class ESIMDStatus { SYCL_ONLY, ESIMD_ONLY, SYCL_AND_ESIMD };


Minor comment: IMO, the name of this enum is a bit confusing. Wouldn't it be more clear to name it something like SplitModuleStatus or ModuleStatus?

asudarsa

LGTM. Thanks

pvchupin · 2022-06-01T22:02:37Z

@kbobrovs, @v-klochkov, please handle post-commit fail: https://github.com/intel/llvm/runs/6694744289?check_suite_focus=true

kbobrovs · 2022-06-02T06:04:47Z

@kbobrovs, @v-klochkov, please handle post-commit fail: https://github.com/intel/llvm/runs/6694744289?check_suite_focus=true

@pvchupin, I'm on it

HaohaiWen · 2022-06-02T06:13:15Z

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

+  }
+  // Verify entry points:
+  for (const auto *F : entries()) {
+    const bool IsESIMDFunction = isESIMDFunction(*F);


Consider to add "(void)IsESIMDFunction;" or wrap this line under macro like #ifdef DEBUG to avoid unused variable warning/error.

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

kbobrovs · 2022-06-02T06:28:59Z

@kbobrovs, @v-klochkov, please handle post-commit fail: https://github.com/intel/llvm/runs/6694744289?check_suite_focus=true

@pvchupin, I'm on it

@pvchupin - the fix is created: #6231

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

kbobrovs commented May 18, 2022

View reviewed changes

kbobrovs marked this pull request as ready for review May 19, 2022 07:23

kbobrovs requested a review from a team as a code owner May 19, 2022 07:23

kbobrovs requested a review from v-klochkov May 19, 2022 07:23

v-klochkov reviewed May 19, 2022

View reviewed changes

kbobrovs added 6 commits May 19, 2022 15:43

Test failure fixes.

5759c00

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

More test fixes

198c42a

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

Fixed help message, fix more tests

a8d7b9a

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

Modify names of SYCL/ESIMD shared functions before linkage, add test.

44cbb8e

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

added test comment

d067250

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

kbobrovs force-pushed the invoke_simd_post_link branch from ded3946 to d067250 Compare May 19, 2022 23:29

kbobrovs mentioned this pull request May 21, 2022

[WIP][SYCL] Make sycl-post-link process split modules sequentially, simplify. #5001

Closed

v-klochkov previously approved these changes May 24, 2022

View reviewed changes

asudarsa reviewed May 25, 2022

View reviewed changes

llvm/tools/sycl-post-link/ModuleSplitter.h Show resolved Hide resolved

asudarsa reviewed May 25, 2022

View reviewed changes

llvm/tools/sycl-post-link/ModuleSplitter.h Outdated Show resolved Hide resolved

asudarsa reviewed May 25, 2022

View reviewed changes

llvm/tools/sycl-post-link/ModuleSplitter.h Outdated Show resolved Hide resolved

asudarsa reviewed May 25, 2022

View reviewed changes

llvm/tools/sycl-post-link/ModuleSplitter.h Show resolved Hide resolved

asudarsa reviewed May 26, 2022

View reviewed changes

llvm/tools/sycl-post-link/ModuleSplitter.cpp Show resolved Hide resolved

asudarsa reviewed May 26, 2022

View reviewed changes

llvm/tools/sycl-post-link/sycl-post-link.cpp Outdated Show resolved Hide resolved

asudarsa reviewed May 26, 2022

View reviewed changes

llvm/tools/sycl-post-link/sycl-post-link.cpp Outdated Show resolved Hide resolved

asudarsa reviewed May 26, 2022

View reviewed changes

llvm/tools/sycl-post-link/sycl-post-link.cpp Show resolved Hide resolved

asudarsa reviewed May 26, 2022

View reviewed changes

llvm/tools/sycl-post-link/sycl-post-link.cpp Outdated Show resolved Hide resolved

asudarsa reviewed May 26, 2022

View reviewed changes

kbobrovs added 2 commits May 31, 2022 06:18

Merge remote-tracking branch 'intel/sycl' into invoke_simd_post_link

e207813

address review comments

fdec8e0

kbobrovs dismissed v-klochkov’s stale review via fdec8e0 June 1, 2022 10:10

kbobrovs requested review from asudarsa and v-klochkov June 1, 2022 10:12

v-klochkov approved these changes Jun 1, 2022

View reviewed changes

asudarsa approved these changes Jun 1, 2022

View reviewed changes

kbobrovs merged commit 8c7bb45 into intel:sycl Jun 1, 2022

kbobrovs deleted the invoke_simd_post_link branch June 1, 2022 17:38

HaohaiWen reviewed Jun 2, 2022

View reviewed changes

kbobrovs added a commit to kbobrovs/llvm that referenced this pull request Jun 2, 2022

Fix shared library build mode failure introduced by intel#6160

12a4405

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

pvchupin pushed a commit that referenced this pull request Jun 2, 2022

Fix shared library build mode failure introduced by #6160 (#6231)

015e922

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


		TableFiles processOneModule(std::unique_ptr<Module> M, bool IsEsimd,

[SYCL] sycl-post-link changes to support invoke_simd. #6160

[SYCL] sycl-post-link changes to support invoke_simd. #6160

Uh oh!

Conversation

kbobrovs commented May 17, 2022

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!

kbobrovs May 18, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asudarsa commented May 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

pvchupin commented Jun 1, 2022

Uh oh!

kbobrovs commented Jun 2, 2022

Uh oh!

HaohaiWen Jun 2, 2022 • 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 commented Jun 2, 2022

Uh oh!

Uh oh!

kbobrovs May 18, 2022 •

edited

Loading

HaohaiWen Jun 2, 2022 •

edited

Loading