[SYCL][PI] get backend type from Plugin, not env #1477
Conversation
useBackend queried for "SYCL_BE" env. scheduler wrongly relied on this. Instead it should get the backend type from the used plugin Signed-off-by: hiaselhans <simon.klemenc@gmail.com>
|
oh, and i found that that usm code is gone as well with #1474 |
| @@ -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. | |||
There was a problem hiding this comment.
Does the comment still apply? The environmental variable is not used anymore
There was a problem hiding this comment.
no, i will remove it. i just took the part from pi.hpp to get feedback on this
| @@ -31,6 +31,10 @@ class plugin { | |||
|
|
|||
| const RT::PiPlugin &getPiPlugin() const { return MPlugin; } | |||
|
|
|||
| bool isBackendType(Backend backend) const { | |||
| return MPlugin.backend == backend; | |||
| } | |||
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
+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>
?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
+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.
There was a problem hiding this comment.
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.
| // OpenCL 2.1 and greater require clCreateProgramWithIL | ||
| if (pi::useBackend(pi::SYCL_BE_PI_OPENCL) && | ||
| if (ContextImpl->getPlugin().isBackendType(Backend::SYCL_BE_PI_OPENCL) && |
There was a problem hiding this comment.
This could be instead:
ContextImpl->getPlugin()->getBackend() == Backend::SYCL_BE_PI_OPENCLThere was a problem hiding this comment.
as above, getBackendType ? if you prefer, i will do that :)
There was a problem hiding this comment.
I would say getBackend is enough, since the return type encodes what the type is (a Backend)
| @@ -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)) { | |||
The centerpiece of this patch is the addition of getParameterTypes function, which will parse out the pointee struct types using LLVM's built-in Itanium demangling interface (which is unfortunately not easy to use). Passing this information along via llvm StructTypes was chosen to minimize the impact of this change on caller code; it is likely that a future cleanup patch to use an enum or similar mechanism to represent possible SPIR-V struct types would be advisable. A side effect of this change is that several helper methods that query pointer element types can be eliminated, which does lead to some cleanup that got tangled up in the demangler-enabling work. Extra function calls to getPointerElementType() are temporarily added to disentangle this patch from work moving OCLTypeToSPIRV to store pointee types rather than pointer types; this will likely be fixed as part of the effort to get SPIRVWriter working with opaque pointer types. Original commit: KhronosGroup/SPIRV-LLVM-Translator@76e181e
useBackend queried for "SYCL_BE" env. scheduler wrongly relied on this.
Instead it should get the backend type from the used plugin
Signed-off-by: hiaselhans simon.klemenc@gmail.com
This PR is adressing #1473 which i was experiencing as well.
It is just an idea how it could work, maybe it could be solved more elegant by making the enum extendible or so. There are still some "useBackend" left in USM code which i think we should get rid of aswel. that function just checks for the SYCL_BE env variable which does not garuantee that this plugin is actually used.