[SYCL][ClangLinkerWrapper] Add SYCL Module split library usage in clang-linker-wrapper #13806

maksimsab · 2024-05-16T13:00:47Z

This patch introduces -sycl-module-split-mode command line option in clang-linker-wrapper tool and makes the tool invoke either library or sycl-post-link tool depending on the presense of -sycl-module-split-mode option.

Also the patch removes funcionality of reading/writing of Table files in clang-linker-wrapper. Instead of them the tool starts using in-memory structures.

maksimsab · 2024-05-21T16:32:14Z

Note for reviewers.
This patch functionally depends on #13282. It is supposed that the current patch will be merged strictly after #13282. However, in order to make it work the corresponding functionality was copy-pasted from #13282. If you have any issues about splitting part then, please, address them directly to #13282.

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

asudarsa · 2024-06-05T01:00:53Z

clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td

@@ -163,6 +163,15 @@ def sycl_post_link_options_EQ : Joined<["--", "-"], "sycl-post-link-options=">,
  Flags<[WrapperOnlyOption]>,
  HelpText<"Options that will control sycl-post-link step">;

+def sycl_module_split_mode_EQ :


I think we will need to add tests to make sure that the existing tests which use the sycl-post-link will run the same way when we use the split library. I think this might be a good place to start adding E2E tests using new offloading model.

Thanks

asudarsa · 2024-06-05T01:44:36Z

llvm/lib/SYCLLowerIR/ModuleSplitter.cpp

+  ModuleDesc MD = std::move(M); // makeModuleDesc() ?
+  // FIXME: false arguments are temporary for now.
+  auto Splitter =
+      getDeviceCodeSplitter(std::move(MD), Settings.Mode, false, false);


We can try and pass the entries from -sycl-post-link-options via the Settings variable. I am ok to do it in a subsequent PR. There will be other options like -emit-exported-symbols, -emit-imported-symbols if/when we want to add dynamic linking support here.

Thanks

asudarsa · 2024-06-05T01:46:18Z

llvm/lib/SYCLLowerIR/ModuleSplitter.cpp

