[SYCL] Move SYCL Module Splitting to library. Part 2 #13282

maksimsab · 2024-04-04T14:02:14Z

Add SYCL Module Splitting as a library. ESIMD splitting is not present
in this patch and will be added in the upcoming patch.
Add particular testing tool sycl-module-split that invokes added functionality.
The following list of tests from llvm/test/tools/sycl-post-link/device-code-split additionally invokes new added sycl-module-split tool in order to check the library part.
auto-module-split-1.ll
auto-module-split-2.ll
auto-module-split-3.ll
auto-module-split-func-ptr.ll
basic-module-split.ll
complex-indirect-call-chain.ll
one-kernel-per-module.ll
per-aspect-split-1.ll
per-aspect-split-2.ll
per-aspect-split-3.ll
per-joint-matrix-1.ll
per-joint-matrix-2.ll
per-joint-matrix-mad-1.ll
per-joint-matrix-mad-2.ll
per-joint-matrix-mad-4.ll
per-joint-matrix-mad-5.ll
per-reqd-sub-group-size-split-1.ll
per-reqd-sub-group-size-split-2.ll
per-reqd-wg-size-split-1.ll
per-reqd-wg-size-split-2.ll
split-with-kernel-declarations.ll
vtable.ll

The following list of tests wasn't edited in this patch:
per-joint-matrix-3.ll
per-aspect-split-4.ll
per-joint-matrix-mad-3.ll
per-reqd-wg-size-split-3.ll
split-with-func-ptrs.ll

The above tests aren't involved in this patch because they don't test module splitting
itself. They will be migrated in the upcoming patch.
split-with-func-ptrs.ll requires ESIMD splitting and will be migrated
with the corresponding patch.

asudarsa · 2024-04-04T14:27:33Z

Hi Maksim,

Do you want me to start reviewing or wait for it to come out of draft state?

Thanks

maksimsab · 2024-04-04T14:29:04Z

@asudarsa I am going to make it open in an hour.

asudarsa · 2024-04-04T14:30:19Z

@asudarsa I am going to make it open in an hour.

Awesome! Thanks. Also, I do understand the basic premise of this change. But, a few lines in the description about the purpose and scope of this change.

Add SYCL Module Splitting as a library. ESIMD splitting is not present in this patch and will be added in the upcoming patch. Add particular testing tool sycl-module-split that invokes added functionality. Copy the following tests from llvm/test/tools/sycl-post-link/device-code-split: auto-module-split-1.ll auto-module-split-2.ll auto-module-split-3.ll auto-module-split-func-ptr.ll basic-module-split.ll complex-indirect-call-chain.ll one-kernel-per-module.ll per-aspect-split-1.ll per-aspect-split-2.ll per-aspect-split-3.ll per-joint-matrix-1.ll per-joint-matrix-2.ll per-joint-matrix-mad-1.ll per-joint-matrix-mad-2.ll per-joint-matrix-mad-4.ll per-joint-matrix-mad-5.ll per-reqd-sub-group-size-split-1.ll per-reqd-sub-group-size-split-2.ll per-reqd-wg-size-split-1.ll per-reqd-wg-size-split-2.ll split-with-kernel-declarations.ll vtable.ll The above tests were adjusted to use sycl-module-split tool. The following list of tests wasn't copied in this patch: per-joint-matrix-3.ll per-aspect-split-4.ll per-joint-matrix-mad-3.ll per-reqd-wg-size-split-3.ll split-with-func-ptrs.ll The above tests are exclude because they don't test module splitting itself. They will be migrated in the upcoming patch. split-with-func-ptrs.ll requires ESIMD splitting and will be migrated with the corresponding patch.

llvm/tools/sycl-module-split/sycl-module-split.cpp

llvm/include/llvm/SYCLLowerIR/ModuleSplitter.h

llvm/lib/SYCLLowerIR/ModuleSplitter.cpp

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

sarnex

