[SYCL] Specialization constants: fix scope, allow redefinition and AOT. #1633

kbobrovs · 2020-05-02T03:22:41Z

Specialization constants are now per-program, as the implemented spec
requires.
Program's specialization constant set is added to the program
compilation cache key, it gets recompiled on new specialization
constant combination.

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

kbobrovs · 2020-05-04T18:27:19Z

Link to the spec proposal:
https://github.com/codeplaysoftware/standards-proposals/blob/master/spec-constant/index.md

- Specialization constants are now per-program, as the implemented spec requires. - Program's specialization constant set is added to the program compilation cache key, it gets recompiled on new specialization constant combination. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

…x it. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

romanovvlad · 2020-05-06T17:33:37Z

sycl/source/detail/program_impl.cpp

+
+  auto LockGuard = Ctx->getKernelProgramCache().acquireCachedPrograms();
+
+  for (SCItTy SCIt = SCRange.begin(); SCIt != SCRange.end(); SCIt++) {


Suggested change

for (SCItTy SCIt = SCRange.begin(); SCIt != SCRange.end(); SCIt++) {

for (SCItTy &SCIt: SCRange) {

OK, but I'll avoid reference to iterator

romanovvlad · 2020-05-07T06:22:27Z

sycl/test/spec_const/spec_const_redefine.cpp

+// RUN: env SYCL_PI_TRACE=2 %CPU_RUN_PLACEHOLDER %t.out 2>&1 %CPU_CHECK_PLACEHOLDER
+// RUN: env SYCL_PI_TRACE=2 %GPU_RUN_PLACEHOLDER %t.out 2>&1 %GPU_CHECK_PLACEHOLDER
+// RUN: env SYCL_PI_TRACE=2 %ACC_RUN_PLACEHOLDER %t.out 2>&1 %ACC_CHECK_PLACEHOLDER
+// TODO the test currently fails on devices these devices:


Suggested change

// TODO the test currently fails on devices these devices:

// TODO the test currently fails on these configurations:

right, I think I've pushed a fix already

sycl/source/detail/program_manager/program_manager.hpp

romanovvlad · 2020-05-07T06:40:38Z

sycl/include/CL/sycl/detail/spec_constant_impl.hpp

 class spec_constant_impl {
 public:
-  spec_constant_impl(unsigned int ID) : ID(ID), Size(0), Bytes{0} {}
+  spec_constant_impl() = default;


Please, initialize member to some default value(0).

sycl/source/detail/program_impl.cpp

romanovvlad · 2020-05-07T07:08:07Z

sycl/source/detail/program_manager/program_manager.cpp

+        if (DbgProgMgr > 0)
+          std::cerr << ">>> WARNING: flushSpecConstants requested on a "
+                       "program w/o known binary image\n";
+        return; // program origin is unknown


I think we should throw exception here.

romanovvlad · 2020-05-07T07:09:21Z

sycl/source/detail/program_manager/program_manager.cpp

+      if (DbgProgMgr > 0)
+        std::cerr << ">>> ProgramManager::flushSpecConstants: binary image "
+                  << &Img->getRawData() << " doesn't support spec constants\n";
+      // this device binary image does not support runtime setting of


Do we throw exception somewhere if user sets spec constant for an image which doesn't support this feature?

No. But exception can't be thrown here - the implemented spec has some gaps here.
What can happen in practice is AOT compilation, after which spec constants can't be set. On the other hand, we don't want to throw or change the program depending on JIT vs AOT scenario. Defaults will be used instead.

added a comment

So, there will be no diagnostic if a user uses AOT compiled binary but sets specialization constant. Is there any plan to address this problem?

romanovvlad · 2020-05-07T07:12:49Z

sycl/source/detail/program_impl.cpp

  SC.set(ValSize, ValAddr);
 }

+void program_impl::flush_spec_constants(const RTDeviceBinaryImage &Img,


Suggest moving functionality of this function to program_manager.cpp:991 to avoid dealing with RTDeviceBeinaryImage outside of program_manager.

What is the problem with dealing with RTDeviceBeinaryImage outside of program_manager?
Or do you mean incurred dependence on program_manager.h everywhere RTDeviceBeinaryImage is used? That can be fixed by extracting BinaryImage infra into a separate header.

I think it would be nice if program_manager abstracts away low level image details from other parts of the SYCL.
Is there any reason for this function to be a method of program class?
This comment doesn't block PR approval.

Spec constants are now bound to a program_impl, this is the main reason. But I agree that it is better keep as few inter-dependencies as possible. Anyway, upcoming modules implementation will require significant refactoring in this area - let's revisit this design question as a part of modules design, not to refactor twice. Do you agree?

s-kanaev · 2020-05-07T12:09:20Z

sycl/source/detail/program_manager/program_manager.cpp

+  return getOrBuild<PiProgramT, compile_program_error>(
+      Cache, KeyT(std::move(SpecConsts), KSId), AcquireF, GetF, BuildF);


Should these programs (i.e. with specialization consts) be cached at all?

Yes. Why it should not?

s-kanaev · 2020-05-07T15:11:23Z

sycl/source/detail/program_manager/program_manager.cpp

@@ -184,7 +184,7 @@ RetT *waitUntilBuilt(KernelProgramCache &Cache,
 ///         cache. Accepts nothing. Return pointer to built entity.
 template <typename RetT, typename ExceptionT, typename KeyT, typename AcquireFT,
          typename GetCacheFT, typename BuildFT>
-RetT *getOrBuild(KernelProgramCache &KPCache, const KeyT &CacheKey,
+RetT *getOrBuild(KernelProgramCache &KPCache, KeyT &&CacheKey,


Why do you need universal reference for CacheKey here?

to avoid copying of the CacheKey when working with the cache map, as the key now includes the spec constant set.

Co-authored-by: Romanov Vlad <[email protected]>

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

romanovvlad

Ok to me.

kbobrovs requested a review from a team as a code owner May 2, 2020 03:22

kbobrovs requested review from v-klochkov, romanovvlad and AlexeySachkov May 2, 2020 03:22

kbobrovs force-pushed the spec-const-redefine branch from d5133e3 to ed05d8e Compare May 6, 2020 00:03

[SQUASH] Exculde invalid exec paths leading to flushSpecConstants, fi…

cba353c

…x it. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

kbobrovs force-pushed the spec-const-redefine branch from ed05d8e to cba353c Compare May 6, 2020 00:22

[SQUASH] Disable CPU SC redefine test due to old CI CPU RT.

d0a6846

romanovvlad requested changes May 7, 2020

View reviewed changes

romanovvlad requested a review from s-kanaev May 7, 2020 10:39

s-kanaev reviewed May 7, 2020

View reviewed changes

kbobrovs and others added 3 commits May 7, 2020 11:04

Update sycl/source/detail/program_manager/program_manager.hpp

aed25f2

Co-authored-by: Romanov Vlad <[email protected]>

Update sycl/source/detail/program_impl.cpp

bc7001d

Co-authored-by: Romanov Vlad <[email protected]>

Address review comments. Disable tests properly.

11df804

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

kbobrovs force-pushed the spec-const-redefine branch from 59f389f to 11df804 Compare May 7, 2020 21:42

kbobrovs requested review from romanovvlad and s-kanaev May 7, 2020 22:20

romanovvlad approved these changes May 8, 2020

View reviewed changes

pvchupin merged commit c22e34b into intel:sycl May 8, 2020

kbobrovs deleted the spec-const-redefine branch July 30, 2020 12:31


		auto LockGuard = Ctx->getKernelProgramCache().acquireCachedPrograms();

		for (SCItTy SCIt = SCRange.begin(); SCIt != SCRange.end(); SCIt++) {

	for (SCItTy SCIt = SCRange.begin(); SCIt != SCRange.end(); SCIt++) {
	for (SCItTy &SCIt: SCRange) {

	// TODO the test currently fails on devices these devices:
	// TODO the test currently fails on these configurations:

		return getOrBuild<PiProgramT, compile_program_error>(
		Cache, KeyT(std::move(SpecConsts), KSId), AcquireF, GetF, BuildF);

[SYCL] Specialization constants: fix scope, allow redefinition and AOT. #1633

[SYCL] Specialization constants: fix scope, allow redefinition and AOT. #1633

Uh oh!

Conversation

kbobrovs commented May 2, 2020

Uh oh!

kbobrovs commented May 4, 2020

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!

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!

Choose a reason for hiding this comment

Uh oh!

romanovvlad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!