[SYCL][ESIMD] Refactor ESIMD kernel support in FE. #2747

kbobrovs · 2020-11-09T15:15:57Z

This patch adds necessary support in FE to get rid of buffer object wrapping into image1d_buffer -
workaround for ESIMD BE gaps.

Add 'esimd_acc_ptr' parameter attribute to FE.
This attribute is to be used by the FE to mark ESIMD kernel arguments
originating from buffer accessors, which is then translated to BE-specific
metadata needed for correct code generation.
Use 'esimd_acc_ptr' attribute to mark ESIMD kernel pointer
arguments originating from accessors
Use '__init_esimd' initializer in FE for ESIMD kernel accessors.
Add 'isESIMD()' function to the integration header for the RT to
be able to distinguish ESIMD kernels from normal ones for the
purpose of proper accessor arg setting.

TODO add tests for the new attrs

The other two PRs needed for this one to work are:

This attribute is to be used by the FE to mark ESIMD kernel arguments originating from buffer accessors, which is then translated to BE-specific metadata needed for correct code generation. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

- Use 'esimd_acc_ptr' attribute to mark ESIMD kernel pointer arguments originating from accessors - Use '__init_esimd' initializer in FE for ESIMD kernel accessors. - Add 'isESIMD()' function to the integration header for the RT to be able to distinguish ESIMD kernels from normal ones for the purpose of proper accessor arg setting. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

DenisBakhvalov · 2020-11-10T00:40:51Z

clang/lib/Sema/SemaSYCL.cpp

-  SyclKernelArgsSizeChecker ArgsSizeChecker(*this, Args[0]->getExprLoc());
+
+  const CXXMethodDecl *OpParens = getOperatorParens(KernelObj);
+  bool IsSIMD = (OpParens != nullptr) && OpParens->hasAttr<SYCLSimdAttr>();


nit: Those two lines are repeated multiple times. Maybe create a helper function?

Fznamznon · 2020-11-10T09:00:22Z

clang/include/clang/Basic/Attr.td

+// Not supposed to be used directly in the source - SYCL device compiler FE
+// automatically adds it for ESIMD kernels.
+def SYCLSimdAccessorPtr : InheritableAttr {
+  let Spellings = [GNU<"esimd_acc_ptr">, Declspec<"esimd_acc_ptr">];


I guess we don't need spelling if this attribute is not supposed to be used in the source.

Fznamznon · 2020-11-10T09:00:48Z

clang/include/clang/Basic/Attr.td

+// Used to mark kernel pointer parameters which correspond to the original
+// lambda's captured accessor. FE turns it to some metadata required by the
+// ESIMD Back-End.
+// Not supposed to be used directly in the source - SYCL device compiler FE
+// automatically adds it for ESIMD kernels.


I think we can move this information to the doc.

Fznamznon · 2020-11-10T09:02:09Z

clang/include/clang/Sema/Sema.h

@@ -375,6 +375,9 @@ class SYCLIntegrationHeader {

    SourceLocation KernelLocation;

+    /// Whether this kernel is an ESIMD one.
+    bool IsESIMD;


How about

Suggested change

bool IsESIMD;

bool IsESIMDKernel;

To make it clear that this flag is about kernel and not language options, for example.

Fznamznon · 2020-11-10T09:03:50Z

clang/lib/CodeGen/CodeGenModule.cpp

+
+      argESIMDAccPtrs.push_back(
+          llvm::ConstantAsMetadata::get(CGF->Builder.getInt1(
+              parm->getAttr<SYCLSimdAccessorPtrAttr>() != nullptr)));


I think it will be a bit more optimal.

Suggested change

parm->getAttr<SYCLSimdAccessorPtrAttr>() != nullptr)));

parm->hasAttr<SYCLSimdAccessorPtrAttr>())));

not sure. I want 0 or 1 is a clean input for getInt1. Non-null parm->getAttr() is neither

