[SYCL] Plugin Interface And Creation of OpenCL Plugin. #708

garimagu · 2019-10-08T20:32:20Z

created a plugin as a shared plugin: lib/libpi_opencl.so. lib/pi_opencl.dll
Moved the sources to new location plugins/opencl.
removed the dependency on pi.cpp by pi_opencl.cpp
added the preliminary plugin recognition mechanism and loading the
plugin as a shared object using dlopen/dlsym etc.

Signed-off-by: Garima Gupta [email protected]

This change is

sycl/plugins/opencl/pi_opencl.cpp

sycl/source/CMakeLists.txt

keryell · 2019-10-09T00:49:17Z

sycl/source/detail/pi.cpp

      // Any other value would yield PI_OPENCL (current default)
-    }[ GetEnv ? GetEnv : "PI_OPENCL"];
+  }[GetEnv ? GetEnv : "PI_OPENCL"];


What a hack instead of using another ?: for example :-)

Yes. :)
It was already there. Took me an extra minute to understand what it was doing.

Well, is it an obfuscation contest? :-)
Anyway I doubt this code can remain like this...
Some black hats can inject some radioactive string in SYCL_BE and then it inserts Backend {} in the std::map and return this value... What is a Backend {}? :-)
https://en.cppreference.com/w/cpp/container/map/operator_at
This selection logic should be part of a real Backend class, such as a static factory, that should throw or something like that.

Thank you for noticing this.
the default constructor on enum Backend will have 0 in it, which corresponds to SYCL_BE_PI_OPENCL. So it will not be an issue.
The comment above the statement says: " // Any other value would yield PI_OPENCL (current default)"
Let me know if you feel this is still unsafe/obscure and would like me to change it.

I hope you are kidding... :-)
What about launching a SYCL kernel to figure out what is the back-end to use, just in case a std::map memory allocation + insertion time & memory + obfuscation were not enough and too fast? :-)

I understand that it is an overkill for a simple logic. I have edited the code. Let me know if the logic doesn't look better to you.
done.

sycl/source/detail/pi.cpp

sycl/CMakeLists.txt

smaslov-intel · 2019-10-09T22:12:13Z

sycl/include/CL/sycl/detail/pi.hpp

+// Implementation is OS dependent. 
+void* loadOsLibrary(const char* library);
+
+// Function to Function Address corresponding to a symbol in the shared library,


"Function to Function" ?

changed to function to get address of...
done.

smaslov-intel · 2019-10-09T22:12:51Z

sycl/include/CL/sycl/detail/pi.hpp

+
+// Function to Function Address corresponding to a symbol in the shared library,
+// Implementation is OS dependent.
+void* getOsLibraryFuncAddress(void*, const char*);


Please name and describe the arguments

smaslov-intel · 2019-10-09T22:14:02Z

sycl/include/CL/sycl/detail/pi.hpp

@@ -26,6 +35,12 @@ namespace pi {
    SYCL_BE_PI_OTHER
  };

+#ifdef SYCL_RT_OS_WINDOWS
+#define PLUGIN_NAME "pi_opencl"


let's have it be "libpi_opencl.dll"

If the string specifies a module name without a path and the file name extension is omitted, the function appends the default library extension .dll to the module name. To prevent the function from appending .dll to the module name, include a trailing point character (.) in the module name string

So add the "." in the implementation of loadOsLibrary for Windows

i'll change it to pi_opencl.dll?
The windows platform generates the .dll with the same name as pi_opencl. It does not append lib to pi_opencl

smaslov-intel · 2019-10-09T22:16:06Z

sycl/plugins/opencl/CMakeLists.txt

+    PRIVATE ${OpenCL_LIBRARIES}
+)
+
+if (SYCL_USE_LIBCXX)


Is there a better way to apply common workarounds for building a new C++ component?

@ilyastepykin This section of the CMakeFile mirrors the changes from the CMakefile in the sources directory. These changes were added by you. Can you comment why these changes were added ? Do they address a specific requirement?

These changes were added to allow libc++ library to be used instead of libstdc++ for compiling host and device code. For that case SYCL runtime library has to be build and linked with libc++ as well. So this code below does that if SYCL_USE_LIBCXX option is set. Looks like the same thing has to be done for plugin as well.

can this apparatus somehow be defined as a macro, or included from a common file?

The following PR seems to provide such a helper function or what could be the base of a helper function: https://github.com/intel/llvm/pull/685/files#diff-8b14e80fb16152854e3f34abccd44a97R7

sycl/source/detail/pi.cpp

sycl/include/CL/sycl/detail/pi.hpp

Ruyk

Some comments on the OpenCL plugin and overall design.
The intention with the PI API is clearer, and this is a good step towards the modularization of the implementation!

Ruyk · 2019-10-11T13:03:10Z

sycl/plugins/opencl/CMakeLists.txt

+# Create Shared library for libpi_opencl.so.
+#To-DO: remove dependency on pi.hpp in sycl project.
+add_library(pi_opencl SHARED
+  "${sycl_inc_dir}/CL/sycl/detail/pi.hpp"


Where is the pi.hpp going to reside after? sycl/plugins/opencl/XXX or is there a common sycl/plugins/include/ for this header?

Currently, the sycl include headers are being used.
We are refraining to create a copy into a plugins directory yet.
But we can look into it in the future. I have added a TODO above the statement for this.

Ruyk · 2019-10-11T13:04:30Z

sycl/plugins/opencl/CMakeLists.txt

+                                "SYCL_LIBCXX_LIBRARY_PATH should be set")
+        endif()
+        target_include_directories(pi_opencl PRIVATE "${SYCL_LIBCXX_INCLUDE_PATH}")
+        target_link_libraries(pi_opencl PRIVATE "-L${SYCL_LIBCXX_LIBRARY_PATH}" -nodefaultlibs -lc++ -lc++abi -lm -lc -lgcc_s -lgcc)