reviewed llvm/lib/SYCLLowerIR/CMakeLists.txt from ESIMD perspective and LGTM

maksimsab · 2024-04-09T14:25:46Z

Update: I removed tests that I added initially. Instead of them I started using existing llvm/test/tool/sycl-post-link/device-code-split/* tests.

asudarsa · 2024-04-10T14:05:33Z

Hi @maksimsab

Thanks for this change. There is one high level question. How will this tool (which eventually will be present as an API) fit into the overall compilation scheme. Can you please provide a few lines? I suppose you are going to attempt that in an upcoming PR?

asudarsa · 2024-04-10T14:19:54Z

llvm/include/llvm/SYCLLowerIR/ModuleSplitter.h

@@ -270,6 +273,33 @@ void dumpEntryPoints(const Module &M, bool OnlyKernelsAreEntryPoints = false,
                     const char *msg = "", int Tab = 0);
 #endif // NDEBUG

+struct SplitModule {


Why do we need this new data structure? Can we not continue to use IrPropSymFilenameTriple?

Thanks

This is possible.

llvm/include/llvm/SYCLLowerIR/ModuleSplitter.h

maksimsab · 2024-04-10T14:29:25Z

How will this tool (which eventually will be present as an API) fit into the overall compilation scheme

This is tool only for LIT testing. In ClangLinkerWrapper only API calls will be used.

asudarsa

This change looks good to me. Couple of nits about using new data structures.
Please provide a couple of lines about how this will fit inside the bigger sycl-post-link functionality before approving.

Good work in incremental changes.

Thanks

asudarsa

LGTM. Thanks

asudarsa · 2024-04-11T14:56:37Z

Hi @jzc

Can you please take a look when convenient? Thanks much.

bader · 2024-04-11T17:04:32Z

Add particular testing tool sycl-module-split that invokes added functionality.

@maksimsab, please, expand PR description with the justification for adding a new tool instead of using standard tool for testing LLVM passes (i.e. opt).

bader · 2024-04-11T17:08:33Z

BTW, there is already existing LLVM tool with similar functionality - llvm-extract. It's worth to consider integrating SYCL module splitting to this tool.

maksimsab · 2024-04-12T11:47:41Z

llvm-split tool is the closest alternative.

asudarsa · 2024-04-12T13:05:47Z

My understanding is that these tools are added as temporary ‘placeholders’ as we move towards implementation the whole ‘SYCL-post-link’ functionality using API calls. So, I am not sure if we need to spend time to improve/ replace these tools. Maksim, please correct if I am mistaken. Thanks

maksimsab · 2024-04-12T14:51:09Z

@asudarsa I see it as a long living tool that is used only for LIT testing because it would be difficult to test splitting stuff using clang-linker-wrapper.

AlexeySachkov

@maksimsab, if we still plan to proceed with the patch, could you please make a merge with sycl branch so that we re-trigger testing on top of the latest changes?

I would also look into d7c3713 to see if we can refactor our code so that it is testable by existing llvm-split tool. If that requires a bigger refactoring (because we don't have target machine for our SYCL targets), then I think it is ok to proceed with our own temporary solution in form of a custom tool to test this code in isolation. But if we proceed with the patch as-is, we should make sure to have a tracker submitted to refactor everything to align everything with the upstream approach

AlexeySachkov · 2024-06-04T13:14:50Z

llvm/lib/SYCLLowerIR/ModuleSplitter.cpp

@@ -733,6 +737,14 @@ void EntryPointGroup::rebuild(const Module &M) {
      Functions.insert(const_cast<Function *>(&F));
 }

+std::string ModuleDesc::makeSymbolTable() const {
+  std::string ST;


llvm::SmallString would be a better fit to reduce amount of re-allocations

The total size of a string is rarely small to benefit from small string optimizations. C++ mangled names are very long. What size would you suggest for SmallString for this case?

It is not that it would fit into a pre-allocated area on the stack, it is that SmallString is a wrapper around SmallVector, which does not fully re-allocate on every += operation, because its capacity grows at a different pace similar to std::vector's push_back

asudarsa · 2024-06-05T00:41:53Z

@maksimsab, if we still plan to proceed with the patch, could you please make a merge with sycl branch so that we re-trigger testing on top of the latest changes?

I would also look into d7c3713 to see if we can refactor our code so that it is testable by existing llvm-split tool. If that requires a bigger refactoring (because we don't have target machine for our SYCL targets), then I think it is ok to proceed with our own temporary solution in form of a custom tool to test this code in isolation. But if we proceed with the patch as-is, we should make sure to have a tracker submitted to refactor everything to align everything with the upstream approach

Thanks for the comment @AlexeySachkov. We do intend to proceed with this patch as an incremental change. @maksimsab, please merge with sycl TIP and retest. We can merge once testing is complete. I had a look at the PR you mentioned and I think we should be able to use our SPIR-V backend for this purpose and it should be pretty straightforward to shoehorn llvm-split tool for our purposes. We will open a tracker for that.

Sincerely

maksimsab · 2024-06-07T11:40:02Z

SYCL :: syclcompat/memory/memory_management_test3.cpp fails in other PRs as well.

AlexeySachkov · 2024-06-07T12:31:12Z

SYCL :: syclcompat/memory/memory_management_test3.cpp fails in other PRs as well.

The issue is reported in #14086 and a fix is being developed in #14080, I will proceed with the merge.

AlexeySachkov · 2024-06-07T13:52:45Z

The commit seems to have broken shared libraries build: https://github.com/intel/llvm/actions/runs/9417067694/job/25941497305

I'm looking into it

AlexeySachkov · 2024-06-07T14:09:47Z

#14094 should fix post-commit

Added SYCL Module Splitting as a library. ESIMD splitting is not present in this patch and will be added in an upcoming patch. Added particular testing tool sycl-module-split that invokes added functionality. Not all `device-code-split` tests were updated in this patch because the rest of them (mostly) don't test module splitting itself. They will be migrated in an upcoming patch.

maksimsab had a problem deploying to WindowsCILock April 4, 2024 14:09 — with GitHub Actions Error

maksimsab force-pushed the sycl_split_functionality_part2 branch from 2ce8022 to 7f07cb4 Compare April 4, 2024 14:32

maksimsab had a problem deploying to WindowsCILock April 4, 2024 14:44 — with GitHub Actions Failure

maksimsab marked this pull request as ready for review April 4, 2024 14:56

maksimsab requested a review from a team as a code owner April 4, 2024 14:56

maksimsab requested review from asudarsa and AlexeySachkov April 4, 2024 14:56

maksimsab temporarily deployed to WindowsCILock April 4, 2024 15:53 — with GitHub Actions Inactive

jzc reviewed Apr 4, 2024

View reviewed changes

maksimsab commented Apr 5, 2024

View reviewed changes

llvm/test/tools/sycl-module-split/auto-module-split-1.ll Outdated Show resolved Hide resolved

address code review issues

19fca2c

maksimsab requested a review from a team as a code owner April 9, 2024 14:10

sarnex approved these changes Apr 9, 2024

View reviewed changes

sarnex requested a review from a team April 9, 2024 14:12

maksimsab added 2 commits April 9, 2024 07:18

Merge branch 'sycl' into sycl_split_functionality_part2

900e729

use Twine instead of std::to_string

44851de

maksimsab requested a review from jzc April 9, 2024 14:23

fix code style issue

4ba59e6

maksimsab had a problem deploying to WindowsCILock April 9, 2024 15:27 — with GitHub Actions Error

fix compile error

53168e1

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

maksimsab temporarily deployed to WindowsCILock April 9, 2024 18:12 — with GitHub Actions Inactive

add sycl-module-split in LLVM_TEST_DEPENDS

07ecdc5

maksimsab temporarily deployed to WindowsCILock April 10, 2024 12:20 — with GitHub Actions Inactive

maksimsab temporarily deployed to WindowsCILock April 10, 2024 13:21 — with GitHub Actions Inactive

asudarsa reviewed Apr 10, 2024

View reviewed changes

llvm/include/llvm/SYCLLowerIR/ModuleSplitter.h Show resolved Hide resolved

asudarsa requested changes Apr 10, 2024

View reviewed changes

asudarsa approved these changes Apr 10, 2024

View reviewed changes

jzc approved these changes Apr 11, 2024

View reviewed changes

maksimsab mentioned this pull request May 21, 2024

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

Merged

AlexeySachkov approved these changes Jun 4, 2024

View reviewed changes

Merge branch 'sycl' into sycl_split_functionality_part2

d3892d9

maksimsab temporarily deployed to WindowsCILock June 6, 2024 19:12 — with GitHub Actions Inactive

maksimsab temporarily deployed to WindowsCILock June 6, 2024 21:06 — with GitHub Actions Inactive

AlexeySachkov merged commit cad941f into intel:sycl Jun 7, 2024
13 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Move SYCL Module Splitting to library. Part 2 #13282

[SYCL] Move SYCL Module Splitting to library. Part 2 #13282

maksimsab commented Apr 4, 2024 •

edited

Loading

asudarsa commented Apr 4, 2024

maksimsab commented Apr 4, 2024

asudarsa commented Apr 4, 2024

sarnex left a comment

maksimsab commented Apr 9, 2024

asudarsa commented Apr 10, 2024

asudarsa Apr 10, 2024

maksimsab Apr 10, 2024

maksimsab commented Apr 10, 2024

asudarsa left a comment

asudarsa left a comment

asudarsa commented Apr 11, 2024

bader commented Apr 11, 2024

bader commented Apr 11, 2024

maksimsab commented Apr 12, 2024

asudarsa commented Apr 12, 2024

maksimsab commented Apr 12, 2024

AlexeySachkov left a comment

AlexeySachkov Jun 4, 2024

maksimsab Jun 6, 2024

AlexeySachkov Jun 7, 2024

asudarsa commented Jun 5, 2024

maksimsab commented Jun 7, 2024

AlexeySachkov commented Jun 7, 2024

AlexeySachkov commented Jun 7, 2024

AlexeySachkov commented Jun 7, 2024

[SYCL] Move SYCL Module Splitting to library. Part 2 #13282

[SYCL] Move SYCL Module Splitting to library. Part 2 #13282

Conversation

maksimsab commented Apr 4, 2024 • edited Loading

asudarsa commented Apr 4, 2024

maksimsab commented Apr 4, 2024

asudarsa commented Apr 4, 2024

sarnex left a comment

Choose a reason for hiding this comment

maksimsab commented Apr 9, 2024

asudarsa commented Apr 10, 2024

asudarsa Apr 10, 2024

Choose a reason for hiding this comment

maksimsab Apr 10, 2024

Choose a reason for hiding this comment

maksimsab commented Apr 10, 2024

asudarsa left a comment

Choose a reason for hiding this comment

asudarsa left a comment

Choose a reason for hiding this comment

asudarsa commented Apr 11, 2024

bader commented Apr 11, 2024

bader commented Apr 11, 2024

maksimsab commented Apr 12, 2024

asudarsa commented Apr 12, 2024

maksimsab commented Apr 12, 2024

AlexeySachkov left a comment

Choose a reason for hiding this comment

AlexeySachkov Jun 4, 2024

Choose a reason for hiding this comment

maksimsab Jun 6, 2024

Choose a reason for hiding this comment

AlexeySachkov Jun 7, 2024

Choose a reason for hiding this comment

asudarsa commented Jun 5, 2024

maksimsab commented Jun 7, 2024

AlexeySachkov commented Jun 7, 2024

AlexeySachkov commented Jun 7, 2024

AlexeySachkov commented Jun 7, 2024

maksimsab commented Apr 4, 2024 •

edited

Loading