[sycl-post-link] Add property with the default values of specialization constants #3666

vmaksimo · 2021-04-30T16:43:13Z

This patch introduces a new property which contains all the defaults for spec
constants.
It is done by setting of metadata with the default values:

; Compilation line:
; sycl-post-link -spec-const=default -S llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll -o property.table
!5 = !{i32 42}
!6 = !{%struct.ComposConst { i32 1, double 2.000000e+00, %struct.myConst { i32 13, float 0x4020666660000000 } }}

Property set looks this way:

[SYCL/specialization constants default values]
all=2|gAAAAAAAAAgKAAAA

…on constants This patch introduces a new property which contains all the defaults for spec constants. It is done by setting of metadata with the default values: ``` ; Compilation line: ; sycl-post-link -spec-const=default -S llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll -o property.table !5 = !{i32 42} !6 = !{%struct.ComposConst { i32 1, double 2.000000e+00, %struct.myConst { i32 13, float 0x4020666660000000 } }} ``` Property set looks this way: ``` [SYCL/specialization constants default values] all=2|gAAAAAAAAAgKAAAA ```

AlexeySachkov

Partial review

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

AlexeySachkov · 2021-04-30T17:14:57Z

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

+    Offset += Ty->getPrimitiveSizeInBits() / 8 +
+              (Ty->getPrimitiveSizeInBits() % 8 != 0);


Since you are appending to a vector and not inserting into a pre-allocated vector, you probably don't need this argument anymore

Offset argument is actually used for calculations in StructTy - it helps to fill the buffer with the correct alignment:
https://github.com/intel/llvm/pull/3666/files#diff-f9a582625f70ea58ef88b7713c591e8187629cca507b424557a3e4664e23848fR308-R312

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

AlexeySachkov

And the rest of the review

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

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

llvm/include/llvm/Support/PropertySetIO.h

kbobrovs · 2021-05-05T22:17:38Z

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

+/// Recursively iterates over a composite type in order to collect information
+/// about default values of its scalar elements.
+void collectCompositeElementsDefaultValuesRecursive(
+    const Module &M, Constant *C, unsigned &Offset,


NIT: processing of composite spec constants is similar in this function, emitSpecConstantRecursiveImpl and generateSpecConstDefaultValueMetadata. Could be factored out using e.g. the visitor pattern. At least a TODO for now would be good.

kbobrovs · 2021-05-05T22:27:09Z

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

+          // default value of corresponding specialization constant.
+          DefaultValue = Initializer->getAggregateElement(0u);
+        }
+      }


what GV == 0 means? please add a comment or handle

Probably we should just use cast instead of dyn_cast and always expect to see a value there.

We can meet nullptr instead of value here - this check is still needed. I left a comment above the if-.

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

kbobrovs · 2021-05-08T07:03:07Z

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

+/// Recursively iterates over a composite type in order to collect information
+/// about default values of its scalar elements.
+/// TODO: processing of composite spec constants here is similar to
+/// collectCompositeElementsInfoRecursive. Possible place for improvement -


Is it also similar to emitSpecConstantRecursiveImpl and generateSpecConstDefaultValueMetadata?

Not really, it doesn't look similar. generateSpecConstDefaultValueMetadata is not related to recursive methods, and emitSpecConstantRecursiveImpl is too different from this function.

kbobrovs

added few comments

AlexeySachkov · 2021-05-11T08:55:21Z

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

+          // default value of corresponding specialization constant.
+          DefaultValue = Initializer->getAggregateElement(0u);
+        }
+      }


Probably we should just use cast instead of dyn_cast and always expect to see a value there.

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

alexbatashev

RT changes look good

bader · 2021-05-13T12:16:01Z

@kbobrovs, ping.

…binaries DefaultValues.DefaultValuesAreSet and DefaultValues.DefaultValuesAreOverriden tests only support JIT which is not the case for the default values. Once AOT binaries support is added, the tests can be enabled back.

AlexeySachkov · 2021-05-24T14:47:21Z

sycl/source/detail/device_image_impl.hpp

@@ -256,7 +256,7 @@ class device_image_impl {
          // supposed to be called from c'tor.
          MSpecConstSymMap[std::string{SCName}].push_back(
              SpecConstDescT{/*ID*/ It[0], /*CompositeOffset*/ It[1],
-                             /*Size*/ It[2], BlobOffset, HasDefaultValues});
+                             /*Size*/ It[2], BlobOffset});


I'm a bit concerned that this change is being committed in scope of this PR. The PR title says that this is a modification of sycl-post-link tool and all 13 commits are very likely to be squashed into a single one during the merge - we will end up with a commit to sycl-post-link, which modifies DPC++ RT.

Moreover, it is not clear to me why this change is needed at all. From my point of view it is better if we submit it in a separate PR

Tagging @dm-vodopyanov here

