[SYCL][ESIMD] Fix a compilation of mix of unnamed ESIMD and non ESIMD kernels #6557

fineg74 · 2022-08-10T18:27:02Z

The purpose of this PR is to fix an issue that leads to runtime exception when unnamed ESIMD and non ESIMD kernels are present.

…nels to crash

fineg74 · 2022-08-10T18:28:04Z

Complementary test PR: intel/llvm-test-suite#1143

asudarsa · 2022-08-10T21:02:02Z

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

+    return true;
+  }
+  for (const Function *F1 : Deps.successors(F)) {
+    if (isESIMDFunctionInCallChain(F1, Deps)) {


Is there a possibility to break out of this loop sooner?
Thanks

The loop itself probably not, as it needs to check all the functions to decide if they belong to ESIMD or SYCL kernels. The isESIMDFunctionInCallChain can technically limit its recursion depth and return faster.

kbobrovs · 2022-08-15T17:15:06Z

@fineg74, could you please describe what the actual problem is - why unnamed ESIMD kernels lead to this problem?

fineg74 · 2022-08-15T20:36:39Z

@fineg74, could you please describe what the actual problem is - why unnamed ESIMD kernels lead to this problem?

The immediate cause of the issue is that ESIMD kernel is wrapped with RoundedRangeKernel and sycl_explicit_esimd attribute is not propagated to the wrapper class. As a result ModuleSplitter in sycl-post-link creates a spurious split for the wrapper and later creates a spurious non-esimd module and pulls called functions into it. As a result non-esimd module would contain esimd functions and later fails to link. It looks like that for named ESIMD kernels the attributes are properly propagated and therefore the issue doesn't occur.
The fix proposed here fixes it by using a fact that ESIMD functions can only be called from ESIMD kernels so spurious split is eliminated and modules are properly created and linked. It is possible to implement it in a more formalized way by creating a pass that would propagate the attributes before performing module split but it would require significant code duplication between ModuleSplitter and the new pass.

…or ESIMD attribute.

kbobrovs · 2022-08-16T21:00:09Z

We should try to limit ESIMD specifics penetration into general-purpose tools like post-link. My suggestion would be to find the wrapper function and add 'sycl_explicit_simd' metadata to it before doing any splits in the post-link. I believe the wrapper should have spir_kernel calling convention, which automatically makes it an entry point. The task of finding the wrapper(s) is then searching for all spir_kernels up the call chain. ESIMD splitting and lowering infra will then correctly interpret the 'sycl_explicit_simd' metadata and do what's needed.

If the wrapper does not have spir_kernel (and is a spir_function), then the user ESIMD kernel must be annotated with spir_kernel. In which case the search for the wrapper would be finding terminal function in the callgraph starting from the user kernel up the call chain. Callgraph traversal up the call chain is implemented in ESIMDUtils.cpp (traverseCallgraphUp). In this case (no spir_kernel in the wrapper), we should also force the wrapper to be an entry point by copying the "sycl_module_id" attribute from the user kernel or creating a dummy one.

kbobrovs · 2022-08-16T21:03:14Z

... I would suggest to implement finding the wrapper and adding the metadata as a pass defined within the llvm/lib/SYCLLowerIR directory and invoked in sycl-post-link.cpp somewhere around (and similarly to) the const bool InvokeSimdMet = runModulePass<SYCLLowerInvokeSimdPass>(*M);

asudarsa

sycl-post-link change looks good. Thanks

kbobrovs · 2022-08-18T17:21:42Z

llvm/lib/Passes/PassRegistry.def

-                        },
-                        parseASanPassOptions,
-                        "kernel")
+MODULE_PASS_WITH_PARAMS(


Looks like most of the changes in this file are unintended reformatting. Please remove.
This command formats only modified lines (requires clang-format in the PATH):
git diff -U0 --no-color HEAD^ | clang/tools/clang-format/clang-format-diff.py -i -p1

kbobrovs · 2022-08-18T17:23:07Z

llvm/include/llvm/SYCLLowerIR/ESIMD/LowerESIMD.h

@@ -76,6 +76,12 @@ class SYCLLowerESIMDKernelPropsPass
  PreservedAnalyses run(Module &M, ModuleAnalysisManager &);
 };

+// Fixes ESIMD Kernel attributes for wrapper functions for ESIMD kernels
+class SYCLLowerESIMDKernelAttrPass


