[SYCL][ESIMD][EMU] single_task support, v.2 #5671

lsatanov · 2022-02-25T10:26:40Z

ESIMD EMU: single_task() support.
ESIMD plugin: misc clarifying refactoring.

lsatanov · 2022-02-25T12:51:58Z

sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp

@@ -16,6 +16,7 @@

 #include <CL/sycl/backend_types.hpp>
 #include <CL/sycl/detail/accessor_impl.hpp>
+#include <CL/sycl/detail/cg_types.hpp> // for HostKernelBase


Remnant from variant 1. Will eliminate after testing completes

lsatanov · 2022-02-25T12:52:08Z

sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp

@@ -142,38 +143,21 @@ static char ESimdEmuVersionString[32];

 using IDBuilder = sycl::detail::Builder;

+using HostKernelBase = cl::sycl::detail::HostKernelBase;


Remnant from variant 1. Will eliminate after testing completes

kbobrovs

@romanovvlad, @smaslov-intel - please take a look as well.

kbobrovs · 2022-03-01T05:05:54Z

sycl/include/CL/sycl/handler.hpp

   */
-  // For 'void' and 'sycl::group<Dims>' kernel argument
+  // For 'sycl::group<Dims>' kernel argument


please use single comment style - '//'

kbobrovs · 2022-03-01T05:39:26Z

sycl/include/CL/sycl/detail/cg_types.hpp

