[SYCL] Add DPC++ RT support for non-native SYCL 2020 spec constants #3589

dm-vodopyanov · 2021-04-21T14:03:24Z

This patch adds support of non-native SYCL 2020 specialization constants
to DPC++ runtime. Non-native specialization constants emulate the usage
of native specialization constants for AOT compilation and CUDA

This patch adds support of non-native SYCL 2020 specialization constants to DPC++ runtime. Non-native specialization constants emulate the usage of native specialization constants for AOT compilation and CUDA

dm-vodopyanov · 2021-04-21T14:05:11Z

This patch depends on #3561 and #3609. For now tested locally on Linux with workarounds and tests passed.

dm-vodopyanov · 2021-04-23T09:21:08Z

@kbobrovs , @rbegam , please review.

dm-vodopyanov · 2021-04-23T12:34:51Z

@alexbatashev, can you please review this feature?

sycl/source/detail/kernel_bundle_impl.hpp

sycl/test/on-device/basic_tests/specialization_constants/non_native/Inputs/common.cpp

sycl/source/detail/device_image_impl.hpp

smaslov-intel

+1 for PI change

kbobrovs

program_manager.cpp LGTM, with one Nit

kbobrovs · 2021-04-26T21:52:31Z

sycl/source/detail/program_manager/program_manager.cpp

@@ -1309,7 +1309,8 @@ void ProgramManager::bringSYCLDeviceImagesToState(
        break;
      }
      case bundle_state::executable:
