[SYCL] Introduce the Level Zero plugin #1718

againull · 2020-05-19T20:48:40Z

Plugin itself consists of the header and the source file plus cmake
file to build the plugin. Also the following changes were made to
suport the Level Zero plugin in SYCL RT:
* New level0 value was added to the backend enum
* New PI_LEVEL0 value support was added to the SYCL_BE config.
* Docs were updated. Mentioned Level Zero backend and provided the
link to the Level Zero runtime for Intel GPU.
* Changes in sycl cmake file to build Level Zero plugin by default and to
install it with sycl toolchain.

LIT testing with PI_LEVEL0 backend will be enabled in the following
commits. This commits introduces the plugin and makes it buildable.

Signed-off-by: Artur Gainullin artur.gainullin@intel.com

Plugin itself consists of the header and the source file plus cmake file to build the plugin. Also the following changes were made to suport the Level Zero plugin in SYCL RT: * New level0 value was added to backend enum * New PI_LEVEL0 value support was added to SYCL_BE config. * Docs were updated. Mentioned Level Zero backend and provided the link to the Level Zero runtime for Intel GPU. * Changes in sycl cmake file to build level0 plugin by default and to install it with sycl toolchain. LIT testing with PI_LEVEL0 backend will be enabled in the following commits. This commits introduces the plugin and makes it buildable. Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

L0 plugin doesn't support piProgramCompile/piProgramLink commands, program is built during piProgramCreate. Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

againull · 2020-05-19T20:51:02Z

Please don't review the fist commit "[SYCL] Untie PI functions from OpenCL" which is reviewed separately #1717

* Fix clang-format issue * Fix using statement Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

pvchupin · 2020-05-19T22:44:10Z

sycl/doc/GetStartedGuide.md

 Please, refer to [the Release Notes](../ReleaseNotes.md) for recommended Intel
 runtime versions.

-The `GPU` runtime that is needed to run DPC++ application on Intel `GPU` devices
-can be downloaded from the following web pages:
+To run DPC++ application on Intel `GPU` devices the OpenCL `GPU` runtime or the


#1699 have this wording changed a bit.

Thx, according to new wording I just need to specify the version of the L0 runtime in buildbot/dependency.conf. I am not sure if the purpose of this file is just to provide information about version or if it used in some scripts. I will ask people working on CI and then will update this file.

buildbot/dependency.conf is used in CI scripts. Added comment to this file explaining that there is a same driver for Level Zero and OpenCL,

sycl/doc/GetStartedGuide.md

Align with changes made for OpenCL plugin in intel#1638: * Fix build on Windows by defining __SYCL_BUILD_SYCL_DLL in cmake file * Export only pi* symbols in libpi_level0.so * Add test to check exported symbols Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

* Add link to the Level Zero specification * Revert changes in the section providing links to download low level runtimes. Links are supposed to be provided in buildbot/dependency.conf according to upcoming changes intel#1699 Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

romanovvlad · 2020-05-20T08:46:26Z

@againull Could you please PR your patch even more? Some ideas: all changes not in Level0 dir go to a separate PRs; group PI API implementations(queue related, program related) and create separate PRs for each of such group.

againull · 2020-05-20T16:15:33Z

@againull Could you please PR your patch even more? Some ideas: all changes not in Level0 dir go to a separate PRs; group PI API implementations(queue related, program related) and create separate PRs for each of such group.

@romanovvlad did you want to write this comment to #1723 ?

Ruyk

Just a few comments, nice to see more non-opencl plugins added :-)

sycl/plugins/Intel_level0/pi_level0.cpp

Ruyk · 2020-05-20T16:32:22Z

sycl/plugins/Intel_level0/pi_level0.cpp

+    break;
+  case ZE_RESULT_ERROR_UNKNOWN:
+    ErrorString = "ZE_RESULT_ERROR_UNKNOWN";
+    break;


It would be easier to maintain with a macro for each case, e.g.

