[SYCL][Fusion] API for kernel fusion library #7465

sommerlukas · 2022-11-21T12:59:08Z

This is the second patch in a series of patches to add an implementation of thekernel fusion extension. We have split the implementation into multiple patches to make them more easy to review.

This patch can be reviewed and merged independently of #7416.

This patch adds the first components for the JIT compiler used for implementation of kernel fusion at runtime , concretely:

API definitions
Input translation from SPIR-V to LLVM IR
Insertion of fused kernel function stub and metadata
Supporting infrastructure such as compiler options.

CMake logic to link and code to invoke this JIT from the SYCL runtime will follow in a later patch.

Co-authored-by: Lukas Sommer [email protected]
Co-authored-by: Victor Perez [email protected]
Signed-off-by: Lukas Sommer [email protected]

sommerlukas · 2022-11-21T13:05:29Z

The design document for the overall implementation of kernel fusion can be found here.

Naghasan · 2022-11-29T12:23:45Z

@bader @pvchupin Can anyone help with the review here ?

pvchupin · 2022-11-29T17:16:13Z

Since this is a new library we may need to determine who would be the best reviewers going forward for these. I'm going to add a few people.
@sommerlukas, can you please also update CODEOWNERS with this change (adding Codeplay folks)?

steffenlarsen

Just a couple initial comments. I think I'll need a good cup of coffee for this one, so I will return in a couple of days.

steffenlarsen · 2022-11-24T13:47:46Z

sycl-fusion/common/include/Kernel.h

+///
+/// Enumerate possible kinds of parameters.
+/// 1:1 correspondence with the definition in kernel_desc.hpp in the DPC++ SYCL
+/// runtime.


Does this need to stay in sync with the definition in kernel_desc?

For now, only Accessor (for internalization) and StdLayout (for constant propagation) receive special treatment by the JIT compilation process. The interface logic to this library in the SYCL runtime (part of a later PR) uses the SYCL RT internal ArgDesc directly, so additional members for this enum in kernel_desc would not cause errors in the JIT.

sycl-fusion/common/include/Kernel.h

AlexeySachkov

Just a brief review, I haven't been able to grasp it in details yet

sycl-fusion/README.md

sycl-fusion/CMakeLists.txt

sycl-fusion/common/include/Kernel.h

sycl-fusion/jit-compiler/lib/translation/SPIRVLLVMTranslation.cpp

AlexeySachkov · 2022-11-29T22:41:17Z

sycl-fusion/jit-compiler/lib/translation/SPIRVLLVMTranslation.cpp