Sorry, I didn't get your statement. I thought that parm->getAttr<SYCLSimdAccessorPtrAttr>() != nullptr actually gives bool which is 0 or 1 as well as hasAttr function which also returns bool (https://clang.llvm.org/doxygen/classclang_1_1Decl.html#ae7a63b99398864d86f53afd8a03c359b).

Sorry, you are right. hasAttr is OK - will change

Fznamznon · 2020-11-10T09:04:52Z

clang/lib/Sema/SemaDeclAttr.cpp

+  case ParsedAttr::AT_SYCLSimdAccessorPtr:
+    handleSimpleAttribute<SYCLSimdAccessorPtrAttr>(S, D, AL);
+    break;


I guess we won't need this part if we remove spelling.

clang/lib/Sema/SemaSYCL.cpp

Co-authored-by: Mariya Podchishchaeva <[email protected]>

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

Fznamznon

Please add tests.
I would expect a codegen IR test and a test for integration header changes.

kbobrovs · 2020-11-12T14:24:12Z

Yes, tests will be added.

kbobrovs · 2020-11-14T07:09:33Z

@Fznamznon, I added 2 tests you to this PR: #2746
where I consolidated all the patches so that I can pass tests. All the patches should be committed at the same time anyway.

kbobrovs · 2020-11-17T12:09:20Z

now part of #2746

``` .../SPIRV-LLVM-Translator/lib/SPIRV/SPIRVWriter.cpp:1697:44: warning: private field 'IndexGroupArrayMap' is not used [-Wunused-private-field] 1697 | LLVMToSPIRVBase::LLVMToSPIRVMetadataMap &IndexGroupArrayMap; | ``` IndexGroupArrayMap is used but only during the constructor to validate and build other things. This change happened in 4a7804bc45ada92b8365c82b99387726382b5d7d where code was moved from a separate method, into the constructor. Original commit: KhronosGroup/SPIRV-LLVM-Translator@9a23d54079204ae

kbobrovs added 2 commits November 9, 2020 05:36

kbobrovs requested review from elizabethandrews, Fznamznon and premanandrao as code owners November 9, 2020 15:15

This was referenced Nov 9, 2020

[SYCL][ESIMD] LowerESIMD: add 'buffer_t' MD for accessor kernel args. #2748

Closed

[SYCL][ESIMD] Remove wrapping buffer objects into images. #2746

Merged

DenisBakhvalov reviewed Nov 10, 2020

View reviewed changes

Fznamznon reviewed Nov 10, 2020

View reviewed changes

kbobrovs and others added 5 commits November 10, 2020 07:18

Update clang/lib/Sema/SemaSYCL.cpp

e5ab3ef

Co-authored-by: Mariya Podchishchaeva <[email protected]>

[FE] Don't generate kernel_arg_accessor_ptr MD for non-ESIMD kernels.

f9c7ea2

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

[FE] Address review comments

b442079

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

[FE] Apply clang-format

95053d4

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

[FE] Address review comment - use hasAttr vs getAttr != nullptr

bb9c41e

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

kbobrovs force-pushed the drop_image_wrap_fe branch from 7bc4c00 to bb9c41e Compare November 12, 2020 08:05

kbobrovs requested a review from Fznamznon November 12, 2020 08:06

[FE] Fix crash in CodeGenModule.cpp

d3b1493

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

Fznamznon reviewed Nov 12, 2020

View reviewed changes

kbobrovs closed this Nov 17, 2020

kbobrovs deleted the drop_image_wrap_fe branch January 19, 2022 22:51

	parm->getAttr<SYCLSimdAccessorPtrAttr>() != nullptr)));
	parm->hasAttr<SYCLSimdAccessorPtrAttr>())));

[SYCL][ESIMD] Refactor ESIMD kernel support in FE. #2747

[SYCL][ESIMD] Refactor ESIMD kernel support in FE. #2747

Uh oh!

Conversation

kbobrovs commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

kbobrovs commented Nov 12, 2020

Uh oh!

kbobrovs commented Nov 14, 2020

Uh oh!

kbobrovs commented Nov 17, 2020

Uh oh!

Uh oh!

kbobrovs commented Nov 9, 2020 •

edited

Loading