WIP: [SYCL][PI] Make PI an independent library #1983

ProGTX · 2020-06-24T23:06:48Z

This PR extracts PI functionality into a separate library, called piapi.
The library can be built completely independently of DPC++ and LLVM.
It produces piapiConfig.cmake and comes with a CTest test.

When integrating the library into DPC++,
it is built as an independent library,
but sets PI_DPCPP_INTEGRATION to handle a few edge cases.

The PR was done in a way that minimizes code changes
and preserves full backwards compatibility.

Moved all PI headers and CMake into sycl/piapi folder
Removed SYCL specific code from PI headers
Moved PI functionality out of cl::sycl namespace
- Into pi namespace
- pi_sycl.hpp and plugin_sycl.hpp
  for backwards compatibility
Removed contextSetExtendedDeleter
- Never used
Some clang-format of unformatted headers
- Tried to minimize changes

The code compiles and links
(apart from a Level0 linker problem in a test that's easy to fix).
However, there are some TODOs:

XPTI support in PI
Handling SYCLConfig in PI
Package plugins when installing DPC++
Ensure CUDA and Level0 plugins work at runtime
Return contextSetExtendedDeleter
Documentation

bader

Great refactoring!
I'll let runtime team to review this PR, but a have a couple of notes:

it would be great if we can come-up with better name for SYCL plug-in interface library. If I understand it correct both is in piapi mean "interface"...
sycl/piapi/src/shim.cpp is empty - should we remove it?

bader · 2020-06-29T13:16:11Z

sycl/piapi/src/pi.cpp

-
-  std::call_once(PluginsInitDone, []() {
+const std::vector<plugin> &initialize() {
+  static std::vector<plugin> *Plugins = []() {


Please, take a look at the comments of #1614 justifying the need for PluginsInitDone flag.
Tagging @smaslov-intel and @kholland-intel.

The main reason I did it was because I was seeing strange segfaults because of call_once, somehow related to pthreads. I linked in Threads::Threads, but that seemed to have only moved the segfault. As far as I can tell, this code is functionally identical: static initialization is guaranteed to be thread safe and to happen only once. Is there something that I'm missing? I'm not sure from the discussion you linked.

sycl/piapi/plugins/test.cpp

ProGTX · 2020-06-30T14:24:39Z

it would be great if we can come-up with better name for SYCL plug-in interface library. If I understand it correct both is in piapi mean "interface"...

libpi? Seems a bit short, but is probably the most "correct".

sycl/piapi/src/shim.cpp is empty - should we remove it?

That's actually a bit of a workaround to get a unified export header. We want to have a single piapi_export.h header for the plugins. As only the main PI library is guaranteed to exists (plugins could all be optional), we could use that library to tell CMake to generate the export header. But since it's a static library, the generated export header will not work properly. To work around that, we define a "shim" shared library that allows CMake to generate the right kind of header and use that for the plugins.

smaslov-intel · 2020-07-08T11:39:40Z

@ProGTX, why are you doing this change, i.e. why do you want the PI be an independent library (which of course has its downsides too)?

bader · 2020-07-08T11:43:29Z

@ProGTX, why are you doing this change, i.e. why do you want the PI be an independent library (which of course has its downsides too)?

@smaslov-intel, what downsides do you see in this change?

romanovvlad · 2020-07-08T11:36:08Z

sycl/include/CL/sycl/detail/sycl_mem_obj_t.hpp

@@ -20,14 +20,17 @@
 #include <cstring>
 #include <type_traits>

+namespace pi {


Please, define internal things inside cl::sycl::detail namespace.

The PI library needs to be independent of SYCL, this code is forward declaring some PI members.

So, then please, make sure this header is not directly or indirectly included when the user includes sycl.hpp.

romanovvlad · 2020-07-08T11:39:38Z

sycl/piapi/include/pi/plugin.hpp


  /// Checks return value from PI calls.
  ///
  /// \throw Exception if pi_result is not a PI_SUCCESS.
-  template <typename Exception = cl::sycl::runtime_error>
-  void checkPiResult(RT::PiResult pi_result) const {
+  template <typename Exception = std::runtime_error>


Why std? I think we should throw sycl exceptions.

Again, this is because of the need to make PI independent of SYCL. However, that's not always possible, so I've included the DPC++ integration macro, so I can change it to default to cl::sycl::runtime_error when that macro is enabled.

By the SYCL specification we have to throw exceptions of sycl type, not std ones. As a solution this function can be moved out of the PI library files to the SYCL RT level.

smaslov-intel · 2020-07-09T17:43:19Z

@ProGTX, why are you doing this change, i.e. why do you want the PI be an independent library (which of course has its downsides too)?

@smaslov-intel, what downsides do you see in this change?

We will need to be much more strict on the interface to such library, including versioning it. Performance may be another issue since now every call will not be inlined and also done through an extra indirection.

ProGTX · 2020-07-13T08:23:17Z

why do you want the PI be an independent library

This is to allow other projects to use the interface, for example sycl-info. Creating a library makes the interface more stable, and more importantly makes it easy to generate a package.

We will need to be much more strict on the interface to such library, including versioning it

There seems to already be some versioning going on, although just in form of a comment and macros specifying the major and minor version. So already developers have to be careful of that. This patch bumps that version to 2.0.

Performance may be another issue since now every call will not be inlined and also done through an extra indirection.

I still need to add documentation, but this independent PI library is a static library, not a shared one, so I believe performance should be the same. I don't think I moved any inline functions to be non-inline. I had to change a few profiling functions to be extern, but that only has a minor (if any) impact when using XPTI.

Removed SYCL detail includes for PI headers Removed SYCL dependencies from CMake Basic CMake setup for plugins Updated OpenCL in CMake Use higher level CMake Independent PIAPI static library Better package Moved headers to `pi` folder Made plugin class public Some clang-format Enable dynamic linking Basic testing Fixed up CUDA plugin CUDA clang-format Simpler CMake for OpenCL plugin Don't link test with plugin Fixed Windows plugin names Proper test dependency tracking Export `piPluginInit` Put all libraries in same folder Made the test more generic Align CMake requirements with LLVM Set up paths for the test Use `CUDA_CUDA_LIBRARY` Updated PI paths in SYCL Structured CMake file Generate basic config package Fixed reference capture bug Removed `piapiConfigVersion.cmake` Packaging improvements Fixed include headers for package Basic level0 plugin Removed SYCL context dependency `contextSetExtendedDeleter` is never used Use `PIAPI_EXPORT` Moved backends enum into `pi.hpp` Compilable `pi.cpp` Build level0 only when SYCL_BUILD_PI_LEVEL_ZERO Removed awkward symbol visibility setting Link threads Doesn't seem to be enough ... Replaced call_once with static initialization Always set CL_TARGET_OPENCL_VERSION Level 0 compiles CUDA plugin compiles Properly include PI Initial attempt at external project Removed extension header External project compiles Full OpenCL include piapi::piapi Basic includes in RT Fix checkPiResult CMake integration done Returned cude_definitions.hpp Set piapi library as a byproduct Moved GlobalPlugin into `plugin.hpp` PI wrapper headers Forward declare `pi::plugin` Include `pi_cuda` Enabled level0 in SYCL Typo fix Returned `__SYCL_EXPORT` Missed `pi.def` include Link `sycl-ls` with `piapi` library Use `pi::backend` Fixed doc headers Reverted some CUDA changes Reverted some namespace changes Moved `pi_sycl.hpp` to detail SYCL Link with CUDA Try __PI_EXPORT__ Missing pi_cuda.h include Reverted removed CUDA includes DPCPP_INTEGRATION Install into LLVM directory `PI_DPCPP_INTEGRATION` Removed PI ld-version-script Better level0 packaging get_link_library_path Level0 fully linked Documentation for get_library_path XPTI support PI configuration hooks Fix for test environment Better options for toggling plugins Bump PI version to 2.0 Use folders in IDEs Install plugins Moved piapi CMake into new file Use RPATH for test Rebase fix Windows fixes for CUDA plugin LLVM copyright header for PI test Use add_subdirectory for piapi

ProGTX · 2021-05-03T07:58:06Z

Will split up the PR, first part is here: #3679

It will be still missing for OpenCL debug info, but for NonSemantic the correct behavior is preserved. Signed-off-by: Sidorov, Dmitry [email protected] Original commit: KhronosGroup/SPIRV-LLVM-Translator@4645b06

ProGTX requested review from jbrodman, kbobrovs, pvchupin, smaslov-intel and a team as code owners June 24, 2020 23:06

ProGTX force-pushed the peter/piapi branch from 303d39d to ea530ec Compare June 27, 2020 14:39

bader reviewed Jun 29, 2020

View reviewed changes

ProGTX force-pushed the peter/piapi branch from 1a6fde1 to 277d60b Compare June 30, 2020 14:09

romanovvlad reviewed Jul 8, 2020

View reviewed changes

ProGTX force-pushed the peter/piapi branch from d586c92 to 777cd8e Compare November 18, 2020 20:11

ProGTX closed this May 3, 2021

WIP: [SYCL][PI] Make PI an independent library #1983

WIP: [SYCL][PI] Make PI an independent library #1983

Uh oh!

Conversation

ProGTX commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

ProGTX commented Jun 30, 2020

Uh oh!

smaslov-intel commented Jul 8, 2020

Uh oh!

bader commented Jul 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smaslov-intel commented Jul 9, 2020

Uh oh!

ProGTX commented Jul 13, 2020

Uh oh!

ProGTX commented May 3, 2021

Uh oh!

Uh oh!

ProGTX commented Jun 24, 2020 •

edited

Loading