+  if (KernelFunction->hasMetadata(REQD_WORK_GROUP_SIZE_ATTR)) {
+    auto *MD = KernelFunction->getMetadata(REQD_WORK_GROUP_SIZE_ATTR);
+    SYCLKernelAttribute ReqdAttr{REQD_WORK_GROUP_SIZE_ATTR};
+    getAttributeValues(ReqdAttr.Values, MD, 3);


Please note that there is an ongoing effort to allow less than 3 operands in reqd_work_group_size metadata, see #7450 (@steffenlarsen can link PRs to other components)

Thanks for pointing this out, I wasn't aware of that. We should be able to adapt our processing of the attributes once that PR is merged, to match the new behavior.

I think for this particular case KhronosGroup/SPIRV-LLVM-Translator#1726 is also relevant. Note that in the reverse-translator case you will always get 3 operands back, but I believe you also care about generated LLVM IR that hasn't gone through the translator, in which case you may need to adjust for optional parameters.

Thanks for the hint!

Right now, we only care about consuming SPIR-V. Untranslated LLVM IR will become relevant for targets that the DPC++ frontend currently does not compile to SPIR-V, e.g., targeting CUDA or AMD GPUs. Will keep this in mind for when we work on that.

sycl-fusion/jit-compiler/lib/translation/SPIRVLLVMTranslation.cpp

First components for the JIT compiler used for SYCL kernel fusion: * API definitions * Input translation from SPIR-V to LLVM IR * Insertion of fused kernel function stub and metadata * Supporting infrastructure such as compiler options. Co-authored-by: Lukas Sommer <[email protected]> Co-authored-by: Victor Perez <[email protected]> Signed-off-by: Lukas Sommer <[email protected]>

sommerlukas · 2022-11-30T14:24:29Z

Thank you for the first reviews @steffenlarsen and @AlexeySachkov!

Since this is a new library we may need to determine who would be the best reviewers going forward for these. I'm going to add a few people. @sommerlukas, can you please also update CODEOWNERS with this change (adding Codeplay folks)?

@pvchupin: I've added the Codeplay team that has been working on this implementation/library to CODEOWNERS. Let me know if I should add more people/teams.

steffenlarsen

Just a couple more comments, but I think it looks good overall.

Note that the comment about the use of auto should probably also apply to a few other files. It doesn't have to be all uses of auto, for example X::create should be clear enough about the type being X, but for those that aren't necessarily clear about exactly what type it uses would be nice to have explicit.

Also, what are the plans for testing this? I realize much of this is just the skeleton with more to come, but is there any of this that might make sense to unit-test now or does it make more sense to wait?

sycl-fusion/common/include/Kernel.h

sycl-fusion/jit-compiler/lib/KernelFusion.cpp

steffenlarsen · 2022-12-02T12:49:15Z

sycl-fusion/jit-compiler/lib/translation/SPIRVLLVMTranslation.cpp

+  if (KernelFunction->hasMetadata(REQD_WORK_GROUP_SIZE_ATTR)) {
+    auto *MD = KernelFunction->getMetadata(REQD_WORK_GROUP_SIZE_ATTR);
+    SYCLKernelAttribute ReqdAttr{REQD_WORK_GROUP_SIZE_ATTR};
+    getAttributeValues(ReqdAttr.Values, MD, 3);


I think for this particular case KhronosGroup/SPIRV-LLVM-Translator#1726 is also relevant. Note that in the reverse-translator case you will always get 3 operands back, but I believe you also care about generated LLVM IR that hasn't gone through the translator, in which case you may need to adjust for optional parameters.

Signed-off-by: Lukas Sommer <[email protected]>

sommerlukas · 2022-12-05T09:40:32Z

Thanks again for the feedback @steffenlarsen.

Also, what are the plans for testing this? I realize much of this is just the skeleton with more to come, but is there any of this that might make sense to unit-test now or does it make more sense to wait?

The sycl-fusion library will be tested in two ways:

Through llvm-lit based unit tests for the different passes that are part of the library (but not yet part of this PR). These tests also test the metadata format for information about the fusion, identical parameters etc., and the whole SYCLKernelInfo/SYCLModuleInfo infrastructure in Kernel.h.
Through SYCL application tests as part of llvm-test-suite, which test the whole fusion process as driven by the SYCL runtime. These tests are not yet part of this PR, as they require the logic connecting SYCL runtime and the sycl-fusion library to be upstreamed first (future PR).

steffenlarsen

Thanks, @sommerlukas! LGTM!

sommerlukas · 2022-12-05T18:24:42Z

Thanks for the approvals!

Can either of you please merge this PR, I'm not authorized to merge this, @steffenlarsen @pvchupin?

This is the third patch in a series of patches to add an implementation of the [kernel fusion extension](#7098). We have split the implementation into multiple patches to make them more easy to review. This patch integrates the kernel fusion extension into the SYCL runtime scheduler. Next to collecting the kernels submitted while in fusion mode in the fusion list associated with the queue, the integration into the scheduler is also responsible for detecting the synchronization scenarios. Various scenarios, such as buffer destruction or event wait, require fusion to be aborted early. The full list of scenarios is available in the [extension proposal](https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_codeplay_kernel_fusion.asciidoc#synchronization-in-the-sycl-application). A high-level description of the integration into the scheduler can be found in the [design document](#7204). This PR can be reviewed and merged independently of #7465. Signed-off-by: Lukas Sommer <[email protected]> Signed-off-by: Lukas Sommer <[email protected]>

aelovikov-intel · 2023-02-03T18:25:43Z

sycl-fusion/jit-compiler/include/JITContext.h

+  SPIRVBinary &emplaceSPIRVBinary(std::string Binary);
+
+private:
+  // FIXME: Change this to std::shared_mutex after switching to C++17.


We have switched to C++17 as a minimal required version mid 2022.

sommerlukas requested a review from pvchupin as a code owner November 21, 2022 12:59

sommerlukas force-pushed the kernel-fusion/second-patch branch from be9e0fa to 0ff42c4 Compare November 21, 2022 13:04

sommerlukas requested a review from bader November 21, 2022 13:04

sommerlukas mentioned this pull request Nov 24, 2022

[SYCL][Fusion] Scheduler support for kernel fusion #7531

Merged

sommerlukas force-pushed the kernel-fusion/second-patch branch 2 times, most recently from deebaf4 to 523c851 Compare November 25, 2022 09:20

sommerlukas self-assigned this Nov 28, 2022

Naghasan requested a review from steffenlarsen November 29, 2022 15:23

pvchupin requested review from smaslov-intel, xtian-github, AlexeySachkov, gmlueck and jbrodman November 29, 2022 17:17

steffenlarsen reviewed Nov 29, 2022

View reviewed changes

AlexeySachkov reviewed Nov 29, 2022

View reviewed changes

sommerlukas force-pushed the kernel-fusion/second-patch branch from 523c851 to 962c922 Compare November 30, 2022 14:10

sommerlukas requested review from AlexeySachkov and steffenlarsen November 30, 2022 14:24

steffenlarsen reviewed Dec 2, 2022

View reviewed changes

[SYCL][Fusion] Reduce usage of "auto" in JIT

2b34c65

Signed-off-by: Lukas Sommer <[email protected]>

steffenlarsen approved these changes Dec 5, 2022

View reviewed changes

pvchupin approved these changes Dec 5, 2022

View reviewed changes

pvchupin merged commit 441bffe into intel:sycl Dec 5, 2022

aelovikov-intel reviewed Feb 3, 2023

View reviewed changes

[SYCL][Fusion] API for kernel fusion library #7465

[SYCL][Fusion] API for kernel fusion library #7465

Uh oh!

Conversation

sommerlukas commented Nov 21, 2022

Uh oh!

sommerlukas commented Nov 21, 2022

Uh oh!

Naghasan commented Nov 29, 2022

Uh oh!

pvchupin commented Nov 29, 2022

Uh oh!

steffenlarsen left a comment

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!

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!

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!

sommerlukas commented Nov 30, 2022

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sommerlukas commented Dec 5, 2022

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

sommerlukas commented Dec 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!