@@ -246,6 +246,7 @@ template <class KernelType, class KernelArgType, int Dims>
 class HostKernel : public HostKernelBase {
  using IDBuilder = sycl::detail::Builder;
  KernelType MKernel;
+


here and in other places - please avoid unrelated changes like adding empty lines

kbobrovs · 2022-03-01T05:41:37Z

sycl/include/CL/sycl/handler.hpp

+
+  /* 'wrapper'-based approach using 'NormalizedKernelType' struct is not used
+   * for 'void(sycl::group<Dims>)' since 'void(sycl::group<Dims>)' is not
+   * supported in ESIMD.


Is sycl::group<Dims> kernel supported at all (non-ESIMD case)? If not, then RT error would be better.

My current understanding is the following: it's a "won't happen" case for ESIMD, hence no error.
Shall review this.

Assuming something won't happen and just ignoring the possibility is not safe. Please add error reporting.

That's true. This code was introduced previously and results in runtime failure in case of using esimd emu backend without marking kernel as ESIMD kernel. If kernel is marked as ESIMD kernel - the compiler crashes without a human-friendly error report, which is also to be fixed.

All of it is to be addressed in the following changes.

kbobrovs · 2022-03-01T05:44:53Z

sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp

-template <int NDims> struct LambdaWrapper {
+// kernel execution. Function instances of 'KernelWrapper' un-wrap
+// this struct instance and invoke lambda function ('Func')
+template <int NDims> struct KernelWrapperData {


I don't see how KernelWrapper is better than InvokeLambda. If Lambda does not cover all kernel cases then InvokeKernel or KernelInvoker?
Then KernelWrapperData -> KernelInvocationContext.

If this all is only supposed to be used on a host device (and/or esimd emulator), then I'd add 'Host' prefix as well.

Warning: naming discussion.
Ok, the point was to make more sense (and less code). Yes, kernel is not always lambda.

There are many ways to do it. I consider KernelWrapper and KernelWrapperData clear enough and find adding "Host" prefix for things used inside esimd_emulator only an overkill and not bringing more sense.

If inevitable, the naming can be changed to the required liking.
e.g.,

InvokeKernel
KernelInvocationContext

would be totally clear.

Classes should not be named with verbs, but adhjectives. KernelCaller, KernelInvoker

Warning: naming discussion.

Not sure I understand the warning. You started changing names, so I commented.

Konstantin, I understand your point, but InvokeKernel is a function (template), not a class.

P.S. regarding classes, how they should be named (verbs/adjectives/nouns) is debatable taking into account that they can be callable in C++ (as we all know). In other languages, e.g. JavaScript, everything is an object (including functions). Should we name everything in JavaScript not using verbs? I doubt that.

romanovvlad

LGTM

[SYCL][ESIMD][EMU] misc clarifying plugin refactoring.

lsatanov · 2022-03-07T07:59:59Z

@romanovvlad, @kbobrovs hi. Merging?

bader · 2022-03-10T11:14:55Z

@lsatanov, this patch break the build due to

/home/runner/work/llvm/llvm/src/sycl/include/CL/sycl/handler.hpp:671:44: error: unused parameter 'Arg' [-Werror,-Wunused-parameter]
      void operator()(const nd_item<Dims> &Arg) {
                                           ^
1 error generated.

Could you fix this issue, please?
See full log here: https://github.com/intel/llvm/runs/5493988722?check_suite_focus=true

lsatanov · 2022-03-10T13:21:05Z

@lsatanov, this patch break the build due to
/home/runner/work/llvm/llvm/src/sycl/include/CL/sycl/handler.hpp:671:44: error: unused parameter 'Arg' [-Werror,-Wunused-parameter]
      void operator()(const nd_item<Dims> &Arg) {
                                           ^
1 error generated.
Could you fix this issue, please? See full log here: https://github.com/intel/llvm/runs/5493988722?check_suite_focus=true

Sure! #5779

Interesting, how the pre-commit checks have passed (or compilation options have changed meanwhile this has been reviewed?)

bader · 2022-03-10T13:35:48Z

Post-commit has wider coverage. Thanks for fixing it!

lsatanov requested a review from a team as a code owner February 25, 2022 10:26

lsatanov requested a review from againull February 25, 2022 10:26

lsatanov marked this pull request as draft February 25, 2022 10:27

lsatanov force-pushed the single-task-test-v2 branch 2 times, most recently from 7023689 to b9acc83 Compare February 25, 2022 11:19

lsatanov changed the title ~~Draft v2, ESIMD EMU, single_task~~ [SYCL][ESIMD][EMU] single_task support. Feb 25, 2022

lsatanov changed the title ~~[SYCL][ESIMD][EMU] single_task support.~~ [SYCL][ESIMD][EMU] single_task support, v.2 Feb 25, 2022

lsatanov commented Feb 25, 2022

View reviewed changes

dongkyunahn-intel previously approved these changes Feb 25, 2022

View reviewed changes

lsatanov dismissed dongkyunahn-intel’s stale review via 8063191 February 28, 2022 08:41

lsatanov force-pushed the single-task-test-v2 branch 5 times, most recently from d2fbf16 to b206c79 Compare February 28, 2022 11:28

lsatanov marked this pull request as ready for review February 28, 2022 14:06

lsatanov requested review from ozhdikhin, romanovvlad and kbobrovs February 28, 2022 14:06

kbobrovs reviewed Mar 1, 2022

View reviewed changes

lsatanov force-pushed the single-task-test-v2 branch 3 times, most recently from edd7398 to a538630 Compare March 1, 2022 16:01

romanovvlad previously approved these changes Mar 1, 2022

View reviewed changes

dongkyunahn-intel previously approved these changes Mar 2, 2022

View reviewed changes

lsatanov dismissed stale reviews from dongkyunahn-intel and romanovvlad via 970344d March 2, 2022 10:38

lsatanov force-pushed the single-task-test-v2 branch from a538630 to 970344d Compare March 2, 2022 10:38

[SYCL][ESIMD][EMU] single_task support.

b7e60c1

[SYCL][ESIMD][EMU] misc clarifying plugin refactoring.

lsatanov force-pushed the single-task-test-v2 branch from 970344d to b7e60c1 Compare March 3, 2022 13:42

romanovvlad approved these changes Mar 9, 2022

View reviewed changes

romanovvlad merged commit 2331160 into intel:sycl Mar 9, 2022

		@@ -142,38 +143,21 @@ static char ESimdEmuVersionString[32];

		using IDBuilder = sycl::detail::Builder;

		using HostKernelBase = cl::sycl::detail::HostKernelBase;

[SYCL][ESIMD][EMU] single_task support, v.2 #5671

[SYCL][ESIMD][EMU] single_task support, v.2 #5671

Uh oh!

Conversation

lsatanov commented Feb 25, 2022

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!

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!

lsatanov Mar 10, 2022 • 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!

lsatanov Mar 1, 2022 • 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!

kbobrovs Mar 8, 2022 • 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!

lsatanov Mar 9, 2022 • 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!

romanovvlad left a comment

Choose a reason for hiding this comment

Uh oh!

lsatanov commented Mar 7, 2022

Uh oh!

bader commented Mar 10, 2022

Uh oh!

lsatanov commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bader commented Mar 10, 2022

Uh oh!

Uh oh!

lsatanov Mar 10, 2022 •

edited

Loading

lsatanov Mar 1, 2022 •

edited

Loading

kbobrovs Mar 8, 2022 •

edited

Loading

lsatanov Mar 9, 2022 •

edited

Loading

lsatanov commented Mar 10, 2022 •

edited

Loading