+  std::vector<SplitModule> OutputImages;
+  while (Splitter->hasMoreSplits()) {
+    ModuleDesc MD2 = Splitter->nextSplit();
+    MD2.fixupLinkageOfDirectInvokeSimdTargets();


We will not be upstreaming this. Do we want to add a TODO marker for tracking that?

Thanks

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

asudarsa · 2024-06-05T02:09:24Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

+}
+
+static Expected<std::vector<module_split::SplitModule>>
+runSYCLSplitLibrary(ArrayRef<StringRef> InputFiles, const ArgList &Args,


We will want to parse the -sycl-post-link-options here.

Can you please share your thoughts on this? Thanks

The issue here is that the library is not complete. The plan is to fulfill missing functionality and add the corresponding splitting option parsing.

clang/test/Driver/linker-wrapper-sycl.cpp

asudarsa

The change looks great. Specifically, the move from using 'table' file to using SplitModules is a very good segway to start using the generic 'Image' data structure (extended for SYCL).
I have added a few comments to be addressed. No major concerns from me.

Thanks

sarnex · 2024-06-07T14:16:59Z

This looks great and I should be able to easily react to getting rid of the table in my thinLTO prototype, I'll take an in-depth look once we rebase on top of HEAD to resolve conflicts.

…er-wrapper This patch introduces -sycl-module-split-mode command line option in clang-linker-wrapper tool and makes the tool invoke either library or sycl-post-link tool depending on the presense of -sycl-module-split-mode option. Also the patch removes funcionality of reading/writing of Table files in clang-linker-wrapper. Instead of them the tool starts using in-memory structures.

maksimsab · 2024-07-05T15:54:00Z

I will do final cleanup and reopen this PR for CR.

clang/test/Driver/linker-wrapper-sycl.cpp

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

asudarsa · 2024-07-09T14:49:53Z

Hi @maksimsab
This PR looks good as an incremental change. It is a little difficult to understand the status of these changes though. In the PR description, can you please add the list of sycl-post-link features that are already available in the library (for e.g. code splitting) and also the list of sycl-post-link features that are not yet available (e.g. prop/sym handling and -sycl-post-link-options handling) and also a short plan on when/how they will be added.
In terms of testing, is my understanding correct that the library flow is not yet ready for E2E testing?

Thanks

clang/tools/clang-linker-wrapper/LinkerWrapperOpts.td

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

sarnex · 2024-07-09T17:56:30Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

+    auto [PropertyFilePath, SymbolsFilePath] = Rem.split("|");
+    llvm::util::PropertySetRegistry Properties;
+    std::string Symbols;
+    if (!PropertyFilePath.empty()) {


Is props/syms being empty valid? Should we error?

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

sarnex

lgtm, thanks! only minor comments

sarnex · 2024-07-10T15:02:14Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

-    return *TempFileOrErr;
+  llvm::module_split::ModuleSplitterSettings Settings;
+  Settings.Mode = Mode;
+  Settings.OutputPrefix = "";


Just a question, are there any cases where we're going to set a prefix in the future?

In fact, it should be changed when -save-temps is used.

sarnex · 2024-07-10T15:05:09Z

llvm/include/llvm/SYCLLowerIR/ModuleSplitter.h

@@ -45,6 +46,10 @@ enum IRSplitMode {
  SPLIT_NONE        // no splitting
 };

+// \returns IRSplitMode value if \p S is recognized. Otherwise, std::nullopt is


nit: mix between // comments here and /// for parseSplitModulesFromFile

mdtoguchi · 2024-07-10T15:16:19Z

clang/test/Driver/linker-wrapper-sycl.cpp

@@ -90,7 +103,7 @@
 //
 // RUN: clang-linker-wrapper -sycl-device-libraries=%t3.devicelib.o -sycl-post-link-options="SYCL_POST_LINK_OPTIONS" -llvm-spirv-options="LLVM_SPIRV_OPTIONS" "--host-triple=x86_64-unknown-linux-gnu" "--linker-path=/usr/bin/ld" "--" HOST_LINKER_FLAGS "-dynamic-linker" HOST_DYN_LIB "-o" "a.out" HOST_LIB_PATH HOST_STAT_LIB %t3.o --dry-run 2>&1 | FileCheck -check-prefix=CHK-CMDS-AOT-NV %s
 // CHK-CMDS-AOT-NV: "{{.*}}spirv-to-ir-wrapper" {{.*}} -o [[FIRSTLLVMLINKIN:.*]].bc --llvm-spirv-opts --spirv-preserve-auxdata --spirv-target-env=SPV-IR --spirv-builtin-format=global
-// CHK-CMDS-AOT-NV-NEXT: "{{.*}}llvm-link" [[FIRSTLLVMLINKIN:.*]].bc -o [[FIRSTLLVMLINKOUT:.*]].bc --suppress-warnings
+// CHK-CMDS-AOT-NV-NEXT: "{{.*}}llvm-link" [[FIRSTLLVMLINKIN]].bc -o [[FIRSTLLVMLINKOUT:.*]].bc --suppress-warnings


Eagle eyes to catch the re-setting of the FIRSTLLVMLINKIN var!

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

asudarsa

LGTM. I have one outstanding question about how you plan to handle -sycl-post-link-options in the library version. I am ok with a plan to support it in a future PR. But I would like to see some path forward here.
Thanks again for this PR.

sarnex · 2024-07-15T15:19:23Z

@asudarsa Is it okay if I merge this PR? It doesn't seem like your comment is blocking. I'm dependent on this for thinLTO work. Thanks.

sarnex · 2024-07-15T15:31:57Z

Sorry Arvind is on vacation for a while.

@maksimsab Is it okay if I merge this now and you can address any feedabck from Arvind when he comes back? Thanks.

maksimsab · 2024-07-15T15:34:50Z

@sarnex Sure! I have the upcoming patch that depends on this one.

sarnex · 2024-07-15T15:36:27Z

Thanks!

maksimsab had a problem deploying to WindowsCILock May 16, 2024 13:01 — with GitHub Actions Failure

maksimsab temporarily deployed to WindowsCILock May 16, 2024 13:45 — with GitHub Actions Inactive

maksimsab force-pushed the sycl_split_functionality_part3 branch 2 times, most recently from 30ce597 to e463a8f Compare May 21, 2024 13:25

maksimsab had a problem deploying to WindowsCILock May 21, 2024 15:52 — with GitHub Actions Failure

maksimsab marked this pull request as ready for review May 21, 2024 16:32

maksimsab requested review from a team as code owners May 21, 2024 16:32

maksimsab requested a review from asudarsa May 21, 2024 16:32

maksimsab force-pushed the sycl_split_functionality_part3 branch from e463a8f to 8789bf1 Compare May 22, 2024 12:17

maksimsab had a problem deploying to WindowsCILock May 22, 2024 12:18 — with GitHub Actions Failure

maksimsab had a problem deploying to WindowsCILock May 22, 2024 13:01 — with GitHub Actions Failure

maksimsab commented May 23, 2024

View reviewed changes

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp Outdated Show resolved Hide resolved

asudarsa reviewed Jun 5, 2024

View reviewed changes

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp Show resolved Hide resolved

asudarsa reviewed Jun 5, 2024

View reviewed changes

asudarsa requested review from sarnex, AlexeySachkov and LU-JOHN June 5, 2024 02:21

asudarsa reviewed Jun 5, 2024

View reviewed changes

clang/test/Driver/linker-wrapper-sycl.cpp Outdated Show resolved Hide resolved

asudarsa reviewed Jun 5, 2024

View reviewed changes

maksimsab had a problem deploying to WindowsCILock June 12, 2024 13:35 — with GitHub Actions Error

maksimsab had a problem deploying to WindowsCILock June 12, 2024 13:46 — with GitHub Actions Failure

maksimsab temporarily deployed to WindowsCILock June 12, 2024 14:48 — with GitHub Actions Inactive

maksimsab force-pushed the sycl_split_functionality_part3 branch from 450524d to fbcc7c6 Compare July 5, 2024 15:51

maksimsab temporarily deployed to WindowsCILock July 5, 2024 15:52 — with GitHub Actions Inactive

maksimsab temporarily deployed to WindowsCILock July 5, 2024 17:36 — with GitHub Actions Inactive

clean up code and clang-format

faa6fed

maksimsab temporarily deployed to WindowsCILock July 8, 2024 16:23 — with GitHub Actions Inactive

maksimsab temporarily deployed to WindowsCILock July 8, 2024 18:10 — with GitHub Actions Inactive

asudarsa reviewed Jul 9, 2024

View reviewed changes

clang/test/Driver/linker-wrapper-sycl.cpp Outdated Show resolved Hide resolved

asudarsa reviewed Jul 9, 2024

View reviewed changes

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp Outdated Show resolved Hide resolved

asudarsa reviewed Jul 9, 2024

View reviewed changes

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp Show resolved Hide resolved

asudarsa reviewed Jul 9, 2024

View reviewed changes

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp Outdated Show resolved Hide resolved

asudarsa reviewed Jul 9, 2024

View reviewed changes

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp Show resolved Hide resolved

sarnex reviewed Jul 9, 2024

View reviewed changes

resolve CR issues

21fd73d

maksimsab temporarily deployed to WindowsCILock July 10, 2024 14:09 — with GitHub Actions Inactive

sarnex approved these changes Jul 10, 2024

View reviewed changes

maksimsab temporarily deployed to WindowsCILock July 10, 2024 15:12 — with GitHub Actions Inactive

mdtoguchi approved these changes Jul 10, 2024

View reviewed changes

asudarsa reviewed Jul 11, 2024

View reviewed changes

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp Show resolved Hide resolved

asudarsa reviewed Jul 11, 2024

View reviewed changes

asudarsa mentioned this pull request Jul 11, 2024

[NVPTX][AMD][New offload model] Add support for -fsycl-embed-ir in the new offloading model #14526

Merged

sarnex merged commit 8761d40 into intel:sycl Jul 15, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][ClangLinkerWrapper] Add SYCL Module split library usage in clang-linker-wrapper #13806

[SYCL][ClangLinkerWrapper] Add SYCL Module split library usage in clang-linker-wrapper #13806

maksimsab commented May 16, 2024

maksimsab commented May 21, 2024

asudarsa Jun 5, 2024 •

edited

Loading

asudarsa Jun 5, 2024

asudarsa Jun 5, 2024

asudarsa Jun 5, 2024

asudarsa Jul 11, 2024

maksimsab Jul 12, 2024

asudarsa left a comment

sarnex commented Jun 7, 2024

maksimsab commented Jul 5, 2024

asudarsa commented Jul 9, 2024

sarnex Jul 9, 2024

sarnex left a comment

sarnex Jul 10, 2024

maksimsab Jul 12, 2024

sarnex Jul 10, 2024

mdtoguchi Jul 10, 2024

asudarsa left a comment

sarnex commented Jul 15, 2024

sarnex commented Jul 15, 2024

maksimsab commented Jul 15, 2024

sarnex commented Jul 15, 2024

[SYCL][ClangLinkerWrapper] Add SYCL Module split library usage in clang-linker-wrapper #13806

[SYCL][ClangLinkerWrapper] Add SYCL Module split library usage in clang-linker-wrapper #13806

Conversation

maksimsab commented May 16, 2024

maksimsab commented May 21, 2024

asudarsa Jun 5, 2024 • edited Loading

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

asudarsa left a comment

Choose a reason for hiding this comment

sarnex commented Jun 7, 2024

maksimsab commented Jul 5, 2024

asudarsa commented Jul 9, 2024

Choose a reason for hiding this comment

sarnex 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

asudarsa left a comment

Choose a reason for hiding this comment

sarnex commented Jul 15, 2024

sarnex commented Jul 15, 2024

maksimsab commented Jul 15, 2024

sarnex commented Jul 15, 2024

asudarsa Jun 5, 2024 •

edited

Loading