Skip to content

[SYCL][PI] get backend type from Plugin, not env #1477

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 1 commit into from
Closed

[SYCL][PI] get backend type from Plugin, not env #1477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions sycl/include/CL/sycl/detail/pi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ using pi_uint64 = uint64_t;
using pi_bool = pi_uint32;
using pi_bitfield = pi_uint64;

// For selection of SYCL RT back-end, now manually through the "SYCL_BE"
// environment variable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the comment still apply? The environmental variable is not used anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, i will remove it. i just took the part from pi.hpp to get feedback on this

enum Backend { SYCL_BE_PI_OPENCL, SYCL_BE_PI_CUDA, SYCL_BE_PI_OTHER };

//
// NOTE: prefer to map 1:1 to OpenCL so that no translation is needed
// for PI <-> OpenCL ways. The PI <-> to other BE translation is almost
Expand Down Expand Up @@ -1346,6 +1350,8 @@ struct _pi_plugin {
const char PiVersion[4] = _PI_H_VERSION_STRING;
// Plugin edits this.
char PluginVersion[4] = _PI_H_VERSION_STRING;
// Plugin type
Backend backend;
char *Targets;
struct FunctionPointers {
#define _PI_API(api) decltype(::api) *api;
Expand Down
4 changes: 0 additions & 4 deletions sycl/include/CL/sycl/detail/pi.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,6 @@ void *loadOsLibrary(const std::string &Library);
// library, implementation is OS dependent.
void *getOsLibraryFuncAddress(void *Library, const std::string &FunctionName);

// For selection of SYCL RT back-end, now manually through the "SYCL_BE"
// environment variable.
enum Backend { SYCL_BE_PI_OPENCL, SYCL_BE_PI_CUDA, SYCL_BE_PI_OTHER };

// Check for manually selected BE at run-time.
bool useBackend(Backend Backend);

Expand Down
3 changes: 3 additions & 0 deletions sycl/plugins/cuda/pi_cuda.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3564,6 +3564,9 @@ pi_result piPluginInit(pi_plugin *PluginInit) {
// PI interface supports higher version or the same version.
strncpy(PluginInit->PluginVersion, SupportedVersion, 4);

// Set plugin Type
PluginInit->backend = SYCL_BE_PI_CUDA;

// Set whole function table to zero to make it easier to detect if
// functions are not set up below.
std::memset(&(PluginInit->PiFunctionTable), 0,
Expand Down
3 changes: 3 additions & 0 deletions sycl/plugins/opencl/pi_opencl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,6 +1054,9 @@ pi_result piPluginInit(pi_plugin *PluginInit) {
// PI interface supports higher version or the same version.
strncpy(PluginInit->PluginVersion, SupportedVersion, 4);

// Set plugin Type
PluginInit->backend = SYCL_BE_PI_OPENCL;

#define _PI_CL(pi_api, ocl_api) \
(PluginInit->PiFunctionTable).pi_api = (decltype(&::pi_api))(&ocl_api);

Expand Down
4 changes: 4 additions & 0 deletions sycl/source/detail/plugin.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ class plugin {

const RT::PiPlugin &getPiPlugin() const { return MPlugin; }

bool isBackendType(Backend backend) const {
return MPlugin.backend == backend;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems awkward to have a comparison method inside a plugin, why not simply return the Plugin type and let the caller do the comparison?
Simply have a

Backend getBackendType() const noexcept {
   return MPlugin.backend;
}

Copy link
Contributor

@smaslov-intel smaslov-intel Apr 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

+1 for returning the Backend.

Also we need this info be available externally. Could you add a method to return the BE via, say

platform.get_info<info::platform::backend>

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes sure. Actually, as I'm at it already, maybe there is a more agnostic way than to have the plugin types hard-coded in an enum in "pi.h"?

what if - for example - there was a char* backend which we can return as std::string getBackendType() and std::string platform.get_info<>? Then the plugin interface stays a bit more general and we would compare ContextImpl->getPlugin()->getBackendType() == "OpenCL"

On the other hand i found this: Sycl-Shared and they describe to even have an enum at sycl::backend to compare with...

Copy link
Contributor

Choose a reason for hiding this comment

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

have an enum at sycl::backend
That's right. It is coming very soon in this PR:
https://github.com/intel/llvm/pull/1332/files#diff-f42957493751abda949e943bbb84b93d
Let's use it in these interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks. Then I'll wait for that PR to be merged and base off it.

regarding the platform.get_info<> it should return an std::string in the end...?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should return the "backend" not a string. But note that the BE generalization proposal defines get_backend() method instead (maybe in addition?): https://github.com/KhronosGroup/SYCL-Shared/blob/master/proposals/sycl_generalization.md

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on returning the backend, not a string.

We don't have a get_info query for backends on the generalization proposal, but can be easily added if that is useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, sure. thanks. i will stick to whats in the generalization for now.

// Returns the backend to which this platform belongs
backend get_backend() const noexcept;

i will wait for #1332 to be merged and start from there.


/// Checks return value from PI calls.
///
/// \throw Exception if pi_result is not a PI_SUCCESS.
Expand Down
4 changes: 3 additions & 1 deletion sycl/source/detail/program_manager/program_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,10 @@ static bool isDeviceBinaryTypeSupported(const context &C,
return false;
}

ContextImplPtr ContextImpl = getSyclObjImpl(C);

// OpenCL 2.1 and greater require clCreateProgramWithIL
if (pi::useBackend(pi::SYCL_BE_PI_OPENCL) &&
if (ContextImpl->getPlugin().isBackendType(Backend::SYCL_BE_PI_OPENCL) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be instead:

ContextImpl->getPlugin()->getBackend() == Backend::SYCL_BE_PI_OPENCL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above, getBackendType ? if you prefer, i will do that :)

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 say getBackend is enough, since the return type encodes what the type is (a Backend)

C.get_platform().get_info<info::platform::version>() >= "2.1")
return true;

Expand Down
2 changes: 1 addition & 1 deletion sycl/source/detail/scheduler/commands.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1661,7 +1661,7 @@ cl_int ExecCGCommand::enqueueImp() {
Requirement *Req = (Requirement *)(Arg.MPtr);
AllocaCommandBase *AllocaCmd = getAllocaForReq(Req);
RT::PiMem MemArg = (RT::PiMem)AllocaCmd->getMemAllocation();
if (RT::useBackend(pi::Backend::SYCL_BE_PI_OPENCL)) {
if (Plugin.isBackendType(Backend::SYCL_BE_PI_OPENCL)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above...

Plugin.call<PiApiKind::piKernelSetArg>(Kernel, Arg.MIndex,
sizeof(RT::PiMem), &MemArg);
} else {
Expand Down