This change is related to https://github.com/intel/llvm/pull/3667/files#r631154823. I'll create a separate PR to make it clear.

llvm/include/llvm/Support/PropertySetIO.h

kbobrovs · 2021-05-24T19:16:03Z

llvm/include/llvm/Support/PropertySetIO.h

@@ -201,11 +201,21 @@ class PropertySetRegistry {
      PropSet.insert(std::make_pair(Prop.first, PropertyValue(Prop.second)));
  }

+  template <typename T>


Please add a comment when this function should be used instead of void add(StringRef Category, const std::map<StringRef, T> &Props) above. I.e. when the order of properties matters.

Suggest to make add templated on the map type to have single implementation for both std::map<StringRef, T> and std::map<StringRef, T>

Remove backward compatibility according to discussion: https://github.com/intel/llvm/pull/3667/files#r631154823i This patch is expected to be merged together or before #3666

Moving to intel#3813

vmaksimo added 2 commits April 30, 2021 19:40

Support floats and composite types

45db4f1

vmaksimo requested a review from AlexeySachkov April 30, 2021 16:43

AlexeySachkov reviewed Apr 30, 2021

View reviewed changes

vmaksimo added 4 commits May 5, 2021 18:18

Apply suggestions from code review

eb594cf

Minor line changes

c632c64

Merge branch 'sycl' into spec_const_property_pub

41dc658

Do no generate metadata if default value ptr is null

4ca0b76

vmaksimo marked this pull request as ready for review May 5, 2021 16:48

vmaksimo requested review from kbobrovs, mlychkov, smaslov-intel and a team as code owners May 5, 2021 16:48

kbobrovs reviewed May 5, 2021

View reviewed changes

Minor changes after CR

d6fa1f9

vmaksimo requested review from kbobrovs and AlexeySachkov May 7, 2021 17:06

kbobrovs reviewed May 8, 2021

View reviewed changes

AlexeySachkov reviewed May 11, 2021

View reviewed changes

Apply comments

08de0d4

AlexeySachkov previously approved these changes May 11, 2021

View reviewed changes

remove changes from SYCL header pi.h

6d25843

vmaksimo dismissed AlexeySachkov’s stale review via 6d25843 May 11, 2021 10:55

alexbatashev previously approved these changes May 11, 2021

View reviewed changes

AlexeySachkov previously approved these changes May 11, 2021

View reviewed changes

AlexeySachkov removed the request for review from smaslov-intel May 11, 2021 16:57

bader requested a review from kbobrovs May 11, 2021 17:01

alexbatashev mentioned this pull request May 12, 2021

[SYCL] Add DPC++ RT support for SYCL2020 spec constants' default values #3667

Merged

kbobrovs previously approved these changes May 17, 2021

View reviewed changes

Merge branch 'sycl' into spec_const_property_defaults

a5cbe74

vmaksimo dismissed stale reviews from kbobrovs, AlexeySachkov, and alexbatashev via a5cbe74 May 18, 2021 09:11

AlexeySachkov previously approved these changes May 18, 2021

View reviewed changes

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

19c8857

vmaksimo dismissed AlexeySachkov’s stale review via 05ead09 May 21, 2021 12:04

vmaksimo added 2 commits May 24, 2021 14:32

Fix test failure for non-native spec consts

774051b

vmaksimo force-pushed the spec_const_property_defaults branch from 57f8fbd to 02293fb Compare May 24, 2021 11:32

AlexeySachkov reviewed May 24, 2021

View reviewed changes

kbobrovs reviewed May 24, 2021

View reviewed changes

vmaksimo mentioned this pull request May 25, 2021

[SYCL] Avoid HasDefaultValues to be set as IsSet field of SpecConstDescT #3812

Closed

Minor improvement

c4e538a

vmaksimo mentioned this pull request May 25, 2021

[SYCL] Avoid HasDefaultValues to be set as IsSet field of SpecConstDescT #3813

Merged

Remove RT changes from this PR

9b0305b

Moving to intel#3813

kbobrovs previously approved these changes May 25, 2021

View reviewed changes

Fix typo

c3c7989

vmaksimo dismissed kbobrovs’s stale review via c3c7989 May 26, 2021 09:15

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

df0d057

vmaksimo requested a review from kbobrovs May 26, 2021 16:50

AlexeySachkov approved these changes May 27, 2021

View reviewed changes

alexbatashev approved these changes May 27, 2021

View reviewed changes

romanovvlad merged commit 7d5ee05 into intel:sycl May 27, 2021

		Offset += Ty->getPrimitiveSizeInBits() / 8 +
		(Ty->getPrimitiveSizeInBits() % 8 != 0);

[sycl-post-link] Add property with the default values of specialization constants #3666

[sycl-post-link] Add property with the default values of specialization constants #3666

Uh oh!

Conversation

vmaksimo commented Apr 30, 2021

Uh oh!

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexbatashev left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented May 13, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!