Suggested change

class SYCLLowerESIMDKernelAttrPass

class SYCLFixupESIMDKernelWrapperAttrPass

kbobrovs · 2022-08-18T17:23:21Z

llvm/include/llvm/SYCLLowerIR/ESIMD/LowerESIMD.h

@@ -76,6 +76,12 @@ class SYCLLowerESIMDKernelPropsPass
  PreservedAnalyses run(Module &M, ModuleAnalysisManager &);
 };

+// Fixes ESIMD Kernel attributes for wrapper functions for ESIMD kernels
+class SYCLLowerESIMDKernelAttrPass
+    : public PassInfoMixin<SYCLLowerESIMDKernelPropsPass> {


Suggested change

: public PassInfoMixin<SYCLLowerESIMDKernelPropsPass> {

: public PassInfoMixin<SYCLFixupESIMDKernelWrapperAttrPass> {

kbobrovs · 2022-08-18T17:26:46Z

llvm/include/llvm/SYCLLowerIR/ESIMD/ESIMDUtils.h

@@ -16,6 +16,7 @@ namespace llvm {
 namespace esimd {

 constexpr char ATTR_DOUBLE_GRF[] = "esimd-double-grf";
+constexpr char ATTR_ESIMD_KERNEL[] = "sycl_explicit_simd";


This is not an attribute, actually, but metadata. I suggest to name the same way as in
llvm/lib/SYCLLowerIR/LowerInvokeSimd.cpp:constexpr char ESIMD_MARKER_MD[] = "sycl_explicit_simd";

Suggested change

constexpr char ATTR_ESIMD_KERNEL[] = "sycl_explicit_simd";

constexpr char ESIMD_MARKER_MD[] = "sycl_explicit_simd";

kbobrovs · 2022-08-18T17:28:10Z

llvm/lib/SYCLLowerIR/CMakeLists.txt

@@ -1,4 +1,4 @@
-if(${CMAKE_VERSION} VERSION_LESS 3.14)
+if (${CMAKE_VERSION} VERSION_LESS 3.14)


please drop unneeded changes in this file, only one line change is expected, AFAIU

kbobrovs · 2022-08-18T18:00:44Z

llvm/lib/SYCLLowerIR/ESIMD/LowerESIMDKernelAttrs.cpp

+  bool Modified = false;
+  for (Function &F : M) {
+    if (llvm::esimd::isESIMD(F)) {
+      llvm::esimd::traverseCallgraphUp(&F, [&](Function *GraphNode) {


BTW, is the wrapper function a spir_kernel or spir_function? Is the user function a spir_kernel or spir_function in presence of wrapper?

Both user function and the wrapper are marked as spir_kernel .

For the record: as we talked, the user function is spir_func, which is called by two kernels with spir_kernel CC - the normal and the one with the rounded range.

kbobrovs · 2022-08-19T02:58:24Z

llvm/lib/Passes/PassRegistry.def

-MODULE_PASS("inliner-ml-advisor-release",
-            ModuleInlinerWrapperPass(getInlineParams(), true, {},
-                                     InliningAdvisorMode::Release, 0))
+MODULE_PASS("inliner-ml-advisor-release", ModuleInlinerWrapperPass(getInlineParams(), true, {}, InliningAdvisorMode::Release, 0))


still lots of unrelated changes in the file, please verify there are only intended before pushing update

kbobrovs · 2022-08-19T03:01:17Z

llvm/include/llvm/SYCLLowerIR/ESIMD/LowerESIMD.h

@@ -77,8 +77,8 @@ class SYCLLowerESIMDKernelPropsPass
 };

 // Fixes ESIMD Kernel attributes for wrapper functions for ESIMD kernels
-class SYCLLowerESIMDKernelAttrPass
-    : public PassInfoMixin<SYCLLowerESIMDKernelPropsPass> {
+class SYCLFixupESIMDKernelWrapperAttrPass


Not a good name either, my bad. As I noted, sycl_explicit_simd is a metadata. So please rename once again - SYCLFixupESIMDKernelWrapperMDPass. We need another iteration anyways.

kbobrovs · 2022-08-19T03:02:12Z

llvm/lib/Passes/PassRegistry.def

-                  ShouldNotRunFunctionPassesAnalysis())
-FUNCTION_ANALYSIS("should-run-extra-vector-passes",
-                  ShouldRunExtraVectorPasses())
+FUNCTION_ANALYSIS("should-not-run-function-passes", ShouldNotRunFunctionPassesAnalysis())


unrelated changes

asudarsa

Changes look good. Thanks.

intel#6557 rolled back necessary code, return it back with this patch. Problem somehow reproduced only in internal debug Windows build - the SYCLLowerIR/ESIMD/vec_arg_call_conv.ll test crashed opt. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

…on. (#6908) #6557 rolled back necessary code, return it back with this patch. Problem somehow reproduced only in internal debug Windows build - the SYCLLowerIR/ESIMD/vec_arg_call_conv.ll test crashed opt. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

fineg74 added 2 commits August 5, 2022 13:28

Fix an issue that causes a program containing ESIMD and non-ESIMD ker…

05453f6

…nels to crash

Address test failures

4e91855

fineg74 requested review from a team as code owners August 10, 2022 18:27

fineg74 mentioned this pull request Aug 10, 2022

[ESIMD] Introduce a test for mix of unnamed ESIMD and nonESIMD kernels compilation intel/llvm-test-suite#1143

Merged

asudarsa reviewed Aug 10, 2022

View reviewed changes

Optimize the implementation by limiting number of functions checked f…

323b955

…or ESIMD attribute.

fineg74 added 2 commits August 17, 2022 09:51

Merge remote-tracking branch 'origin/sycl' into KernelBuild

00a1f80

Reimplement the fix as a compiler pass

385d0e0

asudarsa previously approved these changes Aug 18, 2022

View reviewed changes

kbobrovs suggested changes Aug 18, 2022

View reviewed changes

Fix test failure

c2085f3

fineg74 dismissed asudarsa’s stale review via c2085f3 August 18, 2022 18:44

Address PR comments

73c200d

kbobrovs reviewed Aug 19, 2022

View reviewed changes

fineg74 added 4 commits August 19, 2022 10:10

Address PR comments

917b2bb

Remove spurious change

37ac50c

Merge remote-tracking branch 'origin/sycl' into KernelBuild

7de37c2

Fix test failure

e165679

kbobrovs approved these changes Aug 19, 2022

View reviewed changes

kbobrovs requested a review from asudarsa August 20, 2022 22:41

asudarsa approved these changes Aug 24, 2022

View reviewed changes

kbobrovs merged commit 3d54d7e into intel:sycl Aug 25, 2022

fineg74 deleted the KernelBuild branch August 25, 2022 17:22

kbobrovs mentioned this pull request Sep 28, 2022

[ESIMD] Follow-up fix for ESIMDOptimizeVecArgCallConv memory corruption. #6908

Merged

fineg74 mentioned this pull request Nov 2, 2022

[SYCL][ESIMD]Limit propagation of ESIMD attributes #7191

Merged

	class SYCLLowerESIMDKernelAttrPass
	class SYCLFixupESIMDKernelWrapperAttrPass

	: public PassInfoMixin<SYCLLowerESIMDKernelPropsPass> {
	: public PassInfoMixin<SYCLFixupESIMDKernelWrapperAttrPass> {

	constexpr char ATTR_ESIMD_KERNEL[] = "sycl_explicit_simd";
	constexpr char ESIMD_MARKER_MD[] = "sycl_explicit_simd";

		@@ -1,4 +1,4 @@
		if(${CMAKE_VERSION} VERSION_LESS 3.14)
		if (${CMAKE_VERSION} VERSION_LESS 3.14)

[SYCL][ESIMD] Fix a compilation of mix of unnamed ESIMD and non ESIMD kernels #6557

[SYCL][ESIMD] Fix a compilation of mix of unnamed ESIMD and non ESIMD kernels #6557

Uh oh!

Conversation

fineg74 commented Aug 10, 2022

Uh oh!

fineg74 commented Aug 10, 2022

Uh oh!

asudarsa Aug 10, 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!

kbobrovs commented Aug 15, 2022

Uh oh!

fineg74 commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbobrovs commented Aug 16, 2022

Uh oh!

kbobrovs commented Aug 16, 2022

Uh oh!

asudarsa 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!

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!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asudarsa Aug 10, 2022 •

edited

Loading

fineg74 commented Aug 15, 2022 •

edited

Loading