[SYCL] Load PI plugins only once #1614

smaslov-intel · 2020-04-30T01:06:00Z

Stop re-loading PI plugins on every invocation of get_platforms().
Signed-off-by: Sergey V Maslov sergey.v.maslov@intel.com

Signed-off-by: Sergey V Maslov <sergey.v.maslov@intel.com>

garimagu

LGTM.
@romanovvlad In case you have comments on the usage of static keyword here, please comment.

sycl/source/detail/pi.cpp

romanovvlad · 2020-05-08T13:04:57Z

sycl/source/detail/pi.cpp

@@ -197,7 +197,12 @@ bool trace(TraceLevel Level) {

 // Initializes all available Plugins.
 vector_class<plugin> initialize() {
-  vector_class<plugin> Plugins;
+  static bool PluginsInitDone = false;
+  static vector_class<plugin> Plugins;


Who owns plugin? I thought it was platform_impl::MPlatform.
Related question:
What is the lifetime of Plugins? What happens if user creates a platform(to enter this function) from a global object destructor?

plugins are system-wide, think of them as a global list of shared libraries loaded. it is currently created at the first call to pi::initialize() which I think (TODO) we should fine a better place for than in platforms_get(), and also (TODO) deallocate them at global tear-down.

Still, who owns plugins right now? I see that platform_impl has std::shared_ptr<plugin> MPlugin; field, so I expect that platform_impl objects own plugins. When all platform_impl objects are destroyed plugin object will be deallocated as well, so Plugins vector will point to already released memory.

What happens if user creates a platform(to enter this function) from a global object destructor? For example the following SYCL application can(it's UB) catch a crash with your implementation:

class Glob { ~Glob() { sycl::platform Platform(...); } } Glob GVar; int main() { sycl::platform Platform(...); }

The question about the lifetime of the Plugins vector is interesting. I looked at the SYCL spec, and I don't see any restriction that prevents a user from calling SYCL APIs from global destructors, so I think we need to assume that code like @romanovvlad shows above is legal.

This probably means that it is generally unsafe to use any static storage or namespace scope C++ objects anywhere inside the SYCL runtime. Consider the static vector_class<plugin> Plugins added by this PR. The Plugins vector will be destroyed sometime after the application's main() function returns (or when exit() is called). However, a global destructor in the application could call SYCL APIs even after that. If such a call happens after Plugins is destroyed, the application will likely crash.

One way to solve this is to define a static pointer to vector_class instead. The initialize() function can call new to allocate the vector, with no matching delete. This means the memory will never be deallocated, but this is necessary if you want to support SYCL API calls even from global destructors. Of course, the problem might be wider than that this particular PR. There might be other code that defines static storage or namespace scope objects.

I also agree there is a problem caused by undefined order of global objects destruction. The pointer solution is an interesting one (especially for its simplicity), an is probably a suitable one where destructors have no important side effects.

An alternative would be to put some order to the objects destruction (there are C++ extensions to do so, e.g. GNU attribute "destructor" with priority: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html). Vlad mentioned there is already some llvm class like GlobalGlobal (not remembering the exact name), that are guaranteed to be destroyed after other globals.

I would prefer that we do not use global objects.

If it's really needed I would suggest having a global pointer solution which is deallocated in "GNU attribute "destructor"".

an is probably a suitable one where destructors have no important side effects.

Are you sure "plugins" have no important side effects? Shouldn't they at least dlclose a plugin library?

I would suggest having a global pointer solution which is deallocated in "GNU attribute "destructor""

Can we work on it separately from this change?
I added a TODO explaining that we need to destruct plugins last of all.

Are you sure "plugins" have no important side effects? Shouldn't they at least dlclose a plugin library?

I don't think the dlclose is necessary. Remember, this is happening right before the process terminates. The library will get implicitly dlclosed when the process termintes.

User can load libsycl.so using dlopen, pugins libraries will not be properly freed by dlclose.

romanovvlad · 2020-05-08T13:08:04Z

sycl/source/detail/pi.cpp

+  static vector_class<plugin> Plugins;
+  if (PluginsInitDone) {
+    return Plugins;
+  }


It seems it is possible for two threads to reach this point, so both accesses Plugins var. This will lead to problems.

Why? Plugins are really read-only immutable objects after they are constructed in pi::initilize(). We should probably make it explicitly such, so that people aren't able to write it.

If two threads create platforms for the first time simultaneously they can reach this point or I missing something?

…ference to the global vector of plugins, not a temporary copy of it. Change-Id: Ic1373059c9697b0b4d318548b79be7bcb9d4f2a9 Signed-off-by: Sergey V Maslov <sergey.v.maslov@intel.com>

smaslov-intel · 2020-06-10T16:30:06Z

Coming in #1868

The old mutateCallInst* functions and the new mutator doesn't quite get some of the value names correct, which necessitated some test changes that were looking for particular value names. Original commit: KhronosGroup/SPIRV-LLVM-Translator@9284a33

…alue pairs (intel#1614)

[SYCL] Load PI plugins only once

9584ea2

Signed-off-by: Sergey V Maslov <sergey.v.maslov@intel.com>

smaslov-intel requested a review from garimagu April 30, 2020 01:06

garimagu previously approved these changes May 1, 2020

View reviewed changes

keryell reviewed May 6, 2020

View reviewed changes

sycl/source/detail/pi.cpp Show resolved Hide resolved

bader requested a review from romanovvlad May 6, 2020 17:45

romanovvlad requested changes May 8, 2020

View reviewed changes

Merge branch 'sycl' of https://github.com/intel/llvm into pionce

3009338

smaslov-intel dismissed garimagu’s stale review via 9e8ad0e June 7, 2020 02:36

smaslov-intel requested a review from a team as a code owner June 7, 2020 02:36

smaslov-intel force-pushed the pionce branch from 9e8ad0e to f020b1c Compare June 7, 2020 02:45

smaslov-intel requested a review from romanovvlad June 7, 2020 02:45

smaslov-intel force-pushed the pionce branch 2 times, most recently from 5360681 to 3cb6d5c Compare June 7, 2020 02:58

[SYCL] Load PI plugins only once. Make pi::initialize return const re…

5a3df8f

…ference to the global vector of plugins, not a temporary copy of it. Change-Id: Ic1373059c9697b0b4d318548b79be7bcb9d4f2a9 Signed-off-by: Sergey V Maslov <sergey.v.maslov@intel.com>

smaslov-intel force-pushed the pionce branch from 3cb6d5c to 5a3df8f Compare June 8, 2020 02:26

bader mentioned this pull request Jun 10, 2020

Eliminated redundancy in plugin initialization. #1868

Merged

smaslov-intel closed this Jun 10, 2020

bader mentioned this pull request Jun 29, 2020

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

Closed

6 tasks

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Feb 23, 2023

[SYCL] Allow "=" sign as part of value in LIT_EXTRA_ENVIRONMENT key/v…

cf575a2

…alue pairs (intel#1614)

[SYCL] Load PI plugins only once #1614

[SYCL] Load PI plugins only once #1614

Uh oh!

Conversation

smaslov-intel commented Apr 30, 2020

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

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!

smaslov-intel commented Jun 10, 2020

Uh oh!

Uh oh!