-        // Device image is already in the desired state.
+        DevImage = build(DevImage, getSyclObjImpl(DevImage)->get_devices(),


Nit: I assume this build call is optionally needed to do native device code linking? Why not call to link then? Please add a comment.

Build is needed here to create device image which contain spec constants; as device image is in executable state because of AOT, build instead of link (object state) is used.

alexbatashev

LGTM

bader · 2021-04-30T08:50:50Z

@dm-vodopyanov, please, address pre-commit testing failures.

dm-vodopyanov · 2021-04-30T10:25:58Z

@dm-vodopyanov, please, address pre-commit testing failures.

Pre-commit test failures should be resolved after merging of #3609, which now have some merge conflict.

… builbot it runs with L0

…sycl

smaslov-intel · 2021-04-30T19:29:25Z

sycl/test/on-device/basic_tests/specialization_constants/non_native/gpu.cpp

@@ -1,4 +1,4 @@
-// REQUIRES: ocloc, gpu
+// REQUIRES: opencl, ocloc, gpu


why did yo filter our Level Zero?

This test should be built in AOT mode only, so Level Zero should not be here because it don't have such support.
The reason I explicitly added opencl here is that for some reason this test ran on Windows Buildbot for Level Zero only (http://ci.llvm.intel.com:8010/#/builders/18/builds/11898) - but it shouldn't because we have similar classic AOT GPU test here https://github.com/intel/llvm-test-suite/blob/intel/SYCL/AOT/gpu.cpp which doesn't run on Windows on Level Zero. Could be some failure in LIT infra for Windows.

AOT mode only, so Level Zero should not be here because it don't have such support.

Can you elaborate? What exactly do you think is not supported in Level Zero?

Non-native spec constants are available currently only when user is compiling code in AOT mode: for OpenCL CPU/GPU/ACC and CUDA. Level Zero doesn't have ahead-of-time compilation support, only JITting, that's why this test for GPU device requires OpenCL only; for GPU device with CUDA backend this PR introduces another separate test.

Level Zero doesn't have ahead-of-time compilation support

Do you mean SYCL RT running over Level Zero backend doesn't support AOT?
Is there anything missing in Level Zero itself to support AOT?

OK, I had outdated information regarding Level Zero. Now I see that it really has a support of AOT when I compiled some example on Linux with Level Zero in AOT mode. I will remove opencl from the test and check CI again.

BTW, #3609 patch was merged, so currently checks for Linux and Windows are green in CI.

…Zero specific

smaslov-intel

What is the test case that this is fixing? (Looks like a big change to me)

dm-vodopyanov · 2021-04-30T22:23:34Z

What is the test case that this is fixing? (Looks like a big change to me)

@smaslov-intel, could you please elaborate? Not quite understand.

smaslov-intel · 2021-04-30T23:07:06Z

What is the test case that this is fixing? (Looks like a big change to me)

@smaslov-intel, could you please elaborate? Not quite understand.

I am asking specifically about your latest change 022503a

Why is this needed, and how is this tested?

dm-vodopyanov · 2021-05-01T08:32:33Z

Why is this needed, and how is this tested?

In sycl/source/detail/scheduler/commands.cpp we need to pass a buffer (piMem) containing data about spec constants to kernel arguments of a kernel. Before, I used piKernelSetArg func because it suits OpenCL abd CUDA (it accepts piMem). For Level Zero, piKernelSetArg has different interface and doesn't suit our needs (like accepting piMem), so for that, there is a function piextKernelSetArgMemObj which accepts piMem for Level Zero, and just calls piKernelSetArg inside for OpenCL and CUDA.
After changing to piextKernelSetArgMemObj, non-native/gpu.cpp test passed - it runs on Level Zero too.

kbobrovs

program_manager.cpp LGTM

romanovvlad · 2021-05-04T09:52:34Z

sycl/source/handler.cpp

@@ -57,6 +57,10 @@ handler::getOrInsertHandlerKernelBundle(bool Insert) const {
  if (!KernelBundleImpPtr && Insert) {
    KernelBundleImpPtr = detail::getSyclObjImpl(
        get_kernel_bundle<bundle_state::input>(MQueue->get_context()));
+    if (KernelBundleImpPtr->empty()) {


NIT. It would be nice to have a comment explaining this logic.

I'll submit some comments as a separate PR.

romanovvlad · 2021-05-04T09:57:29Z

sycl/source/detail/kernel_bundle_impl.hpp

@@ -442,7 +442,11 @@ class kernel_bundle_impl {
    return SetInDevImg || MSpecConstValues.count(std::string{SpecName}) != 0;
  }

-  const device_image_plain *begin() const { return &MDeviceImages.front(); }
+  const device_image_plain *begin() const {
+    assert(!MDeviceImages.empty() && "MDeviceImages can't be empty");


Could you please clarify why MDeviceImages can't be empty?
I believe this should behave as std::vector which has end() == begin() if empty() is true.

Agree that this is not a valid assert. I'll submit a fix as a separate pull request.

MDeviceImage can't be empty because MDeviceImages.front() is UB in case of MDeviceImages.empty() == true.

@dm-vodopyanov it's UB to access front, but it doesn't mean, that kernel_bundle must have any device image at all. The spec mentions empty() member function for kernel_bundle, which @romanovvlad refers to: https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_the_kernel_bundle_class

[SYCL] Add DPC++ RT support for non-native SYCL 2020 spec constants

6ff1202

This patch adds support of non-native SYCL 2020 specialization constants to DPC++ runtime. Non-native specialization constants emulate the usage of native specialization constants for AOT compilation and CUDA

dm-vodopyanov requested review from kbobrovs and a team as code owners April 21, 2021 14:03

dm-vodopyanov requested a review from rbegam April 21, 2021 14:03

Fix clang-format

bb81670

dm-vodopyanov requested a review from alexbatashev April 23, 2021 12:34

alexbatashev suggested changes Apr 23, 2021

View reviewed changes

Address PR comments

43669f6

dm-vodopyanov requested a review from smaslov-intel as a code owner April 26, 2021 11:36

dm-vodopyanov added 2 commits April 26, 2021 14:52

Update GPU AOT test

06a15c2

Fix clang-format

b5eb9b4

smaslov-intel previously approved these changes Apr 26, 2021

View reviewed changes

kbobrovs previously approved these changes Apr 26, 2021

View reviewed changes

bader requested review from alexbatashev and AlexeySachkov April 29, 2021 19:40

alexbatashev previously approved these changes Apr 30, 2021

View reviewed changes

Small update

8e944f5

dm-vodopyanov dismissed stale reviews from alexbatashev, kbobrovs, and smaslov-intel via 8e944f5 April 30, 2021 11:12

smaslov-intel previously approved these changes Apr 30, 2021

View reviewed changes

Explicitly mark that non-native/gpu.cpp requires OpenCL as on Windows…

2d85e62

… builbot it runs with L0

dm-vodopyanov dismissed smaslov-intel’s stale review via 2d85e62 April 30, 2021 17:39

Merge branch 'private/dvodopya/non-native-sycl2020-spec-consts' into …

665ace5

…sycl

dm-vodopyanov requested a review from AaronBallman as a code owner April 30, 2021 19:19

dm-vodopyanov force-pushed the private/dvodopya/non-native-sycl2020-spec-consts branch from 665ace5 to 2d85e62 Compare April 30, 2021 19:21

dm-vodopyanov removed request for a team, bader, AaronBallman, AGindinson, pvchupin, premanandrao, mdtoguchi, elizabethandrews, mlychkov and DenisBakhvalov April 30, 2021 19:22

smaslov-intel reviewed Apr 30, 2021

View reviewed changes

dm-vodopyanov added 3 commits April 30, 2021 23:23

Remove "REQUIRES: opencl" from non-native/gpu.cpp test

7ae2181

Make the buffer RW as Level Zero supports only RW buffers

0501008

Replace piKernelSetArg with piextKernelSetArgMemObj to support Level …

022503a

…Zero specific

smaslov-intel reviewed Apr 30, 2021

View reviewed changes

dm-vodopyanov requested review from smaslov-intel, alexbatashev and kbobrovs May 1, 2021 08:32

kbobrovs approved these changes May 2, 2021

View reviewed changes

romanovvlad reviewed May 4, 2021

View reviewed changes

romanovvlad approved these changes May 4, 2021

View reviewed changes

romanovvlad merged commit d15b841 into intel:sycl May 4, 2021

dm-vodopyanov deleted the private/dvodopya/non-native-sycl2020-spec-consts branch February 10, 2022 14:07

		@@ -1,4 +1,4 @@
		// REQUIRES: ocloc, gpu
		// REQUIRES: opencl, ocloc, gpu

[SYCL] Add DPC++ RT support for non-native SYCL 2020 spec constants #3589

[SYCL] Add DPC++ RT support for non-native SYCL 2020 spec constants #3589

Uh oh!

Conversation

dm-vodopyanov commented Apr 21, 2021

Uh oh!

dm-vodopyanov commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dm-vodopyanov commented Apr 23, 2021

Uh oh!

dm-vodopyanov commented Apr 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smaslov-intel left a comment

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!

Choose a reason for hiding this comment

Uh oh!

alexbatashev left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Apr 30, 2021

Uh oh!

dm-vodopyanov commented Apr 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dm-vodopyanov Apr 30, 2021 • 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!

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!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

dm-vodopyanov commented Apr 30, 2021

Uh oh!

smaslov-intel commented Apr 30, 2021

Uh oh!

dm-vodopyanov commented May 1, 2021

Uh oh!

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

dm-vodopyanov May 4, 2021 • 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!

Uh oh!

dm-vodopyanov commented Apr 21, 2021 •

edited

Loading

dm-vodopyanov Apr 30, 2021 •

edited

Loading

dm-vodopyanov May 4, 2021 •

edited

Loading