[SYCL][DOC] Update the SYCL Runtime Interface document to give some design updates #680
Conversation
|
Please review only the last commit. The first commit is currently under review @ #206 |
garimagu
force-pushed
the
private/garimagu/pi_doc_update
branch
from
5ac85af to
6137dea
Compare
garimagu
changed the title
sycl/doc/SYCLPluginInterface.md
Outdated
| @@ -1,5 +1,4 @@ | |||
| # The SYCL Runtime Plugin Interface. | |||
|
|
|||
| #The SYCL Runtime Plugin Interface. | |||
There was a problem hiding this comment.
I think "Plugin interface" conflicts with the parent term. I suggest "The SYCL runtime plugin loader interface"
There was a problem hiding this comment.
Or should it be Plugin Loader only?
Plugin Loader would refer to the loading mechanism of the plugins.
After the plugin is loaded, we are using the interface to communicate with the plugin.
There was a problem hiding this comment.
Plugin Loader is ok with me too.
There was a problem hiding this comment.
changing it to Plugin Loader and Plugin Interface
There was a problem hiding this comment.
I am not using Plugin Loader in the description anywhere. Let me know if you feel it must be present.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @garimagu, @kbobrovs, and @romanovvlad)
sycl/doc/SYCLPluginInterface.md, line 48 at r1 (raw file):
Previously, smaslov-intel wrote…
Yes, this is what it would be in more details:
A configuration file with some predefined (TBD) location inside SYCL RT will be listing the names of the PI plugin libraries to search for and load. The search is made in paths given by LD_LIBRARY_PATH. Also, to address the issue of read-only configuration file, there will be an environment variable added (e.g. SYCL_PI_CONFIG) to provide a user-specified config file, which, if given, would override the predefined location inside SYCL RT.
@garimagu, please update the doc here
sycl/doc/SYCLPluginInterface.md, line 77 at r1 (raw file):
Previously, smaslov-intel wrote…
Could you elaborate more on this cons?
If we add extra stuff into PI handles, then there is more work to extract native OpenCL handle from a PI handle. Note that today most of PI handles returned by PI OpenCL plugin are exactly the OpenCL native raw handles.
We are strongly in favor of approach 2, which is to keep/cache the PI plugin descriptors in the SYCL RT, not in the PI handles.
@garimagu, please update the doc to clarify how we are doing here
|
Sorry for the confusion Konst. I haven’t uploaded the changes yet.
Still looking at all the comments and addressing each one by one in my local workspace. Will update the PR, in a few more mins. Maybe I should start a review instead to avoid such confsions.
From: kbobrovs <notifications@github.com>
Sent: Tuesday, October 29, 2019 12:15 PM
To: intel/llvm <llvm@noreply.github.com>
Cc: Gupta, Garima <garima.gupta@intel.com>; Mention <mention@noreply.github.com>
Subject: Re: [intel/llvm] [SYCL][DOC] Update the SYCL Runtime Interface document to give some design updates (#680)
@kbobrovs commented on this pull request.
________________________________
In sycl/doc/SYCLPluginInterface.md<#680 (comment)>:
@@ -41,12 +40,45 @@ once before any actual offload is attempted.
### Plugin discovery
…-Plugins are physically dynamic libraries stored somewhere in the system where
-the SYCL runtime runs. TBD - design and describe the process in details.
+Plugins are physically dynamic libraries or shared objects.
+The process to discover plugins will follow the following guidelines.
+
+The SYCL Runtime will search for plugins at env LD_LIBRARY_PATH location on
+Linux and env LIB on Windows, with names in the format of libpiXXX.so and
+query them. An extension to the search mechanism is to use a configuration file
+which lists all the available plugins and their locations.
+This file can be stored along with SYCL Runtime.
+Plugins should expose the information of supported PI API version as a constant
+string, so it can be read without loading the library.
@garimagu<https://github.com/garimagu> , my impression was that we agreed that SYCL plugin loader does not search for libpi* files in LD_LIBRARY_PATH, but takes the names of the plugin files from the configuration file which must be present. LD_LIBRARY_PATH is still used as a search path for these, though. If you agree - please update the doc.
There were couple more suggestions, which I think can be implemented later atop:
It would be good if it would not be necessary to add plugin directories to LD_LIBRARY_PATH (on Linux, nor PATH on Windows) for the runtime to use the plugins packaged with it.
this one maybe implemented either as an absolute path in the config file or as searching in some predefined directory (like /plugins)
And the other one:
On Linux an DT_RUNPATH can be embedded into the SYCL library for dlopen to look relative to the location of the SYCL library for plugin libraries
There should also be environment variables overriding any of the above.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#680?email_source=notifications&email_token=ALZDQ6VNCPBE246VMABJSN3QRCDRZA5CNFSM4I3ANOHKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJTXCEA#discussion_r340275597>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ALZDQ6X4BQWS5A7PLLF745TQRCDRZANCNFSM4I3ANOHA>.
|
|
No problem, Garima. Anyway I tried to summarize my view of the consensus – maybe that can help you with update
-- Konst
From: Garima Gupta <notifications@github.com>
Sent: Tuesday, October 29, 2019 12:19 PM
To: intel/llvm <llvm@noreply.github.com>
Cc: Bobrovsky, Konstantin S <konstantin.s.bobrovsky@intel.com>; Mention <mention@noreply.github.com>
Subject: Re: [intel/llvm] [SYCL][DOC] Update the SYCL Runtime Interface document to give some design updates (#680)
Sorry for the confusion Konst. I haven’t uploaded the changes yet.
Still looking at all the comments and addressing each one by one in my local workspace. Will update the PR, in a few more mins. Maybe I should start a review instead to avoid such confsions.
From: kbobrovs <notifications@github.com>
Sent: Tuesday, October 29, 2019 12:15 PM
To: intel/llvm <llvm@noreply.github.com>
Cc: Gupta, Garima <garima.gupta@intel.com>; Mention <mention@noreply.github.com>
Subject: Re: [intel/llvm] [SYCL][DOC] Update the SYCL Runtime Interface document to give some design updates (#680)
@kbobrovs commented on this pull request.
________________________________
In sycl/doc/SYCLPluginInterface.md<#680 (comment)>:
@@ -41,12 +40,45 @@ once before any actual offload is attempted.
### Plugin discovery
…-Plugins are physically dynamic libraries stored somewhere in the system where
-the SYCL runtime runs. TBD - design and describe the process in details.
+Plugins are physically dynamic libraries or shared objects.
+The process to discover plugins will follow the following guidelines.
+
+The SYCL Runtime will search for plugins at env LD_LIBRARY_PATH location on
+Linux and env LIB on Windows, with names in the format of libpiXXX.so and
+query them. An extension to the search mechanism is to use a configuration file
+which lists all the available plugins and their locations.
+This file can be stored along with SYCL Runtime.
+Plugins should expose the information of supported PI API version as a constant
+string, so it can be read without loading the library.
@garimagu<https://github.com/garimagu> , my impression was that we agreed that SYCL plugin loader does not search for libpi* files in LD_LIBRARY_PATH, but takes the names of the plugin files from the configuration file which must be present. LD_LIBRARY_PATH is still used as a search path for these, though. If you agree - please update the doc.
There were couple more suggestions, which I think can be implemented later atop:
It would be good if it would not be necessary to add plugin directories to LD_LIBRARY_PATH (on Linux, nor PATH on Windows) for the runtime to use the plugins packaged with it.
this one maybe implemented either as an absolute path in the config file or as searching in some predefined directory (like /plugins)
And the other one:
On Linux an DT_RUNPATH can be embedded into the SYCL library for dlopen to look relative to the location of the SYCL library for plugin libraries
There should also be environment variables overriding any of the above.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#680?email_source=notifications&email_token=ALZDQ6VNCPBE246VMABJSN3QRCDRZA5CNFSM4I3ANOHKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJTXCEA#discussion_r340275597>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ALZDQ6X4BQWS5A7PLLF745TQRCDRZANCNFSM4I3ANOHA>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#680?email_source=notifications&email_token=AFSNGMV6FGD3W6ZTU3AIR4TQRCEB7A5CNFSM4I3ANOHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECRYJZA#issuecomment-547587300>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFSNGMWB2FELBXTYCVLLHODQRCEB7ANCNFSM4I3ANOHA>.
|
garimagu
force-pushed
the
private/garimagu/pi_doc_update
branch
from
6137dea to
3334bce
Compare
bader
requested review from
kbobrovs,
romanovvlad,
sergey-semenov and
smaslov-intel
|
Off-topic: don't we want to move design doc/FAQ doc/etc to project wiki page in github? |
Why would we want this move? Could you elaborate when/how docs are moved? One big disadvantage of wiki is that you can't review changes before they are applied. |
|
If you think it's important topic to discuss, please, open a separate issue. |
|
@romanovvlad, @sergey-semenov, @smaslov-intel, ping. |
sycl/doc/SYCLPluginInterface.md
Outdated
| OpenCL plugin is a usual plugin from SYCL runtime standpoint, but its loading | ||
| and initialization involves a nested discovery process which finds out available | ||
| OpenCL implementations. They can be installed either in the standard Khronos | ||
| ICD-compatible way (e.g. listed in files under /etc/OpenCL/vendors on | ||
| Linux) or not, and the OpenCL plugin can hook up with both. | ||
|
|
||
| TBD describe the nested OpenCL implementation discovery process performed by | ||
| TBD - describe the nested OpenCL implementation discovery process performed by | ||
| the OpenCL plugin | ||
|
|
||
| ### Device enumeration by plugins |
There was a problem hiding this comment.
To me section name is misleading as content doesn't describe "device enumeration".
There was a problem hiding this comment.
I have still kept this section name. Do you have a suggested alternative?
sycl/doc/SYCLPluginInterface.md
Outdated
| the PI API calls needed to run the program to the selected plugin. | ||
| The selected plugin information for a device is stored in the associated | ||
| platform during platform object construction. The API calls are arbitrated | ||
| by using this information. |
There was a problem hiding this comment.
General comment.
I think the doc should cover relationship between sycl::platform, sycl::device, PI plugin, cl_platform_id, cl_device_id.
For example, it's not very clear if we can have multiple sycl::platform's with [the same|different set] of devices dispatching to the same PI plugin.
There was a problem hiding this comment.
We can do this with the help of an example in the future.
What happens when we have the same device that can be connected with multiple platforms?
How would a program be expected to behave in that case?
Eg: A GPU that can be communicated with using OpenCL and a proprietary run time.
There was a problem hiding this comment.
What happens when we have the same device that can be connected with multiple platforms?
It should be OK as an application works with software abstraction of real device. So, it's OK to have several "soft" devices that map to one real device unless there is no bugs/features in the underlying runtimes that prevent such model.
There was a problem hiding this comment.
General comment.
I think the doc should cover relationship between sycl::platform, sycl::device, PI plugin, cl_platform_id, cl_device_id.
For example, it's not very clear if we can have multiple sycl::platform's with [the same|different set] of devices dispatching to the same PI plugin.
Hi Vlad, can you suggest where and how should such details be included? Aren't these more implementation specific?
sycl/doc/SYCLPluginInterface.md
Outdated
|
|
||
| TBD | ||
| TBD - Unloading a bound plugin. |
There was a problem hiding this comment.
I believe it's very important aspect which can affect design.
There was a problem hiding this comment.
Is it really? We are loading the plugin as a library. We can unload it in platform obj destructor.
There was a problem hiding this comment.
No, we cannot. There can be several platform objects using the same "plugin". Furthermore new platform object can be created after old one is destructed.
garimagu
force-pushed
the
private/garimagu/pi_doc_update
branch
from
3334bce to
d48cf7b
Compare
|
Please expect minimum 2 more patches associated with Plugin Interface:
|
|
TODO: After #1030 is merged add the corresponding changes to this doc. |
1 similar comment
|
@garimagu, could you update the PR, please? |
Signed-off-by: Garima Gupta <garima.gupta@intel.com> [SYCL] Addition of recommended design changes Signed-off-by: Garima Gupta <garima.gupta@intel.com> Suggested minor changes. Signed-off-by: Garima Gupta <garima.gupta@intel.com>
garimagu
force-pushed
the
private/garimagu/pi_doc_update
branch
from
d48cf7b to
81329ea
Compare
|
@bader @kbobrovs @smaslov-intel Please review the updated document. |
Signed-off-by: Garima Gupta <garima.gupta@intel.com>
garimagu
force-pushed
the
private/garimagu/pi_doc_update
branch
from
81329ea to
8ed0146
Compare
|
@smaslov-intel @romanovvlad Please review as and when you get sometime. |
…versioning * origin/sycl: [XPTI][Framework] Reference implementation of the Xpti framework to be used with instrumentation in SYCL (intel#1557) [SYCL] Initial ABI checks implementation (intel#1528) [SYCL] Support connection with multiple plugins (intel#1490) [SYCL] Add a new header file with the reduction class definition (intel#1558) [SYCL] Add test for SYCL kernels with accessor and spec constant (intel#1536) [SYCL][CUDA] Move interop tests (intel#1570) [Driver][SYCL] Remove COFF object format designator for Windows device compiles (intel#1574) [SYCL] Fix conflicting visibility attributes (intel#1571) [SYCL][DOC] Update the SYCL Runtime Interface document with design details (intel#680) [SYCL] Improve image accessors support on a host device (intel#1502) [SYCL] Make queue's non-USM event ownership temporary (intel#1561) [SYCL] Added support of rounding modes for non-host devices (intel#1463) [SYCL] SemaSYCL significant refactoring (intel#1517) [SYCL] Support 0-dim accessor in handler::copy(accessor, accessor) (intel#1551)
A preliminary design on the plugin interface.
The design is still under review, feel free to suggest changes.