[SYCL] Plugin Interface: Single Call to Plugin to populate a Plugin Datastructure. #808

garimagu · 2019-11-08T22:46:56Z

[SYCL] Using a single call to Plugin, and populating a Plugin
datastructure with all function pointers. Extension to this will allow
us to bind multiple plugins.
[SYCL] Changing all the PI_CALL(RT:: or PI_CALL_RESULT(RT:: to
PI_CALL( or PI_CALL_RESULT( and other required changes.

Two last commits represent this change.
They are built over the previous PR#776, PR#798 .

garimagu · 2019-11-12T20:09:45Z

rebased and removed conflicts over PR#798.
Valid commits to be checked for this review are the last two commits.

garimagu · 2019-11-15T02:33:16Z

Rebased over PR#798. and removed conflicts after merge of PR#776.
Valid commits are the last two commits.

garimagu · 2019-11-15T19:33:32Z

rebase.

garimagu · 2019-11-18T23:58:51Z

@smaslov-intel Please review.

datastructure with all function pointers. Extension to this will allow us to bind multiple plugins. Signed-off-by: Garima Gupta <[email protected]>

PI_CALL( or PI_CALL_RESULT( and other required changes. Signed-off-by: Garima Gupta <[email protected]>

garimagu · 2019-11-19T00:03:48Z

rebase,clang-format and conflict resolution.

bjoernknafla · 2019-11-19T19:34:03Z

sycl/include/CL/sycl/buffer.hpp

-        detail::pi::cast<detail::RT::PiMem>(MemObject), CL_MEM_SIZE,
-        sizeof(size_t), &BufSize, nullptr);
+    PI_CALL(piMemGetInfo, detail::pi::cast<detail::RT::PiMem>(MemObject),
+            CL_MEM_SIZE, sizeof(size_t), &BufSize, nullptr);


Only noticing this now - this somehow reminds me of Lisp 😄

I have never used Lisp. Are you talking about use of comma after the api name?
I have the call interface changed in #843 . Do you think that works?
Maybe I can close this PR and merge the two PRs together.
Let me know what you think.

Oh, sorry for the confusion and the extra work that I now caused - I meant "Lisp" as a positive thing 😿

Both approaches seem ok to me, the one in this PR seems to require less state in the trace class though.

That's true. The other way makes it more like calling a constructor and a functor on it.

bjoernknafla · 2019-11-19T19:46:35Z

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

+  // checks and writes the appropriate Function Pointers in
+  // PIFunctionTable.
+  const char PiVersion[4] = "1.1";
+  char PluginVersion[4] = "1.1"; // Plugin edits this.


Will the fixed-sized array be a problem?

Hmmm.. I will a todo for this problem. I think this is a different problem to be resolved at a later point.

I have added a TODO like below:
// TODO: Work on version fields and their handshaking mechanism.
// Some choices are:
// - Use of integers to keep major and minor version.
// - Keeping char* Versions.

in commit # 5 in PR 843

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

sycl/unittests/pi/PlatformTest.cpp

bjoernknafla · 2019-11-19T20:06:33Z

sycl/plugins/opencl/pi_opencl.cpp

-// TODO: Remove the 'OclPtr' extension used with the PI_APIs.
-// Forward calls to OpenCL RT.
+pi_result piPluginInit(pi_plugin *PluginInit) {
+  strcpy(PluginInit->PluginVersion, SupportedVersion);


As the version array is of fixed size the use of strncpy would be safer here.

Wondering though if it would be ok to make PluginInit->PluginVersion a const char* to point to the version string literal in this file?

Hmmm..
I thought keeping all memory allocation and destruction in Plugin Interface will be a better idea. The library is loaded and it populates the fields.
I will take the focus off the version discussion and add a TODO for future.

What I mean it, if the plugin cpp file defines the version as a string literal, then piPluginInit could just point PluginInit->PluginVersion to the string literal. No copying required and therefore no memory management.

There might be intricacies with the plugin unloading though which could leave a dangling pointer but my guess is that an unloaded plugin means that most SYCL calls become unsafe anyway.

I will add a TODO. I can create a separate patch for the version discussion and changes. It will help segregate this discussion and keep the progress going.

bjoernknafla · 2019-11-19T20:07:56Z

sycl/plugins/opencl/pi_opencl.cpp

+pi_result piPluginInit(pi_plugin *PluginInit) {
+  strcpy(PluginInit->PluginVersion, SupportedVersion);
+  int CompareVersions = strcmp(PluginInit->PiVersion, SupportedVersion);
+  if (CompareVersions < 0) {


If this comparison fails we already changed the PluginInit->PluginVersion. Should we move the comparison before the version is set in the plugin?

Yes. That was the intention I initially had. For the Plugin Interface to know that the plugin supported some other Version and maybe take an action for it.

Changed it now.

Is that commit not pushed yet as I cannot see a change in the code?

It is their in 4ce1cc4.

Thank you! 👍

bjoernknafla · 2019-11-19T20:32:58Z

sycl/source/detail/pi.cpp

+
+  decltype(::piPluginInit) *PluginInitializeFunction = (decltype(
+      &::piPluginInit))(getOsLibraryFuncAddress(Library, "piPluginInit"));
+  int err = PluginInitializeFunction(&PluginInformation);


I would implement the init function to not affect the argument if an error occurs. If we need/want that at least the plugin version is set - even on an error - then please document this required behavior.

Though I would prefer to not access the argument if an error is returned in the code below.

I have removed piPluginInit function to edit the PluginInformation structure when an error occurs.

bjoernknafla · 2019-11-19T20:37:26Z

sycl/source/detail/pi.cpp

@@ -74,18 +78,21 @@ void *loadPlugin(const std::string &PluginPath) {
 // needs to setup infrastructure to route PI_CALLs to the appropriate plugins.
 // Currently, we bind to a singe plugin.
 bool bindPlugin(void *Library) {


Unsure about the calling-sites of this though passing an explicit plugin argument in would be clearer instead of changing a global variable.

That's true. I think we will have to do that once the plugin data structure becomes a part of a sycl object.

sycl/source/detail/pi.cpp

garimagu

Added the changes in commit 5 of PR#843.

sycl/unittests/pi/PlatformTest.cpp

garimagu · 2019-11-20T20:46:18Z

sycl/include/CL/sycl/buffer.hpp

-        detail::pi::cast<detail::RT::PiMem>(MemObject), CL_MEM_SIZE,
-        sizeof(size_t), &BufSize, nullptr);
+    PI_CALL(piMemGetInfo, detail::pi::cast<detail::RT::PiMem>(MemObject),
+            CL_MEM_SIZE, sizeof(size_t), &BufSize, nullptr);


That's true. The other way makes it more like calling a constructor and a functor on it.

garimagu · 2019-11-20T20:47:03Z

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

+  // checks and writes the appropriate Function Pointers in
+  // PIFunctionTable.
+  const char PiVersion[4] = "1.1";
+  char PluginVersion[4] = "1.1"; // Plugin edits this.


Hmmm.. I will a todo for this problem. I think this is a different problem to be resolved at a later point.

garimagu · 2019-11-20T20:51:18Z

sycl/plugins/opencl/pi_opencl.cpp

-// TODO: Remove the 'OclPtr' extension used with the PI_APIs.
-// Forward calls to OpenCL RT.
+pi_result piPluginInit(pi_plugin *PluginInit) {
+  strcpy(PluginInit->PluginVersion, SupportedVersion);


I will add a TODO. I can create a separate patch for the version discussion and changes. It will help segregate this discussion and keep the progress going.

garimagu · 2019-11-20T21:19:37Z

sycl/plugins/opencl/pi_opencl.cpp

+pi_result piPluginInit(pi_plugin *PluginInit) {
+  strcpy(PluginInit->PluginVersion, SupportedVersion);
+  int CompareVersions = strcmp(PluginInit->PiVersion, SupportedVersion);
+  if (CompareVersions < 0) {


Yes. That was the intention I initially had. For the Plugin Interface to know that the plugin supported some other Version and maybe take an action for it.

garimagu · 2019-11-20T21:22:50Z

sycl/plugins/opencl/pi_opencl.cpp

+pi_result piPluginInit(pi_plugin *PluginInit) {
+  strcpy(PluginInit->PluginVersion, SupportedVersion);
+  int CompareVersions = strcmp(PluginInit->PiVersion, SupportedVersion);
+  if (CompareVersions < 0) {


Changed it now.

garimagu · 2019-11-20T21:24:06Z

sycl/source/detail/pi.cpp

@@ -74,18 +78,21 @@ void *loadPlugin(const std::string &PluginPath) {
 // needs to setup infrastructure to route PI_CALLs to the appropriate plugins.
 // Currently, we bind to a singe plugin.
 bool bindPlugin(void *Library) {


That's true. I think we will have to do that once the plugin data structure becomes a part of a sycl object.

garimagu · 2019-11-20T21:24:11Z

sycl/source/detail/pi.cpp

+
+  decltype(::piPluginInit) *PluginInitializeFunction = (decltype(
+      &::piPluginInit))(getOsLibraryFuncAddress(Library, "piPluginInit"));
+  int err = PluginInitializeFunction(&PluginInformation);


garimagu · 2019-11-20T22:09:29Z

@bjoernknafla
Let me know if you are ok if I close this PR.
The review #843 includes this change, and you can review it there further. I have taken care of your comments in the 5th commit in the review.
Also, regarding the version comparison and on how to keep it, we can have a separate commit to have it better designed. Let me know if you think it is fine.

bjoernknafla

Not seeing some of the TODOs in the code - probably missing pushing the commits, otherwise ok.

garimagu · 2019-11-21T17:20:54Z

Not seeing some of the TODOs in the code - probably missing pushing the commits, otherwise ok.

I have incorporated them in the 5th commit of #843 . I should have given the link earlier.
4ce1cc4

garimagu · 2019-11-21T21:38:50Z

This PR is a part of PR #843 .
Closing this to avoid duplicated review effort.

garimagu force-pushed the private/garimagu/pi_functiontable branch 2 times, most recently from 9f11c42 to 234ef82 Compare November 12, 2019 20:07

smaslov-intel self-requested a review November 13, 2019 00:02

smaslov-intel self-assigned this Nov 13, 2019

garimagu force-pushed the private/garimagu/pi_functiontable branch from 234ef82 to 708fc25 Compare November 15, 2019 02:31

garimagu force-pushed the private/garimagu/pi_functiontable branch from 708fc25 to 5fb5ca1 Compare November 15, 2019 19:18

garimagu requested a review from romanovvlad November 15, 2019 19:18

garimagu force-pushed the private/garimagu/pi_functiontable branch from 5fb5ca1 to ade27b1 Compare November 15, 2019 19:45

garimagu added 2 commits November 18, 2019 15:59

[SYCL] Using a single call to Plugin, and populating a Plugin

a8e86c7

datastructure with all function pointers. Extension to this will allow us to bind multiple plugins. Signed-off-by: Garima Gupta <[email protected]>

[SYCL] Changing all the PI_CALL(RT:: or PI_CALL_RESULT(RT:: to

bc2305d

PI_CALL( or PI_CALL_RESULT( and other required changes. Signed-off-by: Garima Gupta <[email protected]>

garimagu force-pushed the private/garimagu/pi_functiontable branch from ade27b1 to bc2305d Compare November 19, 2019 00:02

bjoernknafla reviewed Nov 19, 2019

View reviewed changes

bjoernknafla suggested changes Nov 19, 2019

View reviewed changes

garimagu commented Nov 20, 2019

View reviewed changes

bjoernknafla approved these changes Nov 21, 2019

View reviewed changes

garimagu closed this Nov 21, 2019

[SYCL] Plugin Interface: Single Call to Plugin to populate a Plugin Datastructure. #808

[SYCL] Plugin Interface: Single Call to Plugin to populate a Plugin Datastructure. #808

Uh oh!

Conversation

garimagu commented Nov 8, 2019

Uh oh!

garimagu commented Nov 12, 2019

Uh oh!

garimagu commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garimagu commented Nov 15, 2019

Uh oh!

garimagu commented Nov 18, 2019

Uh oh!

garimagu commented Nov 19, 2019

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

garimagu 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

garimagu commented Nov 15, 2019 •

edited

Loading