Add support for composite specialization constants #2779

AlexeySachkov · 2020-11-16T18:10:57Z

Design document can be found in https://github.com/intel/llvm/blob/sycl/sycl/doc/SpecializationConstants.md, also this PR is build upon #2669

Two more changes would be needed to enable the whole feature:

support for composite spec constants in the SPIR-V translator
support for composite spec constants in the SYCL runtime

This refactoring is needed to simplify upcoming functional changes. Outlined some code into a helper function Removed `getDefaultValue` function: it is unclear why default value of spec constant would be different for AOT and non-AOT flows. Removed unused SymGlob argument of getStringLiteralArg. Removed unneeded static keyword. Added `const` to a few arguments of different helpers within the pass.

New category is going to be used for storing information about composite specialization constants.

kbobrovs

LGTM - no more comments from my side after internal review. One small nit.

llvm/test/tools/sycl-post-link/composite-spec-constant-default-value.ll

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

Fznamznon · 2020-11-23T15:08:41Z

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

+      if (Callee->getName().contains(SPIRV_GET_SPEC_CONST_COMPOSITE)) {
+        auto Res = getCompositeSpecConstMetadata(CI);
+        if (!Res.first.empty()) {
+          CompositeIDMap[Res.first] = Res.second;
+          Met = true;
+        }
+      } else if (Callee->getName().contains(SPIRV_GET_SPEC_CONST_VAL)) {
+        auto Res = getScalarSpecConstMetadata(CI);
+        if (!Res.first.empty()) {
+          ScalarIDMap[Res.first] = Res.second;
+          Met = true;
+        }


These parts look like a copy-paste with small changes, although I didn't come up with a good suggestion to reduce repetitions.

It seems to me that there is only ~3 similar lines here and this code snippet is only encountered once, so I don't think that this is a huge problem

3/5 similar lines.

Fznamznon · 2020-11-23T15:10:01Z

llvm/tools/sycl-post-link/SpecConstants.h

+  // "spec constant name" -> "spec constant int ID" map for scalar spec
+  // constants and
+  // "spec constant name" -> vector<"spec constant int ID"> map for composite
+  // spec constants


What is the reason for two different maps for scalars and composites?

Because structures representing scalar and composite spec constants are different, i.e. for scalar spec constant we only need to have a mapping from string to int, while for composite spec constants we need to have a mapping from string to vector.

It is easier to maintain two maps because those maps are stored into separate properties of device image

Can we add mapping for scalar spec constants from string to vector with one element?

It is easier to maintain two maps because those maps are stored into separate properties of device image

Why it is required to store separate properties for scalar and composite spec constants?

Can we add mapping for scalar spec constants from string to vector with one element?

Technically this is double, but there is a significant downside which I will describe below:

Why it is required to store separate properties for scalar and composite spec constants?

It is easier and probably more clean and reliable way of differentiating them in runtime: instead of checking for the size of vector I just scan two different properties in order to handle scalar and composite spec constants and their handling is different there

It is easier and probably more clean and reliable way of differentiating them in runtime: instead of checking for the size of vector I just scan two different properties in order to handle scalar and composite spec constants and their handling is different there

I guess their handling only is different because you need to set several values of spec constants (I mean do several calls of smth like clSetSpecConst or how it is called, I don't know exactly) in case of composite spec constant and only one in case of scalar spec constant. I believe if we save scalar constants as vector of one element and handle it in the same way as it was scalar spec constant, you won't need to differentiate them in RT and sycl-post-link. Although this is probably a big change across two components and can be done in the future as some kind of refactoring.

I believe if we save scalar constants as vector of one element and handle it in the same way as it was scalar spec constant, you won't need to differentiate them in RT and sycl-post-link.

This is a good point, actually, thanks!

Although this is probably a big change across two components and can be done in the future as some kind of refactoring.

Yes, we would also need to update our design document to unify representation/handling of scalar and composite spec constants. I would like to hear feedback from @kbobrovs about this before proceeding with this refactoring

This makes sense to me. But I'm assuming SPIRV and __sycl intrinsics will remain separate.

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

AlexeySotkin · 2020-11-24T09:49:56Z

llvm/test/tools/sycl-post-link/composite-spec-constant.ll

+; composite specialization constants by lowering them into a set of SPIR-V
+; friendly IR operations representing those constants.
+;
+; CHECK: %[[#NS0:]] = call i32 @_Z20__spirv_SpecConstantii(i32 [[#ID:]], i32


What is value of ID? Is it 0?

It is not necessary 0, because if we have a composite spec constant, then the first ID will be dedicated to the composite itself and only the second available ID will be dedicated to its first member.

I'm using FileCheck numeric substitutions here because numbers are more or less consecutive

It is not necessary 0, because if we have a composite spec constant, then the first ID will be dedicated to the composite itself and only the second available ID will be dedicated to its first member.

I updated algorithm in 9a82d53, so no IDs are being reserved for composites anymore, but it is still not necessary is zero in general, because it depends on the fact whether some other specialization constants were encountered earlier.

AlexeySotkin · 2020-11-24T09:54:17Z

llvm/test/tools/sycl-post-link/composite-spec-constant.ll

+; CHECK: %[[#NS1:]] = call float @_Z20__spirv_SpecConstantif(i32 [[#ID + 1]], float
+; CHECK: %[[#NA0:]] = call %struct._ZTS1A.A @_Z29__spirv_SpecConstantCompositeif(i32 %[[#NS0]], float %[[#NS1]])
+;
+; CHECK: %[[#NS2:]] = call i32 @_Z20__spirv_SpecConstantii(i32 [[#ID + 3]], i32
+; CHECK: %[[#NS3:]] = call float @_Z20__spirv_SpecConstantif(i32 [[#ID + 4]], float
+; CHECK: %[[#NA1:]] = call %struct._ZTS1A.A @_Z29__spirv_SpecConstantCompositeif(i32 %[[#NS2]], float %[[#NS3]])
+;
+; CHECK: %[[#NA:]] = call [2 x %struct._ZTS1A.A] @_Z29__spirv_SpecConstantCompositestruct._ZTS1A.Astruct._ZTS1A.A(%struct._ZTS1A.A %[[#NA0]], %struct._ZTS1A.A %[[#NA1]])
+;
+; CHECK: %[[#B0:]] = call i32 @_Z20__spirv_SpecConstantii(i32 [[#ID + 7]], i32{{.*}})
+; CHECK: %[[#B1:]] = call i32 @_Z20__spirv_SpecConstantii(i32 [[#ID + 8]], i32{{.*}})


Why ID + 2, ID + 5, ID + 6 are missing? Can we make ids consecutive?

Why ID + 2, ID + 5, ID + 6 are missing?

I allocate a separate ID for each composite encountered

Can we make ids consecutive?

Theoretically, yes, because composites don't have IDs in SPIR-V

I made IDs consecutive in 9a82d53

Also updated enumeration algorithm, so IDs are not reserved for __spirv_SpecConstantComposite entries anymore.

Fznamznon

I don't have another comments except we could make it lot simpler if we handle composites and scalars in the same way.

AlexeySachkov · 2020-11-27T08:48:48Z

I don't have another comments except we could make it lot simpler if we handle composites and scalars in the same way.

It will mostly simplify runtime part, not SpecConstants pass

kbobrovs

LGTM.

kbobrovs · 2020-11-27T17:21:44Z

llvm/tools/sycl-post-link/SpecConstants.h

+  // "spec constant name" -> "spec constant int ID" map for scalar spec
+  // constants and
+  // "spec constant name" -> vector<"spec constant int ID"> map for composite
+  // spec constants


This makes sense to me. But I'm assuming SPIRV and __sycl intrinsics will remain separate.

AlexeySachkov · 2020-12-01T14:54:40Z

@bader, could you please take a look/approve as a code owner?

Both scalar and composite spec constants are not communicated between the device compiler and runtime in a single propery set using unified format (the same as previously used for composite spec constants). This change was suggested on code review here: intel#2779 (comment)

Add support for translating `OpCopyMemory` into `llvm.memcpy`. Fixes KhronosGroup/SPIRV-LLVM-Translator#2769 Original commit: KhronosGroup/SPIRV-LLVM-Translator@8dc0349c0860172

AlexeySachkov requested a review from kbobrovs November 16, 2020 18:10

AlexeySachkov requested a review from bader as a code owner November 16, 2020 18:10

AlexeySachkov added 2 commits November 18, 2020 11:32

[SYCL][NFC] Register new property category

1c1c4c3

New category is going to be used for storing information about composite specialization constants.

[SYCL] Add support for composites in SpecConstantsPass

09fb54a

AlexeySachkov force-pushed the private/asachkov/support-for-pod-spec-constants-in-tools branch from 3a88a4f to 09fb54a Compare November 18, 2020 08:38

AlexeySachkov requested a review from Fznamznon November 19, 2020 09:08

AlexeySachkov mentioned this pull request Nov 19, 2020

[SYCL RT] Add support for composite specialization constants #2797

Merged

kbobrovs previously approved these changes Nov 23, 2020

View reviewed changes

llvm/test/tools/sycl-post-link/composite-spec-constant-default-value.ll Show resolved Hide resolved

Fznamznon reviewed Nov 23, 2020

View reviewed changes

AlexeySotkin reviewed Nov 24, 2020

View reviewed changes

AlexeySachkov added 3 commits November 24, 2020 18:02

Apply comments

4c9ce32

Fix handling of multiple usage of composite spec constant

9a82d53

Also updated enumeration algorithm, so IDs are not reserved for __spirv_SpecConstantComposite entries anymore.

Add test with optimizations disabled

9de55f1

AlexeySachkov dismissed kbobrovs’s stale review via 9de55f1 November 26, 2020 15:57

AlexeySachkov requested review from kbobrovs, Fznamznon and AlexeySotkin November 27, 2020 08:18

Fznamznon approved these changes Nov 27, 2020

View reviewed changes

kbobrovs approved these changes Nov 27, 2020

View reviewed changes

AlexeySachkov changed the title ~~Add support for POD types in specialization constants~~ Add support for composite specialization constants Dec 1, 2020

bader merged commit e481174 into intel:sycl Dec 1, 2020

AlexeySachkov deleted the private/asachkov/support-for-pod-spec-constants-in-tools branch February 25, 2021 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for composite specialization constants #2779

Add support for composite specialization constants #2779

AlexeySachkov commented Nov 16, 2020

kbobrovs left a comment

Fznamznon Nov 23, 2020

AlexeySachkov Nov 24, 2020

Fznamznon Nov 27, 2020

Fznamznon Nov 23, 2020

AlexeySachkov Nov 24, 2020

Fznamznon Nov 24, 2020

AlexeySachkov Nov 24, 2020

Fznamznon Nov 27, 2020

AlexeySachkov Nov 27, 2020

kbobrovs Nov 27, 2020

AlexeySotkin Nov 24, 2020

AlexeySachkov Nov 24, 2020

AlexeySachkov Nov 26, 2020

AlexeySotkin Nov 24, 2020

AlexeySachkov Nov 24, 2020

AlexeySachkov Nov 26, 2020

Fznamznon left a comment

AlexeySachkov commented Nov 27, 2020

kbobrovs left a comment

kbobrovs Nov 27, 2020

AlexeySachkov commented Dec 1, 2020

Add support for composite specialization constants #2779

Add support for composite specialization constants #2779

Conversation

AlexeySachkov commented Nov 16, 2020

kbobrovs 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

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

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

Fznamznon left a comment

Choose a reason for hiding this comment

AlexeySachkov commented Nov 27, 2020

kbobrovs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexeySachkov commented Dec 1, 2020