[SYCL] create PI (Plugin Interface) layer and redirect there platform/device stuff. #222

smaslov-intel · 2019-06-16T03:42:01Z

This might look like a lot of changes but the vast majority of it is just renaming and moving (due to file renaming). The key points are:

PI level is being created (pi* files), with true but yet naive plugin mechanism. It replaces CNRI (cnri* files).
New naming conventions are introduced for PI
Directly calling OpenCL is still the default path until everything switches to PI
The platform class implementation is refactored - host and device implementations are merged into single file.

Signed-off-by: Sergey V Maslov [email protected]

romanovvlad · 2019-06-16T11:15:20Z

Copying here major design comments from Ronan from previous pull request. @smaslov-intel , please, address them.

+++++++++++++++++++
The concept is super useful.
We need this to have SYCL more general!
But SYCL is about modern C++ and reimplementing a C-like ICD in SYCL seems not like the right abstraction in 2019. OpenCL is written in C with its limitations...
You are reimplementing dynamic polymorphism in C while it is already available in C++.
An alternative approach can be to have the SYCL implementation classes inheriting from some more or less abstract classes and the SYCL runtime can just use transparently a launch kernel virtual member function of the device class to redirect to the actual kernel launch.
The ICD C-like method does not solve well the adaptation problem because it forces at the first place to encapsulate any runtime (even when written in C++!) to the same low-level C method...
I planned to come with an example of this in triSYCL about using dynamic polymorphism for these extensions triSYCL/triSYCL#237 but we are late... I made some presentation in the SYCL committee with some ideas and in triSYCL there is already the use of dynamic polymorphism but without the ability to inject different runtime on the fly yet...
Your plug-in mechanism can be a class inheriting from the runtime interface and implementing various operations or customization points.
A multilayered abstraction seems better suited than going directly down to the metal, because probably some SYCL implementation parts will be common to some classes of devices or API (for example some devices require eager buffer allocation, other lazy late allocation, and this does not fit well is a low-level only approach).
++++++++++++++++++++
I am unsure that reinventing a low-level runtime C API in a high-level modern C++ API is the good direction.
++++++++++++++++++++
The OpenCL ICD exists just because it is implemented in C according to 1970's techniques...
SYCL is about modern C++...
Your method can work actually, just by reusing the OpenCL ICD or the OpenCL intercepting layer from Intel.
But then do it there, not inside the SYCL implementation.
++++++++++++++++++++

smaslov-intel · 2019-06-16T19:15:08Z

@keryell: We do want to keep the core pi.h interfaces to be C-only for greater interoperability with non-C++ environments, which is envisioned to be needed. The integration of pi.h into SYCL is done with C++ and all the good modern stuff.

sycl/include/CL/sycl/detail/pi.h

keryell

A few typos.
I am understanding better what you are doing, even if I do not understand the internal reason you might have to rename "cl" to "pi_".
It might be clearer in the middle/long-term future to split the ...impl.cpp into several files, to have for example the host backend in some directories or files, the OpenCL in some others, the PI in some others, etc.

sycl/include/CL/sycl/detail/pi.h

sycl/include/CL/sycl/detail/device_impl.hpp

sycl/include/CL/sycl/detail/device_info.hpp

sycl/include/CL/sycl/detail/pi.h

sycl/source/detail/pi_opencl.cpp

sycl/source/detail/platform_impl.cpp

smaslov-intel · 2019-06-18T06:33:50Z

A few typos.

Thanks for pointing those out! I really feel like I need to get a better source editor, not sure that one exist though ;)

reason you might have to rename "cl" to "pi_".

For the most part it is to decouple from OpenCL.

The core of PI is still OpenCL as you can see, but it only includes a subset of OpenCL, and it also extends it with what OpenCL does not have. This, we thought, deserves a new name and also the whole SYCL is moving to untie from OpenCL, making the later just one of the possible back-ends.

keryell · 2019-06-19T17:30:46Z

A few typos.

Thanks for pointing those out! I really feel like I need to get a
better source editor, not sure that one exist though ;)

