Skip to content

[SYCL] Load PI plugins only once #1614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[SYCL] Load PI plugins only once #1614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9584ea2
[SYCL] Load PI plugins only once
smaslov-intel Apr 29, 2020
3009338
Merge branch 'sycl' of https://github.com/intel/llvm into pionce
smaslov-intel Jun 7, 2020
5a3df8f
[SYCL] Load PI plugins only once. Make pi::initialize return const re…
smaslov-intel Jun 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sycl/include/CL/sycl/detail/pi.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ template <class To, class From> To cast(From value);
extern std::shared_ptr<plugin> GlobalPlugin;

// Performs PI one-time initialization.
vector_class<plugin> initialize();
const vector_class<plugin> &initialize();

// Utility Functions to get Function Name for a PI Api.
template <PiApiKind PiApiOffset> struct PiFuncInfo {};
Expand Down
16 changes: 14 additions & 2 deletions sycl/source/detail/pi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,19 @@ bool trace(TraceLevel Level) {
}

// Initializes all available Plugins.
vector_class<plugin> initialize() {
vector_class<plugin> Plugins;
// The returned reference lifetime is through the end of the process.
//
// TODO: make sure that the global Plugins is destroyed last, at least
// after other tear-down relying on plugins (like destructors of global
// buffers) is completed.
//
const vector_class<plugin> &initialize() {
static bool PluginsInitDone = false;
static vector_class<plugin> Plugins;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.

  2. 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(...);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (PluginsInitDone) {
return Plugins;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


vector_class<std::pair<std::string, backend>> PluginNames;
findPlugins(PluginNames);

Expand Down Expand Up @@ -310,6 +321,7 @@ vector_class<plugin> initialize() {
<< "Plugin found and successfully loaded: "
<< PluginNames[I].first << std::endl;
}
PluginsInitDone = true;

#ifdef XPTI_ENABLE_INSTRUMENTATION
if (!(xptiTraceEnabled() && !XPTIInitDone))
Expand Down
2 changes: 1 addition & 1 deletion sycl/source/detail/platform_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ static bool IsBannedPlatform(platform Platform) {

vector_class<platform> platform_impl::get_platforms() {
vector_class<platform> Platforms;
vector_class<plugin> Plugins = RT::initialize();
const vector_class<plugin> &Plugins = RT::initialize();

info::device_type ForcedType = detail::get_forced_type();
for (unsigned int i = 0; i < Plugins.size(); i++) {
Expand Down