Are these flags required for the libopencl component?

I think i can ignore -lm, -lgcc_s, -lgcc , lc++abi
I think i need -lc for cassert and cstring; and -lc++ for basic other c++ functionality.
I don't know if the continuous integration tests this setting.
I'll remove the options for now.

Ruyk · 2019-10-11T13:07:14Z

sycl/plugins/opencl/pi_opencl.cpp

+    assert(num_platforms != 0);
+    *num_platforms = 0;
+    result = PI_SUCCESS;
+  }


This is one of the reasons I asked if there could be more than one OpenCL plugin.
This plugin absorbs CL_PLATFORM_NOT_FOUND_KHR error, but a different OpenCL plugin may not. The API contract with the SYCL runtime is then unclear, is ever the CL_PLATFORM_NOT_FOUND error going to be returned to a SYCL runtime from PI API?

@smaslov-intel Will be in a better position to answer this.
I think we should document this as a requirement in the pi.h file where we declare these functions.

Yes, this would be a documented requirement to report "0" if no platforms are found.

Ruyk · 2019-10-11T13:21:52Z

sycl/plugins/opencl/pi_opencl.cpp

+    assert(num_devices != 0);
+    *num_devices = 0;
+    result = PI_SUCCESS;
+  }


If the idea of the PI API is to not return errors if there is no device or platform available, then this should be part of the PI Documentation, so all plugins behave the same.

agreed. I think we will need a separate patch on the PI interface documentation.

Ruyk · 2019-10-11T13:23:32Z

sycl/plugins/opencl/pi_opencl.cpp