switch(ZeError) { #define ZE_ERRCASE(ERR)\ case ERR:\ ErrorString = ""#ERR;\ break; ZE_ERRCASE(ZE_RESULT_ERROR_OVERLAPPING_REGIONS); ZE_ERRCASE(ZE_RESULT_ERROR_UNKNOWN); // ... #undef ZE_ERRCASE } // switch

Thank you for the code, fixed.

sycl/plugins/Intel_level0/pi_level0.cpp

Ruyk · 2020-05-20T16:42:33Z

sycl/plugins/Intel_level0/pi_level0.cpp

+                             void (*PFnNotify)(pi_event Event,
+                                               pi_int32 EventCommandStatus,
+                                               void *UserData),
+                             void *UserData) {


Callbacks are not used by SYCL-RT anymore. Among other problems, they caused performance issues on the CUDA backend. This code is not used, could be removed - we are going to mark it as not implemented in the CUDA backend soon.

Removed the body. I think this functions should be removed from PI in the separate commit as they are not used in SYCL RT.

Ruyk · 2020-05-20T16:43:30Z

sycl/plugins/Intel_level0/pi_level0.cpp

+  return PI_SUCCESS;
+}
+
+pi_result piEventSetStatus(pi_event Event, pi_int32 ExecutionStatus) {


This is not used by SYCL RT anymore (thanks @s-kanaev !)

Made same change as for piEventSetCallback.

Ruyk · 2020-05-20T16:44:40Z

sycl/plugins/Intel_level0/pi_level0.cpp

+                          &ZeSamplerDesc, // TODO: translate properties
+                          &ZeSampler));
+
+  *RetSampler = new _pi_sampler(ZeSampler);


On error or bad_alloc this should return error code

Thanks, fixed.

Ruyk · 2020-05-20T16:50:22Z

sycl/plugins/Intel_level0/pi_level0.cpp

+  if (Res != PI_SUCCESS)
+    return Res;
+
+  piEventCreate(Queue->Context, Event);


Why is piEventCreate twice?

Thanks, fixed.

Ruyk · 2020-05-20T16:53:28Z

sycl/plugins/Intel_level0/pi_level0.cpp

+  assert(Flags == PI_MEM_FLAGS_ACCESS_RW);
+  assert(BufferCreateType == PI_BUFFER_CREATE_TYPE_REGION);
+  assert(!(static_cast<_pi_buffer *>(Buffer))->isSubBuffer() &&
+         "Sub-buffer cannot be partitioned");


is this a level0 restriction or just keeping the existing SYCL/OpenCL limitations?

You are right, just keeping the existing SYCL/OpenCL limitations.

Ruyk · 2020-05-20T16:57:20Z

sycl/plugins/Intel_level0/pi_level0.hpp

+
+  // L0 doesn't do the reference counting, so we have to do.
+  // Must be atomic to prevent data race when incrementing/decrementing.
+  std::atomic<pi_uint32> RefCount;


This is also a problem/question we have on the CUDA plugin: Is reference counting required by SYCL-RT? seems from what I understand of the SYCL-RT code, Increment/Decrement of all PI objects is only used for interoperability.

As far as I understand we need to do reference counting to properly release the allocated resources.
@smaslov-intel Could you please give your input here.

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

* Use macro in zeParseError function which is easier to maintain * Fix env variable naming * Remove implementation of piEventSetCallback and piEventSetStatus, make them deprecated. These functions should be removed from PI in the separate patch because they are not used in the SYCL RT. Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

kbobrovs · 2020-05-20T18:24:24Z

sycl/include/CL/sycl/info/info_desc.hpp

-  usm_device_allocations            = PI_USM_DEVICE_SUPPORT,
-  usm_host_allocations              = PI_USM_HOST_SUPPORT,
-  usm_shared_allocations            = PI_USM_SINGLE_SHARED_SUPPORT,
+  usm_device_allocations = PI_USM_DEVICE_SUPPORT,


formatting changes unrelated to this PR

sycl/plugins/Intel_level0/pi_level0.cpp

sycl/plugins/Intel_level0/pi_level0.hpp

sycl/plugins/Intel_level0/pi_level0.cpp

sycl/plugins/Intel_level0/pi_level0.hpp

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

* Added license header to pi_level0.cpp/pi_level0.hpp * use unordered_map instead of map where possible * Refactor _pi_mem::addMapping according to suggestion * Add sanity checks to piProgramCompile and piPrgoramBuild * Narrowed extern "C" to exported functions. * Other minor fixes Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

keryell

Very interesting.
A few questions and nitpicks.

sycl/include/CL/sycl/backend_types.hpp

keryell · 2020-05-20T23:30:06Z

sycl/plugins/level_zero/pi_level0.cpp