I do not know what you are using.
I am using an operating system named "Emacs" which has a good editor
mode. ;-) The problem is that the default mode is not that great and
needs some tweaking to shine... :-(
The Git mode is amazingly powerful https://magit.vc/

The core of PI is still OpenCL as you can see, but it only includes a subset of OpenCL, and it also extends it with what OpenCL does not have. This, we thought, deserves a new name and also the whole SYCL is moving to untie from OpenCL, making the later just one of the possible back-ends.

That makes sense.
We can always massage this later anyway.

bader

I think description missing that:

CNRI was removed (or replaced by PI)
platform class implementation is refactored - host and opencl or pi implementations are merged into single file.

sycl/include/CL/sycl/detail/device_info.hpp

bader · 2019-06-21T16:50:05Z

sycl/include/CL/sycl/detail/platform_impl.hpp

@@ -8,8 +8,9 @@

 #pragma once
 #include <CL/sycl/detail/common.hpp>
-#include <CL/sycl/detail/platform_info.hpp>
+#include <CL/sycl/detail/force_device.hpp>


Why did you move force_device.hpp to the interface headers?
This is supposed to be an implementation details of SYCL runtime library only.

I moved them into "detail" headers, which seemed to be consistent with the overall structure. If you inisist, I can copy back to the sources, but prefer doing so in a separate subsequent change-set. Please let me know.

I think the right place for this header is source directory because it doesn't declare public interface, so SYCL library users are not supposed to include this header fie.
It must be included into sycl/source/* files only.

Nothing in the "sycl/detail" is supposed to be included directly by SYCL users, and this one is not an exception to this. If this was really the goal (not to let people see the detail) we could have gone too much further in separation, but is is the goal?

The reason why we have "sycl/detail" sub-directory in public interface is because they contain implementation of template classes/methods, which can't be moved to the source directory, but it's not the case with force_device.hpp.

No problem, we can "hide" this one and many more from sycl/detail if we wanted to.

sycl/include/CL/sycl/detail/pi.h

sycl/include/CL/sycl/detail/pi_opencl.hpp

sycl/source/detail/pi.cpp

sycl/source/detail/pi_opencl.cpp

sycl/source/detail/platform_impl.cpp

Ruyk · 2019-06-24T13:09:37Z

There should be some macro defining the version of the PI interface used. I am assuming incremental changes to the API will happen and that people need to ensure the right version of the PI API is being used, or that some new API entries could be added in future versions.

Simply adding #define PI_VERSION 1 would do, unless there is some other scheme already in place!

keryell · 2019-06-24T19:43:25Z

There should be some macro defining the version of the PI interface used. I am assuming incremental changes to the API will happen and that people need to ensure the right version of the PI API is being used, or that some new API entries could be added in future versions.

Simply adding #define PI_VERSION 1 would do, unless there is some other scheme already in place!

What about using a versioning like TeX?
#define PI_VERSION 3
#define PI_VERSION 3.1
#define PI_VERSION 3.14
#define PI_VERSION 3.141
#define PI_VERSION 3.1415
:-)
More seriously, this macro should be renamed with a name quite longer and related to the project...

bader · 2019-06-25T06:09:30Z

@smaslov-intel, please, rebase the patch to the tip of the branch.

…/device stuff. Signed-off-by: Sergey V Maslov <[email protected]>

Signed-off-by: Sergey V Maslov <[email protected]>

bader

LGTM.
Just a few typos.

sycl/source/detail/device_info.cpp

sycl/include/CL/sycl/detail/device_impl.hpp

sycl/include/CL/sycl/detail/pi.h

sycl/source/detail/device_impl.cpp

sycl/source/detail/device_info.cpp

Signed-off-by: Sergey V Maslov <[email protected]>

This incremental change on top of PI infrastructure being added in #222. It turns all of SYCL RT OpenCL calls into PI. For the time being PI implementation is still directly the OpenCL one (in pi_opencl.*), and the next step will be to run OpenCL through the PI. Signed-off-by: Sergey V Maslov <[email protected]>

Bring spirv.hpp in sync with ddf3230 ("Merge pull request intel#222 from kpet/github-actions", 2021-06-30) from github.com/KhronosGroup/SPIRV-Headers . This brings the SPV_KHR_bit_instructions and SPV_INTEL_debug_module extension enum values. Drop the internal enum value of the latter. Original commit: KhronosGroup/SPIRV-LLVM-Translator@da11807

romanovvlad mentioned this pull request Jun 16, 2019

[SYCL] PI interface. #208

Closed

smaslov-intel commented Jun 16, 2019

View reviewed changes

sycl/include/CL/sycl/detail/pi.h Show resolved Hide resolved

keryell reviewed Jun 18, 2019

View reviewed changes

romanovvlad assigned kbobrovs and bader Jun 20, 2019

romanovvlad self-requested a review June 20, 2019 11:06

bader assigned romanovvlad and unassigned bader Jun 20, 2019

bader reviewed Jun 21, 2019

View reviewed changes

smaslov-intel force-pushed the pi branch 3 times, most recently from 18edc1d to 7142bdc Compare June 26, 2019 01:17

smaslov-intel requested a review from bader June 26, 2019 01:19

This was referenced Jun 26, 2019

[SYCL] populate PI (and direct SYCL RT to PI) #259

Merged

[SYCL] Document the SYCL Runtime Plugin Interface. #206

Merged

smaslov-intel added 2 commits June 27, 2019 16:13

[SYCL] create PI (Plugin Interface) layer and redirect there platform…

fcb840f

…/device stuff. Signed-off-by: Sergey V Maslov <[email protected]>

[SYCL] fixed typos, use less C

62ada2c

Signed-off-by: Sergey V Maslov <[email protected]>

smaslov-intel force-pushed the pi branch from 7142bdc to 8f66d44 Compare June 27, 2019 23:40

bader reviewed Jun 28, 2019

View reviewed changes

[SYCL] address Alexey's comments

15a03a7

Signed-off-by: Sergey V Maslov <[email protected]>

smaslov-intel force-pushed the pi branch from 8f66d44 to 15a03a7 Compare June 28, 2019 19:06

bader approved these changes Jun 28, 2019

View reviewed changes

bader merged commit e0e7177 into intel:sycl Jul 1, 2019

[SYCL] create PI (Plugin Interface) layer and redirect there platform/device stuff. #222

[SYCL] create PI (Plugin Interface) layer and redirect there platform/device stuff. #222

Uh oh!

Conversation

smaslov-intel commented Jun 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romanovvlad commented Jun 16, 2019

Uh oh!

smaslov-intel commented Jun 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

keryell left a comment

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!

Uh oh!

smaslov-intel commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keryell commented Jun 19, 2019

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bader Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

smaslov-intel Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

bader Jun 24, 2019

Choose a reason for hiding this comment

Uh oh!

smaslov-intel Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

bader Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

smaslov-intel Jun 25, 2019

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!

Ruyk commented Jun 24, 2019

Uh oh!

keryell commented Jun 24, 2019

Uh oh!

bader commented Jun 25, 2019

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smaslov-intel commented Jun 16, 2019 •

edited

Loading

smaslov-intel commented Jun 16, 2019 •

edited

Loading

smaslov-intel commented Jun 18, 2019 •

edited

Loading