[SYCL][PI] Move files to libpi folder #3679

ProGTX · 2021-05-03T07:57:17Z

The PI plugins could be made into an independent library,
allowing code outside the sycl project to use it.
The first step towards getting that
is to group all PI related files into its own directory structure.

This patch moves all the PI files into the folder libpi.
Includes setting up includes as pi/piXXX.
On the SYCL runtime side the files have been moved
from pi.hpp to pi_sycl.hpp etc.

Apart from the folder change and adjusting CMake,
there should be no change in functionality.
Tests had to be updated to set up include folders.

smaslov-intel

@ProGTX : please update the description of PR with the motivation for this change.

The PI plugins could be made into an independent library, allowing code outside the `sycl` project to use it. The first step towards getting that is to group all PI related files into its own directory structure. This patch moves all the PI files into the folder `libpi`. Includes setting up includes as `pi/piXXX`. On the SYCL runtime side the files have been moved from `pi.hpp` to `pi_sycl.hpp` etc. Apart from the folder change and adjusting CMake, there should be no change in functionality. Tests had to be updated to set up include folders.

ProGTX · 2021-05-05T18:23:47Z

@ProGTX : please update the description of PR with the motivation for this change.

Done, the motivation is to eventually make it into an independent library so that other projects apart from sycl can use it.

smaslov-intel · 2021-05-05T19:14:09Z

the motivation is to eventually make it into an independent library so that other projects apart from sycl can use it.

Could elaborate on what kind of uses you envision and why "libpi" is deemed to be the right abstraction layer for those uses?

bader · 2021-05-11T17:15:10Z

sycl/include/CL/sycl/detail/array.hpp

@@ -9,6 +9,8 @@
 #pragma once
 #include <CL/sycl/detail/type_traits.hpp>
 #include <CL/sycl/exception.hpp>
+#include <pi/pi.h>


It's a pity that we had to include pi.h here to use PI_INVALID_VALUE in the function that has no dependency on plug-in interface.