+class ZeCall {
+private:
+  // The global mutex that is used for total serialization of L0 calls.
+  static std::mutex GlobalLock;


Is this efficient?

keryell · 2020-05-20T23:31:59Z

sycl/plugins/level_zero/pi_level0.cpp

+// Controls L0 calls tracing in zePrint.
+static bool ZeDebug = false;
+
+static void zePrint(const char *Format, ...) {


Is there a need to provide old unsafe C varargs interface in a C++17 code?

Correction. Library source code is still uses C++14.

Anyway, does it requires to go down to C99?

And I do not remember the real reason for C++14 only if SYCL goes to C++17?

AFAIK, a few SYCL features requiring C++17 are implemented in "header-only" style, so they are not requiring the runtime library to be built with C++17 enabled.
We keep using C++14 features in the library to keep the libstdc++ requirement low and make the implementation more portable.

keryell · 2020-05-20T23:34:07Z

sycl/plugins/level_zero/pi_level0.cpp

+// write (as it is OK for multiple threads to read the map without locking it).
+
+pi_result _pi_mem::addMapping(void *MappedTo, size_t Offset, size_t Size) {
+  std::lock_guard<std::mutex> Lock(MappingsMutex);


Suggested change

std::lock_guard<std::mutex> Lock(MappingsMutex);

std::lock_guard Lock { MappingsMutex };

C++17

@keryell The library is compiled with C++14

keryell · 2020-05-20T23:34:37Z

sycl/plugins/level_zero/pi_level0.cpp

+}
+
+pi_result _pi_mem::removeMapping(void *MappedTo, Mapping &MapInfo) {
+  std::lock_guard<std::mutex> Lock(MappingsMutex);


sycl/plugins/level_zero/pi_level0.hpp

keryell · 2020-05-21T00:09:36Z

sycl/plugins/level_zero/pi_level0.hpp

+    size_t Size;
+  };
+
+  virtual ~_pi_mem() = default;


Move this at the end of the class.
Because my first comment here was "Why do you need this?" :-)

Moved, thx.

sycl/plugins/level_zero/pi_level0.hpp

keryell · 2020-05-21T00:16:22Z

sycl/plugins/level_zero/pi_level0.hpp

+
+  // L0 doesn't do the reference counting, so we have to do.
+  // Must be atomic to prevent data race when incrementing/decrementing.
+  std::atomic<pi_uint32> RefCount;


Perhaps you could move this into a (CRTP?) mixin class to avoid all these copy-pastes in every class or so?

Fixed, thx.

vladimirlaz · 2020-05-21T09:05:57Z

sycl/plugins/level_zero/CMakeLists.txt

@@ -0,0 +1,81 @@
+# PI Level0 plugin library
+
+message(STATUS "Download Level Zero loader and headers from github.com")


Can we have an option to provide local version of the L0 headers and libraries like it is done for OpenCl dependencies?

I can do this. But I have a question about how it is done for OpenCL. In configure.py:

21 ocl_header_dir = os.path.join(abs_obj_dir, "OpenCL-Headers") 22 icd_loader_lib = os.path.join(abs_obj_dir, "OpenCL-ICD-Loader", "build")

These paths are used if we pass --system-ocl to configure. But they are not system paths.
So this option/behavior is a little confusing. Shouldn't we look system paths (like /usr/local/lib or smth) if such option is provided?

Added ability to provide local version of headers and loader.

vladimirlaz · 2020-05-21T09:15:56Z

sycl/plugins/level_zero/CMakeLists.txt

+  DEPENDS           ocl-headers
+  BUILD_BYPRODUCTS ${L0_LIBRARY}
+)
+ExternalProject_Add_Step(l0-loader llvminstall


It looks like l0-loader is not added to deploy-sycl-toolchain. So it will not be available after recommended build process. So if we build compiler and runtime on machine with GPU it might be non-functional due to missed dependency library (l0-loader)

@vladimirlaz I have a question, do we deploy OpenCL.so? I tried to do ninja install and don't see ICD in the installation directory. How this works for OpenCL?

@againull you need to download it on build time (rather than using pre-installed) and specify a special flag on cmake configuration. See fd64c8f#diff-ad9ca624b2a795a5f24145a17c806d1f

Added l0-loader to deploy-sycl-toolchain. One question: how the scenario described by Vladimir will work for OpenCL if ICD is added to deploy-sycl-toolchain only under special option?

kbobrovs

LGTM

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

againull · 2020-06-02T21:01:07Z

Conflict with master branch (in dependency.conf) appeared right just after all approvals. Merging master branch with conflict resolving dismissed all of them:(

pvchupin · 2020-06-02T21:53:49Z

I think you've got all approvals, so let's wait for clean testing and merge.
Me or @bader can merge I think.

againull · 2020-06-05T22:08:57Z

Blocked by #1816 which is going to be fixed in #1824

againull · 2020-06-08T16:30:03Z

There were no changes made after approvals expect merging intel/sycl. Problem on cuda target fixed. Can we merge this PR?

bader · 2020-06-08T16:33:58Z

Can I use the description as a commit message or do you want to update it?

againull · 2020-06-08T16:38:51Z

Can I use the description as a commit message or do you want to update it?

Yes, description is ok for commit message.

againull added 2 commits May 19, 2020 23:39

[SYCL] Don't use SYCL device libs when backend is L0

b0fabec

L0 plugin doesn't support piProgramCompile/piProgramLink commands, program is built during piProgramCreate. Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

againull requested review from kbobrovs, pvchupin, smaslov-intel and a team as code owners May 19, 2020 20:48

againull requested a review from s-kanaev May 19, 2020 20:48

Minor fixes

e59ed9a

* Fix clang-format issue * Fix using statement Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

pvchupin reviewed May 19, 2020

View reviewed changes

againull added 3 commits May 20, 2020 03:23

Fix naming of env variables

120ee9c

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

againull mentioned this pull request May 20, 2020

[SYCL] Implements getNative() interoperability for Level Zero #1723

Merged

AlexeySachkov mentioned this pull request May 20, 2020

SYCL Level-Zero plugin #1640

Closed

Ruyk reviewed May 20, 2020

View reviewed changes

againull added 4 commits May 20, 2020 20:25

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

8866e2c

Update dependency.conf with info about Level Zero

097c595

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

Fix directory name

4e56b22

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

kbobrovs reviewed May 20, 2020

View reviewed changes

Handle bad_alloc and other errors which could happend during new()

0fd2f79

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

againull requested a review from bader as a code owner May 20, 2020 20:15

againull added 2 commits May 20, 2020 15:32

Fix formatting problem

0a0aabf

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

keryell suggested changes May 21, 2020

View reviewed changes

vladimirlaz suggested changes May 21, 2020

View reviewed changes

vladimirlaz previously approved these changes Jun 2, 2020

View reviewed changes

pvchupin previously approved these changes Jun 2, 2020

View reviewed changes

kbobrovs previously approved these changes Jun 2, 2020

View reviewed changes

Merge branch 'sycl' into l0_plugin

77e8a78

againull dismissed stale reviews from kbobrovs, pvchupin, vladimirlaz, smaslov-intel, and keryell via 77e8a78 June 2, 2020 20:53

Make ErrorMapping static

d2164f7

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>

pvchupin previously approved these changes Jun 2, 2020

View reviewed changes

artur.gainullin and others added 4 commits June 3, 2020 23:23

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

5cc9836

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

6a85a39

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

41ea7e2

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

a303cad

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

b792fce

againull dismissed pvchupin’s stale review via b792fce June 6, 2020 08:22

bader approved these changes Jun 6, 2020

View reviewed changes

againull requested review from s-kanaev, kbobrovs, pvchupin and smaslov-intel June 8, 2020 16:32

bader merged commit d32da99 into intel:sycl Jun 8, 2020

againull deleted the l0_plugin branch December 3, 2022 00:03

	std::lock_guard<std::mutex> Lock(MappingsMutex);
	std::lock_guard Lock { MappingsMutex };

		@@ -0,0 +1,81 @@
		# PI Level0 plugin library

		message(STATUS "Download Level Zero loader and headers from github.com")

[SYCL] Introduce the Level Zero plugin #1718

[SYCL] Introduce the Level Zero plugin #1718

Uh oh!

Conversation

againull commented May 19, 2020

Uh oh!

againull commented May 19, 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!

Uh oh!

romanovvlad commented May 20, 2020

Uh oh!

againull commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ruyk left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keryell left a comment

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!

againull commented May 20, 2020 •

edited

Loading