Skip to content

[SYCL] Reuse plugin information so that it's not copied when sycl::detail::plugin is copied #5049

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

Merged
merged 7 commits into from
Dec 2, 2021
Merged

[SYCL] Reuse plugin information so that it's not copied when sycl::detail::plugin is copied #5049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec01783
[SYCL] Reuse plugin information so that it's not copied when sycl::de…
Nov 30, 2021
b23932a
Fix style issue
Nov 30, 2021
3be51ed
Revert unwanted change
Nov 30, 2021
27d56f1
Reword plugn information allocation
Nov 30, 2021
cee4953
Fix build
Nov 30, 2021
4d568ab
Fix style issue
Nov 30, 2021
40033fa
Fix re-using the same plugin info instance for different plugins
Dec 1, 2021
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
15 changes: 8 additions & 7 deletions sycl/source/detail/pi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,14 +337,15 @@ int unloadPlugin(void *Library) { return unloadOsLibrary(Library); }
// call is done to get all Interface API mapping. The plugin interface also
// needs to setup infrastructure to route PI_CALLs to the appropriate plugins.
// Currently, we bind to a singe plugin.
bool bindPlugin(void *Library, PiPlugin *PluginInformation) {
bool bindPlugin(void *Library,
const std::shared_ptr<PiPlugin> &PluginInformation) {

decltype(::piPluginInit) *PluginInitializeFunction = (decltype(
&::piPluginInit))(getOsLibraryFuncAddress(Library, "piPluginInit"));
if (PluginInitializeFunction == nullptr)
return false;

int Err = PluginInitializeFunction(PluginInformation);
int Err = PluginInitializeFunction(PluginInformation.get());

// TODO: Compare Supported versions and check for backward compatibility.
// Make sure err is PI_SUCCESS.
Expand Down Expand Up @@ -378,11 +379,11 @@ static void initializePlugins(std::vector<plugin> &Plugins) {
std::cerr << "SYCL_PI_TRACE[all]: "
<< "No Plugins Found." << std::endl;

PiPlugin PluginInformation{
_PI_H_VERSION_STRING, _PI_H_VERSION_STRING, nullptr, {}};
PluginInformation.PiFunctionTable = {};

for (unsigned int I = 0; I < PluginNames.size(); I++) {
std::shared_ptr<PiPlugin> PluginInformation = std::make_shared<PiPlugin>(
PiPlugin{_PI_H_VERSION_STRING, _PI_H_VERSION_STRING,
/*Targets=*/nullptr, /*FunctionPointers=*/{}});

void *Library = loadPlugin(PluginNames[I].first);

if (!Library) {
Expand All @@ -395,7 +396,7 @@ static void initializePlugins(std::vector<plugin> &Plugins) {
continue;
}

if (!bindPlugin(Library, &PluginInformation)) {
if (!bindPlugin(Library, PluginInformation)) {
if (trace(PI_TRACE_ALL)) {
std::cerr << "SYCL_PI_TRACE[all]: "
<< "Failed to bind PI APIs to the plugin: "
Expand Down
23 changes: 14 additions & 9 deletions sycl/source/detail/plugin.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ auto packCallArguments(ArgsT &&... Args) {
class plugin {
public:
plugin() = delete;
plugin(RT::PiPlugin Plugin, backend UseBackend, void *LibraryHandle)
plugin(const std::shared_ptr<RT::PiPlugin> &Plugin, backend UseBackend,
void *LibraryHandle)
: MPlugin(Plugin), MBackend(UseBackend), MLibraryHandle(LibraryHandle),
TracingMutex(std::make_shared<std::mutex>()),
MPluginMutex(std::make_shared<std::mutex>()) {}
Expand All @@ -101,8 +102,11 @@ class plugin {

~plugin() = default;

const RT::PiPlugin &getPiPlugin() const { return MPlugin; }
RT::PiPlugin &getPiPlugin() { return MPlugin; }
const RT::PiPlugin &getPiPlugin() const { return *MPlugin; }
RT::PiPlugin &getPiPlugin() { return *MPlugin; }
const std::shared_ptr<RT::PiPlugin> &getPiPluginPtr() const {
return MPlugin;
}

/// Checks return value from PI calls.
///
Expand Down Expand Up @@ -148,29 +152,30 @@ class plugin {
uint64_t CorrelationID = pi::emitFunctionBeginTrace(PIFnName);
auto ArgsData =
packCallArguments<PiApiOffset>(std::forward<ArgsT>(Args)...);
uint64_t CorrelationIDWithArgs = pi::emitFunctionWithArgsBeginTrace(
static_cast<uint32_t>(PiApiOffset), PIFnName, ArgsData.data(), MPlugin);
uint64_t CorrelationIDWithArgs =
pi::emitFunctionWithArgsBeginTrace(static_cast<uint32_t>(PiApiOffset),
PIFnName, ArgsData.data(), *MPlugin);
#endif
RT::PiResult R;
if (pi::trace(pi::TraceLevel::PI_TRACE_CALLS)) {
std::lock_guard<std::mutex> Guard(*TracingMutex);
const char *FnName = PiCallInfo.getFuncName();
std::cout << "---> " << FnName << "(" << std::endl;
RT::printArgs(Args...);
R = PiCallInfo.getFuncPtr(MPlugin)(Args...);
R = PiCallInfo.getFuncPtr(*MPlugin)(Args...);
std::cout << ") ---> ";
RT::printArgs(R);
RT::printOuts(Args...);
std::cout << std::endl;
} else {
R = PiCallInfo.getFuncPtr(MPlugin)(Args...);
R = PiCallInfo.getFuncPtr(*MPlugin)(Args...);
}
#ifdef XPTI_ENABLE_INSTRUMENTATION
// Close the function begin with a call to function end
pi::emitFunctionEndTrace(CorrelationID, PIFnName);
pi::emitFunctionWithArgsEndTrace(CorrelationIDWithArgs,
static_cast<uint32_t>(PiApiOffset),
PIFnName, ArgsData.data(), R, MPlugin);
PIFnName, ArgsData.data(), R, *MPlugin);
#endif
return R;
}
Expand Down Expand Up @@ -236,7 +241,7 @@ class plugin {
std::shared_ptr<std::mutex> getPluginMutex() { return MPluginMutex; }

private:
RT::PiPlugin MPlugin;
std::shared_ptr<RT::PiPlugin> MPlugin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we have one to one mapping between plugin and RT::PiPlugin and one to many between plugin and platform ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Though, adding such a change to this patch will make it more complex for review.
Shifting to 1-1 plugin-pi_plugin and 1-n plugin-platform is the next step.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we already have 1:many mapping to platforms at line 253?

Copy link
Contributor Author

@s-kanaev s-kanaev Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@smaslov-intel , there's such a mapping between sycl::detail::plugin and pi_platform. Though, I have an intent to add the same mapping for sycl::detail::plugin and sycl::detail::platform_impl as opposed to copying sycl::detail::plugin for each platform.

backend MBackend;
void *MLibraryHandle; // the handle returned from dlopen
std::shared_ptr<std::mutex> TracingMutex;
Expand Down
2 changes: 1 addition & 1 deletion sycl/unittests/helpers/PiMock.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class PiMock {
// Copy the PiPlugin, thus untying our to-be mock platform from other
// platforms within the context. Reset our platform to use the new plugin.
auto NewPluginPtr = std::make_shared<detail::plugin>(
OriginalPiPlugin.getPiPlugin(), OriginalPiPlugin.getBackend(),
OriginalPiPlugin.getPiPluginPtr(), OriginalPiPlugin.getBackend(),
OriginalPiPlugin.getLibraryHandle());
ImplPtr->setPlugin(NewPluginPtr);
// Extract the new PiPlugin instance by a non-const pointer,
Expand Down