__SYCL_ALWAYS_INLINE void check_dimension(int dimension) const { #ifndef __SYCL_DEVICE_ONLY__ if (dimension >= dimensions || dimension < 0) { throw cl::sycl::invalid_parameter_error("Index out of range", PI_INVALID_VALUE); } #endif (void)dimension; }

@romanovvlad, @smaslov-intel, I think there should be PI-independent error codes to report errors for cases like this.

Such error codes getting introduced in SYCL2020

bader · 2021-05-11T17:21:34Z

sycl/include/CL/sycl/detail/pi_sycl.hpp

+#pragma once
+
+#include <CL/sycl/detail/defines.hpp>
+#include <pi/pi.hpp>


I'm not sure I understand the motivation to have this wrapper.
Do we have a dependency on defines.hpp from pi.hpp? If so, I think we should remove such tight coupling between plug-in interface and the rest of the sycl runtime library.

bader · 2021-05-11T17:24:08Z

sycl/libpi/plugins/level_zero/CMakeLists.txt

 include_directories("${sycl_inc_dir}")
+include_directories("${pi_include_dir}")


Suggested change

include_directories("${sycl_inc_dir}")

include_directories("${pi_include_dir}")

include_directories("${sycl_inc_dir}" "${pi_include_dir}")

bader · 2021-05-11T17:27:03Z

sycl/source/CMakeLists.txt

@@ -112,7 +113,7 @@ set(SYCL_SOURCES
    "detail/builtins_integer.cpp"
    "detail/builtins_math.cpp"
    "detail/builtins_relational.cpp"
-    "detail/pi.cpp"
+    "${pi_source_dir}/pi.cpp"


Do I understand correctly that building libpi as a separate library will be added in follow-up patches?

bader · 2021-05-11T17:32:56Z

sycl/test/basic_tests/vectors/ctad_fail.cpp

@@ -1,4 +1,4 @@
-// RUN: %clangxx -std=c++17 -fsyntax-only -Xclang -verify %s -I %sycl_include/sycl -Xclang -verify-ignore-unexpected
+// RUN: %clangxx -fsyntax-only -Xclang -verify %s -fsycl -fsycl-targets=%sycl_triple -Xclang -verify-ignore-unexpected


How is this change related to moving files to libpi directory?
NOTE: I don't think we should make this change. Original test validates compilation of DPC++ header with C++ compiler, but this change involves device compilation as well.

bader · 2021-05-11T17:33:53Z

sycl/unittests/kernel-and-program/Cache.cpp

@@ -8,7 +8,7 @@

 #define SYCL2020_DISABLE_DEPRECATION_WARNINGS

-#include "CL/sycl/detail/pi.h"
+#include "CL/sycl/detail/pi_sycl.hpp"


Suggested change

#include "CL/sycl/detail/pi_sycl.hpp"

#include "pi/pi.h"

?

smaslov-intel · 2021-05-12T21:10:30Z

the motivation is to eventually make it into an independent library so that other projects apart from sycl can use it.

Could elaborate on what kind of uses you envision and why "libpi" is deemed to be the right abstraction layer for those uses?

@ProGTX : friendly ping

ProGTX requested review from bader, pvchupin, smaslov-intel and a team as code owners May 3, 2021 07:57

ProGTX mentioned this pull request May 3, 2021

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

Closed

6 tasks

ProGTX force-pushed the peter/libpi branch from 8d4a4db to 6094de7 Compare May 3, 2021 08:06

ProGTX changed the title ~~[PI] Move files to libpi folder~~ [SYCL][PI] Move files to libpi folder May 3, 2021

smaslov-intel reviewed May 3, 2021

View reviewed changes

ProGTX force-pushed the peter/libpi branch from 66b7eea to a0663de Compare May 5, 2021 18:22

bader reviewed May 11, 2021

View reviewed changes

github-actions bot added the Stale label Feb 15, 2022

github-actions bot closed this Mar 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL][PI] Move files to libpi folder #3679

[SYCL][PI] Move files to libpi folder #3679

Uh oh!

ProGTX commented May 3, 2021 •

edited

Loading

Uh oh!

smaslov-intel left a comment

Uh oh!

ProGTX commented May 5, 2021

Uh oh!

smaslov-intel commented May 5, 2021

Uh oh!

bader May 11, 2021

Uh oh!

romanovvlad May 12, 2021

Uh oh!

bader May 11, 2021

Uh oh!

bader May 11, 2021

Uh oh!

bader May 11, 2021

Uh oh!

bader May 11, 2021

Uh oh!

bader May 11, 2021

Uh oh!

smaslov-intel commented May 12, 2021

Uh oh!

Uh oh!

		include_directories("${sycl_inc_dir}")
		include_directories("${pi_include_dir}")

	include_directories("${sycl_inc_dir}")
	include_directories("${pi_include_dir}")
	include_directories("${sycl_inc_dir}" "${pi_include_dir}")

		@@ -1,4 +1,4 @@
		// RUN: %clangxx -std=c++17 -fsyntax-only -Xclang -verify %s -I %sycl_include/sycl -Xclang -verify-ignore-unexpected
		// RUN: %clangxx -fsyntax-only -Xclang -verify %s -fsycl -fsycl-targets=%sycl_triple -Xclang -verify-ignore-unexpected

[SYCL][PI] Move files to libpi folder #3679

[SYCL][PI] Move files to libpi folder #3679

Uh oh!

Conversation

ProGTX commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

ProGTX commented May 5, 2021

Uh oh!

smaslov-intel commented May 5, 2021

Uh oh!

bader May 11, 2021

Choose a reason for hiding this comment

Uh oh!

romanovvlad May 12, 2021

Choose a reason for hiding this comment

Uh oh!

bader May 11, 2021

Choose a reason for hiding this comment

Uh oh!

bader May 11, 2021

Choose a reason for hiding this comment

Uh oh!

bader May 11, 2021

Choose a reason for hiding this comment

Uh oh!

bader May 11, 2021

Choose a reason for hiding this comment

Uh oh!

bader May 11, 2021

Choose a reason for hiding this comment

Uh oh!

smaslov-intel commented May 12, 2021

Uh oh!

Uh oh!

ProGTX commented May 3, 2021 •

edited

Loading