+}
+
+pi_result OCL(piextDeviceSelectBinary)(
+    pi_device device, // TODO: does this need to be context?


To match the rest of the build interface, this should take a context and optionally a device. If there is no device, the image selected is for all devices in the context, if there is a device then is only for that device, and its bound to a given context.

@smaslov-intel Please comment and suggest changes to the API and the implementation.

@Ruyk , I don't see why this needs to match the build interfaces. SYCL RT asking for an binary image that can run on specific device (already selected from the current context) sounds like a reasonably good interface. Can you clarify your idea and request in a newly created issue?

Ruyk · 2019-10-11T14:18:12Z

sycl/plugins/opencl/pi_opencl.cpp

+pi_result OCL(piSamplerCreate)(pi_context context,
+                               const pi_sampler_properties *sampler_properties,
+                               pi_sampler *result_sampler) {
+  // Initialize properties according to OpenCL 2.1 spec.


Is it the intention of this plugin to take OpenCL 2.1 as "golden" and map all interface to this version of OpenCL?
It is fine, but it should be made clearer maybe at the top or in the plugin documentation

@smaslov-intel Please comment on this.

Ruyk · 2019-10-11T14:20:36Z

sycl/plugins/opencl/pi_opencl.cpp

+  using FuncT =
+      cl_int(CL_API_CALL *)(cl_device_id, cl_program, const char *, cl_ulong *);
+
+  // TODO: add check that device supports corresponding extension


Is it necessary to add this check on every call to this function? Seems it adds unnecessary overhead/repetition - even if its not much.
Maybe its better to have a PI interface to query extensions, so the SYCL runtime can query once and decide if Function Pointers are possible or not.

@smaslov-intel

Ruyk · 2019-10-11T14:24:05Z

sycl/plugins/opencl/pi_opencl.cpp

+_PI_CL(piEventGetProfilingInfo, clGetEventProfilingInfo)
+_PI_CL(piEventsWait, clWaitForEvents)
+_PI_CL(piEventSetCallback, clSetEventCallback)
+_PI_CL(piEventSetStatus, clSetUserEventStatus)


piEventSetStatus should be an picl extension, other plugins may not support this feature

Ruyk · 2019-10-11T14:30:39Z

sycl/source/detail/pi.cpp

+// Find the plugin at the appropriate location and return the location in
+// PluginPath
+// TODO: Change the function appropriately when there are multiple plugins.
+bool findPlugin(std::string &PluginPath) {


Suggested change

bool findPlugin(std::string &PluginPath) {

std::string findPlugin() {

and simply check if string is empty on calling point?

The code will be more complex in this function.
It may not return a single plugin, but a set of plugins it finds. So we will be changing the API based on the functionality we add. I think it is a little symmetrical to the bindPlugin API as well. Let me know if you feel it must be changed, I will do it.

My first thought seeing the function in use (where the reference on the parameter is not visible) was that it searches for the plugin named in PluginPath and returns if it can find it.

Makes sense. Ruyman's suggestion is more understandable in that case.
I will change the function format.
done.

sycl/source/detail/pi.cpp

garimagu · 2019-10-12T01:11:06Z

Regarding the arbitration of the PI_CALL to the appropriate device, I think we can also discuss it on: #680
as a part of the doc update on PI design.

bjoernknafla · 2019-10-14T14:53:30Z

sycl/include/CL/sycl/detail/pi.hpp

  // TODO: see if more sanity checks are possible.
-  PI_ASSERT(sizeof(From) == sizeof(To), "cast failed size check");
+  assert(sizeof(From) == sizeof(To) && "cast failed size check");


I think this could be a static_assert and then the #include <cassert> could be removed.

I am reverting this change.
The call may be at place to have run time checking of the cast, and it may be valid at other locations where pi::cast is used.
In my code in pi_opencl.cpp, I am removing this call and replacing it with static_assert.
Thank you for the suggestion.

bjoernknafla · 2019-10-14T15:30:28Z

sycl/plugins/opencl/pi_opencl.cpp

+    if (res_program != nullptr)
+      *res_program = nullptr;
+    return PI::cast<pi_result>(CL_INVALID_CONTEXT);
+  }


There is not good CL error code to return that the context belongs to a platform that is missing required extensions or is not of the required OpenCL version. Not for this MR, but wondering if checks for required extensions could happen at platform query time to not even expose unusable platforms?

@smaslov-intel Please comment

bjoernknafla · 2019-10-14T15:34:10Z

sycl/plugins/opencl/pi_opencl.cpp

+    *res_program = PI::cast<pi_program>(funcPtr(
+        PI::cast<cl_context>(context), il, length, PI::cast<cl_int *>(&err)));
+  else
+	err = PI_INVALID_VALUE;


Probably not part of this MR though PI could be documented to require a non-nullptr value for res_program so that a single assert at function entry is enough.

bjoernknafla · 2019-10-14T15:58:46Z

sycl/source/detail/pi.cpp

+
+// Load the Plugin by calling the OS dependent library loading call.
+// Return the handle to the Library.
+void *loadPlugin(const std::string &PluginPath) { return loadOsLibrary(PluginPath); }


Why the indirection through loadOsLibrary, couldn't loadOsLibrary not be used directly or is the plan to add more path lookup logic here?

We will be adding more lookup logic in the future.
There may be multiple compatible plugins to be found and multiple locations to be searched at.

bjoernknafla · 2019-10-14T16:06:20Z

sycl/source/detail/pi.cpp

+// Find the plugin at the appropriate location and return the location in
+// PluginPath
+// TODO: Change the function appropriately when there are multiple plugins.
+bool findPlugin(std::string &PluginPath) {


My first thought seeing the function in use (where the reference on the parameter is not visible) was that it searches for the plugin named in PluginPath and returns if it can find it.

sycl/source/detail/pi.cpp

bjoernknafla · 2019-10-14T16:17:58Z

sycl/source/detail/windows_pi.cpp

+#include <string>
+
+void *loadOsLibrary(const std::string &PluginPath) {
+  // TODO: Check if the option RTLD_NOW is correct.


Copy-and-paste comment.

Wondering if the path separators need to be changed to be Windows compatible (should the PluginPath contain Unix forward slashes)?

Currently, we do not have this problem , since we only use the name of the file in PluginPath, and the file is looked for in paths listed in LD_LIBRARY_PATH(Unix) or LIB(Windows) EV.
After we extend this logic, maybe we will need to do that.

Deleted the comment.

bjoernknafla · 2019-10-14T16:21:30Z

sycl/include/CL/sycl/detail/pi.hpp

+
+// Function to get Address of a symbol defined in the shared
+// library, Implementation is OS dependent.
+void *getOsLibraryFuncAddress(void *Library, const std::string &FunctionName);


Wondering if the LLVM Support class DynamicLibrary could be used here - though there might be unintended interaction with the libraries loaded and globally stored by a LLVM runtime compiler.

May be worth a further investigation, but it seems to have its own semantics like unloading only at process end, or searching symbol by name in all loaded libraries, and not the specific one, which isn't suitable for our needs here.

You are right, too much baggage and unwanted behavior.

keryell · 2019-10-15T02:50:05Z

sycl/source/detail/pi.cpp

-      { "PI_OTHER",  SYCL_BE_PI_OTHER }
-      // Any other value would yield PI_OPENCL (current default)
-    }[ GetEnv ? GetEnv : "PI_OPENCL"];
+      (StringGetEnv == "PI_OTHER" ? SYCL_BE_PI_OTHER : SYCL_BE_PI_OPENCL);


This is quite simpler and more understandable now.

bjoernknafla · 2019-10-15T12:46:17Z

sycl/source/detail/linux_pi.cpp

+
+void *loadOsLibrary(const std::string &PluginPath) {
+  // TODO: Check if the option RTLD_NOW is correct.
+  return dlopen(PluginPath.c_str(), RTLD_NOW);


Wondering if it would be useful to also set RTLD_DEEPBIND to prevent the plugin from picking up symbols from the runtime, e.g., to prevent LLVM symbol collisions, though the flag is not POSIX compatible?

bjoernknafla · 2019-10-15T12:50:12Z

sycl/source/detail/windows_pi.cpp

+#include <string>
+
+void *loadOsLibrary(const std::string &PluginPath) {
+  return (void *)LoadLibraryA(PluginPath.c_str());


Would it be useful to prevent Windows error dialogue boxes by using SetErrorMode or SetThreadErrorMode, e.g., to be able to run on headless nodes?

The latest PI doc PR writes that the LIB environment variable is used to find dynamic libraries. Though looking into the description of LoadLibraryA and Dynamci-Link Library Search Order I do not see this env var mentioned. Is the plan to implement use of the LIB env var here to not go through the PATH?

bjoernknafla · 2019-10-15T12:55:13Z

sycl/plugins/opencl/CMakeLists.txt

+#TODO: remove dependency on pi.hpp in sycl project.
+#TODO: Currently, the pi.hpp header is common between sycl and plugin library sources. 
+#This can be changed by copying the pi.hpp file in the plugins project.
+add_library(pi_opencl SHARED


It could be interesting to allow static linking, too, e.g., to create a smaller, more controlled runtime for specific platforms. Unsure about performance gains and unsure how complicated it would be to support static and dynamic plugins.

Though that would need a lot of extra work on making the plugin discovery process work in both cases, so probably not for this MR.

bjoernknafla · 2019-10-15T18:53:50Z

sycl/CMakeLists.txt


 # SYCL toolchain builds all components: compiler, libraries, headers, etc.
 add_custom_target( sycl-toolchain
  DEPENDS sycl
+          pi_opencl


This could be moved into the plugins' CMake, e.g. in sycl/plugins/opencl/CMakeLists.txt:

add_dependencies(sycl-toolchain pi_opencl)

Adding a plugin would then only rquire add_subdirectory call in sycl/plugins/CMakeLists.txt. As plugin authors would copy the CMakeLists.txt of existing plugins it becomes harder to accidentally forget to set up this dependency.

(Also, according to the CMake docs, to add targets as dependencies to a custom target add_dependencies should be used - but that belongs into a different MR).

Though we would have to move the add_subdirectory(plugins) below the definition of the custom sycl-toolchain target.

bjoernknafla · 2019-10-15T19:10:22Z

sycl/source/CMakeLists.txt

@@ -56,8 +63,9 @@ add_library(sycl SHARED
  "sampler.cpp"
  "stream.cpp"
  "spirv_ops.cpp"
+  "${OS_PLUGIN_INTERFACE}"


"${OS_PLUGIN_INTERFACE}" could be replaced with a generator expression and the above if-else could be removed (untested):

$<$<PLATFORM_ID:Windows>:detail/windows_pi.cpp> $<$<PLATFORM_ID:Linux>:detail/linux_pi.cpp>

smaslov-intel

Reviewed 1 of 9 files at r1, 1 of 4 files at r2, 1 of 6 files at r3, 4 of 5 files at r4, 1 of 1 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 32 unresolved discussions (waiting on @bjoernknafla, @garimagu, @ilyastepykin, @keryell, @romanovvlad, and @Ruyk)

sycl/include/CL/sycl/detail/pi.hpp, line 39 at r2 (raw file):

Previously, garimagu (Garima Gupta) wrote…

i'll change it to pi_opencl.dll?
The windows platform generates the .dll with the same name as pi_opencl. It does not append lib to pi_opencl

What will it open if you send "libpi_opencl.dll."?

sycl/source/detail/linux_pi.cpp, line 6 at r5 (raw file):

RTLD_DEEPBIND
Maybe a comment for now to investigate this if needed?

sycl/source/detail/pi.cpp, line 55 at r6 (raw file):

// Find the plugin at the appropriate location and return the location in
// PluginPath

update the comment

sycl/plugins/opencl/pi_opencl.cpp, line 207 at r2 (raw file):

Previously, garimagu (Garima Gupta) wrote…

@smaslov-intel Please comment on this.

I think in the long run it will need to work with different version of OpenCL where possible.

sycl/plugins/opencl/pi_opencl.cpp, line 251 at r2 (raw file):

Previously, garimagu (Garima Gupta) wrote…

@smaslov-intel

I think it makes sense to be able querying certain "large" features from the platform/device, but that can probably be an extension of piPlatformGetInfo/piDeviceGetInfo. If you think this is important please file a new issue

sycl/plugins/opencl/pi_opencl.cpp, line 334 at r2 (raw file):

Previously, Ruyk (Ruyman) wrote…

piEventSetStatus should be an picl extension, other plugins may not support this feature

The current SYCL RT uses it for for syncing on the events from different context/queues.
Anyway, let's have this discussion out of this PR (new issue?), which is just for restructuring how OpenCL plugin is built.

sycl/plugins/opencl/pi_opencl.cpp, line 181 at r3 (raw file):

Previously, garimagu (Garima Gupta) wrote…

@smaslov-intel Please comment

@bjoernknafla, How would you determine if the platform is usable? E.g. in this case it is only not-usable if you were going to build a program for it. If you think we need to define this better please create a new issue.

smaslov-intel

Some comments on the OpenCL plugin and overall design.

Let's keep these discussions outside of this PR and create new issues to follow up later.

Reviewable status: all files reviewed, 32 unresolved discussions (waiting on @bjoernknafla, @garimagu, @ilyastepykin, @keryell, @romanovvlad, and @Ruyk)

smaslov-intel · 2019-10-16T01:34:23Z

As you may see, I've used https://reviewable.io for following up on many different discussions opened in many different revisions. I found it so much more easier to keep control of all the comments there, that I'd strongly prefer to continue using that. Note, it seems that the new comments that I've added are only visible in the main "Conversation" view, but not when in the "Files changed" view. Please let me know if that's not something that you can work with.

ilyastepykin · 2019-10-16T11:53:19Z

sycl/plugins/opencl/CMakeLists.txt

+
+# Workaround for bug in GCC version 5 and higher.
+# More information https://bugs.launchpad.net/ubuntu/+source/gcc-5/+bug/1568899
+if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND


Looks like this code also duplicates code from source's cmake file similar to libc++ handling above. Perhaps it should be handled in a similar way as libc++ options.

bjoernknafla

Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @bjoernknafla, @garimagu, @ilyastepykin, @keryell, @romanovvlad, @Ruyk, and @smaslov-intel)

sycl/source/detail/linux_pi.cpp, line 6 at r5 (raw file):

Previously, smaslov-intel wrote…

RTLD_DEEPBIND
Maybe a comment for now to investigate this if needed?

Or we wait until we hit symbol collusion and then investigate dynamic library loading. Will resolve this.

sycl/plugins/opencl/pi_opencl.cpp, line 181 at r3 (raw file):

Previously, smaslov-intel wrote…

@bjoernknafla, How would you determine if the platform is usable? E.g. in this case it is only not-usable if you were going to build a program for it. If you think we need to define this better please create a new issue.

I opened an issue for this to not block the MR that has nothing to do with my comment: #738

garimagu

Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @bjoernknafla, @garimagu, @ilyastepykin, @keryell, @romanovvlad, @Ruyk, and @smaslov-intel)

sycl/CMakeLists.txt, line 147 at r2 (raw file):

Previously, smaslov-intel wrote…

I still see this indented differently than the line after

fixed.

sycl/CMakeLists.txt, line 147 at r5 (raw file):

Previously, bjoernknafla (Bjoern Knafla) wrote…

Though we would have to move the add_subdirectory(plugins) below the definition of the custom sycl-toolchain target.

Thank you!

sycl/include/CL/sycl/detail/pi.hpp, line 39 at r2 (raw file):

Previously, smaslov-intel wrote…

What will it open if you send "libpi_opencl.dll."?

There will be no file created by the name libpi_opencl.dll. It will be created as pi_opencl.dll on Windows.
The same cmake command add_library (pi_opencl SHARED ...
on windows does not append lib to the library name, whereas linux does. Eg: linux creates libsycl.so and windows creates sycl.dll .
To be able to create libpi_opencl.dll; I will have to change the CMake file. Let me know if you need me to do that.

sycl/plugins/opencl/CMakeLists.txt, line 31 at r2 (raw file):

Previously, bjoernknafla (Bjoern Knafla) wrote…

The following PR seems to provide such a helper function or what could be the base of a helper function: https://github.com/intel/llvm/pull/685/files#diff-8b14e80fb16152854e3f34abccd44a97R7

Thanks. I will use the method they are using.

sycl/plugins/opencl/CMakeLists.txt, line 12 at r5 (raw file):

Previously, bjoernknafla (Bjoern Knafla) wrote…

Though that would need a lot of extra work on making the plugin discovery process work in both cases, so probably not for this MR.

I do not think the design intends to add static linking. But we can consider it as a separate issue.
Please create a separate issue on this to be discussed further.

sycl/plugins/opencl/CMakeLists.txt, line 50 at r6 (raw file):

Previously, ilyastepykin (Ilya Stepykin) wrote…

Looks like this code also duplicates code from source's cmake file similar to libc++ handling above. Perhaps it should be handled in a similar way as libc++ options.

Sure. I'll use a function for this.

sycl/source/CMakeLists.txt, line 66 at r5 (raw file):

Thanks. This is a very good option.

sycl/source/detail/linux_pi.cpp, line 6 at r5 (raw file):

Maybe a comment for now to investigate this if needed?
      6 hours ago
    bjoernknaflaBjoern Knafla

Added a comment.

sycl/source/detail/pi.cpp, line 45 at r5 (raw file):

Previously, keryell (Ronan Keryell) wrote…

This is quite simpler and more understandable now.

Done.

sycl/source/detail/pi.cpp, line 55 at r6 (raw file):

Previously, smaslov-intel wrote…

// Find the plugin at the appropriate location and return the location in
// PluginPath

update the comment

updated.

sycl/source/detail/windows_pi.cpp, line 6 at r5 (raw file):

Previously, bjoernknafla (Bjoern Knafla) wrote…

The latest PI doc PR writes that the LIB environment variable is used to find dynamic libraries. Though looking into the description of LoadLibraryA and Dynamci-Link Library Search Order I do not see this env var mentioned. Is the plan to implement use of the LIB env var here to not go through the PATH?

Oh.. I missed that the location searched at is PATH and not LIB. I somehow thought that Windows looks at the LIB directory. Currently, pi_opencl.dll is kept at the same place where sycl.dll is.
Let me discuss this further in the PI doc PR, if we should change the logic to look into LIB env var locations and not PATH. Thanks for bringing this up.

ilyastepykin · 2019-10-17T13:27:14Z

sycl/CMakeLists.txt

@@ -143,8 +143,36 @@ if (MSVC)
  list(APPEND SYCL_RT_LIBS sycld)
 endif()

+# This function allows building multiple libraries with the same options.
+# Currently used by sycl and plugins library. 
+function( add_libcxxsupport_gccworkaround LIB_NAME)


May be rename it to something like "add_common_options"? Otherwise, LGTM.

smaslov-intel

Reviewed 5 of 5 files at r7.
Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @bjoernknafla, @garimagu, @ilyastepykin, @keryell, @romanovvlad, and @Ruyk)

sycl/include/CL/sycl/detail/pi.hpp, line 39 at r2 (raw file):

Previously, garimagu (Garima Gupta) wrote…

There will be no file created by the name libpi_opencl.dll. It will be created as pi_opencl.dll on Windows.
The same cmake command add_library (pi_opencl SHARED ...
on windows does not append lib to the library name, whereas linux does. Eg: linux creates libsycl.so and windows creates sycl.dll .
To be able to create libpi_opencl.dll; I will have to change the CMake file. Let me know if you need me to do that.

We could complicate our build system a bit (manually add "lib" prefix) to make similar thing looks similar to the user. But I agree it is the issue with all the libraries, so let's not cover that here

sycl/plugins/opencl/pi_opencl.cpp, line 181 at r3 (raw file):

Previously, bjoernknafla (Bjoern Knafla) wrote…

I opened an issue for this to not block the MR that has nothing to do with my comment: #738

thanks!

bjoernknafla

Reviewable status: 6 of 9 files reviewed, 26 unresolved discussions (waiting on @bjoernknafla, @garimagu, @ilyastepykin, @keryell, @romanovvlad, @Ruyk, and @smaslov-intel)

sycl/plugins/opencl/CMakeLists.txt, line 12 at r5 (raw file):

Previously, garimagu (Garima Gupta) wrote…

I do not think the design intends to add static linking. But we can consider it as a separate issue.
Please create a separate issue on this to be discussed further.

Thinking about this it does not make sense to try to design for all eventualities and just add noise to the existing issue backlog. I will simply resolve this issue. Sorry for the noise, I am just getting to excited for this.

garimagu · 2019-10-22T07:11:05Z

Hi, Please let me know if anything else is needed from this patch.

keryell

That looks good to me.

garimagu · 2019-10-24T17:10:54Z

That looks good to me.

Thanks Ronan.
@romanovvlad, @kbobrovs Please check and let me know if something else is needed. If no, please merge?

garimagu · 2019-10-29T23:11:49Z

@kbobrovs @romanovvlad

romanovvlad · 2019-10-31T12:23:24Z

sycl/include/CL/sycl/detail/pi.hpp

@@ -13,169 +13,176 @@
 #include <CL/sycl/detail/common.hpp>
 #include <CL/sycl/detail/os_util.hpp>
 #include <CL/sycl/detail/pi.h>


Please, add empty line before system headers.

romanovvlad · 2019-10-31T12:25:02Z

sycl/include/CL/sycl/detail/pi.hpp

+
+// Function to load the shared library
+// Implementation is OS dependent.
+void *loadOsLibrary(const std::string &library);


Please, try to use the consistent coding style: for new files - LLVM coding style, for existing ones - the same that is already used in the file.
Specifically, here "library" on line 25 "Library".

romanovvlad · 2019-10-31T12:25:17Z

sycl/include/CL/sycl/detail/pi.hpp

+void *loadOsLibrary(const std::string &library);
+
+// Function to get Address of a symbol defined in the shared
+// library, Implementation is OS dependent.


Suggested change

// library, Implementation is OS dependent.

// library, implementation is OS dependent.

romanovvlad · 2019-10-31T12:29:17Z

sycl/plugins/opencl/pi_opencl.cpp

@@ -6,71 +6,77 @@
 //
 //===----------------------------------------------------------------------===//
 #include "CL/opencl.h"
-#include <CL/sycl/detail/pi.hpp>
+#include <CL/sycl/detail/pi.h>


Please, add empty line before system headers.

romanovvlad · 2019-10-31T12:30:52Z

sycl/plugins/opencl/pi_opencl.cpp

  if (ret_err != CL_SUCCESS || deviceCount < 1) {
    if (res_program != nullptr)
      *res_program = nullptr;
    return cast<pi_result>(CL_INVALID_CONTEXT);
  }

+  ret_err = clGetContextInfo(cast<cl_context>(context), CL_CONTEXT_DEVICES,
+                             deviceCount * sizeof(cl_device_id),
+                             devicesInCtx.data(), NULL);


Suggested change

devicesInCtx.data(), NULL);

devicesInCtx.data(), nullptr);

The field is size_t type.
http://khronos.org/registry/OpenCL/sdk/1.0/docs/man/xhtml/clGetContextInfo.html

I'll change it at other locations.

It looks like a mistake on the webpage i gave. I'll correct the code to use nullptr.

romanovvlad · 2019-10-31T12:31:33Z

sycl/plugins/opencl/pi_opencl.cpp

    } else if (sampler_properties[i] == PI_SAMPLER_INFO_FILTER_MODE) {
      filterMode = static_cast<pi_sampler_filter_mode>(sampler_properties[++i]);
    } else {
-      PI_ASSERT(false, "Cannot recognize sampler property");
+      assert(false && "Cannot recognize sampler property");


Why are you changing to assert here?

To keep the dependency of plugin code restricted to pi.h/pi.def files only.
PI_ASSERT is defined in pi.hpp file, which has plugin interface related templated utilities and classes.

romanovvlad · 2019-10-31T12:35:46Z

sycl/source/detail/pi.cpp

+
+  std::string PluginPath = findPlugin();
+  if (PluginPath.empty())
+    die("Plugin Not Found.");


Why are we "die" here? Shouldn't we just return saying "no opencl plugins/platforms" available to the caller?

By return do you mean return and halt the program execution?
The function die currently, prints a message and calls std::terminate.

No I mean just return an error. SYCL RT can continue working with other plugins.

romanovvlad · 2019-10-31T12:39:59Z

I see that there are still several unresolved comment from bjoernknafla .

garimagu

Reviewable status: 4 of 9 files reviewed, 33 unresolved discussions (waiting on @bjoernknafla, @garimagu, @ilyastepykin, @keryell, @romanovvlad, @Ruyk, and @smaslov-intel)

sycl/CMakeLists.txt, line 147 at r5 (raw file):

Previously, garimagu (Garima Gupta) wrote…

Thank you!

Done.

sycl/CMakeLists.txt, line 148 at r7 (raw file):

Previously, ilyastepykin (Ilya Stepykin) wrote…

May be rename it to something like "add_common_options"? Otherwise, LGTM.

Done.

sycl/include/CL/sycl/detail/pi.hpp, line 39 at r2 (raw file):

Previously, smaslov-intel wrote…

We could complicate our build system a bit (manually add "lib" prefix) to make similar thing looks similar to the user. But I agree it is the issue with all the libraries, so let's not cover that here

Done.

sycl/include/CL/sycl/detail/pi.hpp, line 187 at r3 (raw file):

Previously, garimagu (Garima Gupta) wrote…

I am reverting this change.
The call may be at place to have run time checking of the cast, and it may be valid at other locations where pi::cast is used.
In my code in pi_opencl.cpp, I am removing this call and replacing it with static_assert.
Thank you for the suggestion.

Done.

sycl/include/CL/sycl/detail/pi.hpp, line 15 at r9 (raw file):

Previously, romanovvlad (Romanov Vlad) wrote…

Please, add empty line before system headers.

Done.

sycl/include/CL/sycl/detail/pi.hpp, line 21 at r9 (raw file):

Previously, romanovvlad (Romanov Vlad) wrote…

Please, try to use the consistent coding style: for new files - LLVM coding style, for existing ones - the same that is already used in the file.
Specifically, here "library" on line 25 "Library".

Done.

sycl/include/CL/sycl/detail/pi.hpp, line 24 at r9 (raw file):

Previously, romanovvlad (Romanov Vlad) wrote…

// library, implementation is OS dependent.

Done.

sycl/plugins/opencl/CMakeLists.txt, line 11 at r2 (raw file):

Previously, garimagu (Garima Gupta) wrote…

Currently, the sycl include headers are being used.
We are refraining to create a copy into a plugins directory yet.
But we can look into it in the future. I have added a TODO above the statement for this.

Done.

sycl/plugins/opencl/CMakeLists.txt, line 40 at r2 (raw file):

Previously, garimagu (Garima Gupta) wrote…

I think i can ignore -lm, -lgcc_s, -lgcc , lc++abi
I think i need -lc for cassert and cstring; and -lc++ for basic other c++ functionality.
I don't know if the continuous integration tests this setting.
I'll remove the options for now.

Note that these options are used in common with the latest changes, since the same options are used for compiling sycl and pi_opencl library.

sycl/plugins/opencl/CMakeLists.txt, line 12 at r5 (raw file):

Previously, bjoernknafla (Bjoern Knafla) wrote…

Thinking about this it does not make sense to try to design for all eventualities and just add noise to the existing issue backlog. I will simply resolve this issue. Sorry for the noise, I am just getting to excited for this.

Done.

sycl/plugins/opencl/CMakeLists.txt, line 50 at r6 (raw file):

Previously, garimagu (Garima Gupta) wrote…

Sure. I'll use a function for this.

Done.

sycl/source/detail/windows_pi.cpp, line 6 at r5 (raw file):

Previously, garimagu (Garima Gupta) wrote…

Oh.. I missed that the location searched at is PATH and not LIB. I somehow thought that Windows looks at the LIB directory. Currently, pi_opencl.dll is kept at the same place where sycl.dll is.
Let me discuss this further in the PI doc PR, if we should change the logic to look into LIB env var locations and not PATH. Thanks for bringing this up.

I have updated the doc PR that the plugin will be looked at PATH locations for now. It changes as we add config file support.

sycl/plugins/opencl/pi_opencl.cpp, line 32 at r2 (raw file):

Previously, smaslov-intel wrote…

Yes, this would be a documented requirement to report "0" if no platforms are found.

Needs to be added to PI Interface documentation.

sycl/plugins/opencl/pi_opencl.cpp, line 9 at r9 (raw file):

Previously, romanovvlad (Romanov Vlad) wrote…

Please, add empty line before system headers.

Done.

garimagu · 2019-10-31T19:09:35Z

I see that there are still several unresolved comment from bjoernknafla .

I'm unable to find any that has not been addressed. I browsed through the comments again.
Can you tell me specifically which one has not been addressed?

romanovvlad · 2019-11-01T11:38:57Z

I see that there are still several unresolved comment from bjoernknafla .

I'm unable to find any that has not been addressed. I browsed through the comments again.
Can you tell me specifically which one has not been addressed?

Ok, now I'm not sure if there are still unresolved comments. I don't know how to track it correctly because it seems "reviewable" doesn't add comments to github conversations.

garimagu · 2019-11-04T17:58:00Z

I see that there are still several unresolved comment from bjoernknafla .

I'm unable to find any that has not been addressed. I browsed through the comments again.
Can you tell me specifically which one has not been addressed?

Ok, now I'm not sure if there are still unresolved comments. I don't know how to track it correctly because it seems "reviewable" doesn't add comments to github conversations.

Yes, it would be better to use reviewable to see the comments. It has become a little confusing now.
Can you comment if something else is needed? I can update and upload my next patches after this is checked in.

romanovvlad · 2019-11-05T07:51:51Z

I see that there are still several unresolved comment from bjoernknafla .

I'm unable to find any that has not been addressed. I browsed through the comments again.
Can you tell me specifically which one has not been addressed?

Ok, now I'm not sure if there are still unresolved comments. I don't know how to track it correctly because it seems "reviewable" doesn't add comments to github conversations.

Yes, it would be better to use reviewable to see the comments. It has become a little confusing now.
Can you comment if something else is needed? I can update and upload my next patches after this is checked in.

I cannot find answer for the conversation starting with:

Why are we "die" here? Shouldn't we just return saying "no opencl plugins/platforms" available to the caller?

BTW. I would expect @smaslov-intel to make final approve.

garimagu

Reviewable status: 4 of 9 files reviewed, 33 unresolved discussions (waiting on @bjoernknafla, @garimagu, @ilyastepykin, @keryell, @romanovvlad, @Ruyk, and @smaslov-intel)

sycl/source/detail/pi.cpp, line 107 at r9 (raw file):

Previously, romanovvlad (Romanov Vlad) wrote…

No I mean just return an error. SYCL RT can continue working with other plugins.

The TODO above states that the code will iterate over all available plugins. We will expand this as the code is further matured.

- created a plugin as a shared plugin: lib/libpi_opencl.so. Moved the sources to new location plugins/opencl. - removed the dependency on pi.cpp by pi_opencl.cpp - added the preliminary plugin recognition mechanism and loading the plugin as a shared object using dlopen/dlsym etc. - Simplified CMakeLists.txt file : Use a function to call common code in source/CMakeLists.txt and plugins/opencl/CMakeLists.txt. This change allows extension of other plugins to be easier and maintaining the options at a single location. Signed-off-by: Garima Gupta <[email protected]>

smaslov-intel

Reviewed 2 of 3 files at r8, 2 of 2 files at r10, 1 of 1 files at r11.
Reviewable status: all files reviewed, 32 unresolved discussions (waiting on @bjoernknafla, @garimagu, @ilyastepykin, @keryell, @romanovvlad, and @Ruyk)

garimagu · 2019-11-08T18:02:08Z

@romanovvlad @smaslov-intel Does this look good to be merged?

smaslov-intel · 2019-11-08T18:04:34Z

This looks good to me, and I had already approved it.

garimagu mentioned this pull request Oct 8, 2019

[SYCL][DOC] Update the SYCL Runtime Interface document to give some design updates #680

Merged

keryell reviewed Oct 9, 2019

View reviewed changes

romanovvlad self-requested a review October 9, 2019 08:46

romanovvlad assigned smaslov-intel Oct 9, 2019

smaslov-intel reviewed Oct 9, 2019

View reviewed changes

Ruyk reviewed Oct 11, 2019

View reviewed changes

bjoernknafla reviewed Oct 14, 2019

View reviewed changes

garimagu mentioned this pull request Oct 15, 2019

[SYCL] Plugin Interface Changes to query a list of function pointers. #731

Closed

keryell previously approved these changes Oct 15, 2019

View reviewed changes

bjoernknafla reviewed Oct 15, 2019

View reviewed changes

garimagu dismissed keryell’s stale review via f9d34d9 October 15, 2019 19:14

smaslov-intel suggested changes Oct 16, 2019

View reviewed changes

smaslov-intel reviewed Oct 16, 2019

View reviewed changes

ilyastepykin reviewed Oct 16, 2019

View reviewed changes

bjoernknafla reviewed Oct 16, 2019

View reviewed changes

bjoernknafla mentioned this pull request Oct 16, 2019

[SYCL][PI] Design PI plugin lookup to be reusable in tools and tests #741

Closed

garimagu force-pushed the private/garimagu/pi_discovery branch from f9d34d9 to ad823a0 Compare October 17, 2019 00:31

garimagu commented Oct 17, 2019

View reviewed changes

ilyastepykin reviewed Oct 17, 2019

View reviewed changes

smaslov-intel previously approved these changes Oct 17, 2019

View reviewed changes

garimagu dismissed smaslov-intel’s stale review via db5008f October 18, 2019 17:50

garimagu force-pushed the private/garimagu/pi_discovery branch from ad823a0 to db5008f Compare October 18, 2019 17:50

bjoernknafla reviewed Oct 18, 2019

View reviewed changes

keryell previously approved these changes Oct 23, 2019

View reviewed changes

bader requested a review from smaslov-intel October 27, 2019 11:58

romanovvlad reviewed Oct 31, 2019

View reviewed changes

garimagu dismissed keryell’s stale review via beb9b45 October 31, 2019 18:33

garimagu commented Oct 31, 2019

View reviewed changes

keryell previously approved these changes Nov 4, 2019

View reviewed changes

garimagu mentioned this pull request Nov 5, 2019

[SYCL] Changing all PI APIs to the standard format of returning pi_result #776

Merged

garimagu commented Nov 5, 2019

View reviewed changes

garimagu dismissed keryell’s stale review via 884459b November 5, 2019 23:08

garimagu force-pushed the private/garimagu/pi_discovery branch from beb9b45 to 884459b Compare November 5, 2019 23:08

smaslov-intel approved these changes Nov 7, 2019

View reviewed changes

kbobrovs merged commit c4eafea into intel:sycl Nov 8, 2019

AlexeySachkov mentioned this pull request Nov 12, 2019

pi_die: Check if plugin is present. Failed to load plugin: libpi_opencl.so #814

Closed

	bool findPlugin(std::string &PluginPath) {
	std::string findPlugin() {

	// library, Implementation is OS dependent.
	// library, implementation is OS dependent.

[SYCL] Plugin Interface And Creation of OpenCL Plugin. #708

[SYCL] Plugin Interface And Creation of OpenCL Plugin. #708

Conversation

garimagu commented Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garimagu Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ruyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garimagu Oct 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garimagu commented Oct 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjoernknafla Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

bjoernknafla Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaslov-intel left a comment

Choose a reason for hiding this comment

smaslov-intel left a comment

Choose a reason for hiding this comment

smaslov-intel commented Oct 16, 2019

Choose a reason for hiding this comment

bjoernknafla left a comment

garimagu commented Oct 8, 2019 •

edited

Loading

garimagu Oct 11, 2019 •

edited

Loading

garimagu Oct 12, 2019 •

edited

Loading

bjoernknafla Oct 15, 2019 •

edited

Loading

bjoernknafla Oct 15, 2019 •

edited

Loading