[SYCL][UR] Add stub for urGetLastResult #8602
Conversation
Update to oneapi-src/unified-runtime@6fb8e26 Tag: weekly-2023-02-24 Signed-off-by: Jaime Arteaga <jaime.a.arteaga.molina@intel.com>
Signed-off-by: Jaime Arteaga <jaime.a.arteaga.molina@intel.com>
|
this addresses a warning from #8315 and adds the stub. @smaslov-intel : please review. |
| } | ||
|
|
||
| ur_result_t UR_APICALL urGetLastResult( | ||
| ur_platform_handle_t Platform, ///< [in] handle of the platform instance |
There was a problem hiding this comment.
why is there the Platform argument in UR unlike in PI?
There was a problem hiding this comment.
@smaslov-intel : Git blame points at
oneapi-src/unified-runtime#84
From @pbalcer
This makes it possible to have a simple passthrough implementation for this method in the loader. Otherwise we'd have to have a copy of the last error in the loader itself.
There was a problem hiding this comment.
This interface was meant to be per-plugin (and not per-platform). And it's implementation is such now too:
@pbalcer, @jandres742 : I am not against make it per-platform, but then more changes are needed than just these.
There was a problem hiding this comment.
There's no adapter/plugin entity in UR's API. The platform is the closest analog. If it's too burdensome to use this API in its current form, we can remove the platform argument, and use e.g., a thread-local variable that stores the last used adapter instance so that we know where to redirect the call. But that's an extra tls access and write in every call (which might be fine, just something to consider).
There was a problem hiding this comment.
With PI we are registering the specific plugin's entries at the beginning with piPluginInit, so SYCL RT was talking directly to the specific plugin and there the the error-message was a TLS:
Now, if you really need to re-direct to the needed plugin from the UR loader, then it makes sense to add the platform argument (I assume you keep a map of platforms to adapters). But let's not fork the second set of interfaces and re-do the current ones (add platform to PI)
There was a problem hiding this comment.
@pbalcer : even if we leave the platform argument, this needs to be tls (in the adapter instead of the loader), since multiple threads could be calling different UR calls in the same platform, each with a different error code.
From your comment in the PR, the idea of using the platform is to have a way of knowing which adapter the PI runtime is calling. W/o the platform, either we can have only 1 adapter used at a time, or as you mention, to track last adapter used by the thread.
@smaslov-intel : to which other changes are you referring to?
There was a problem hiding this comment.
@smaslov-intel : to which other changes are you referring to?
This PR adds urGetLastResult: https://github.com/intel/llvm/pull/8602/files#diff-d293a76e913cc9b50844ba191619e7c43bbff7c3fdd673e36dcb4c980f4b0589R406
But there is already zerPluginGetLastError:
The 2 need to converge
There was a problem hiding this comment.
I see @smaslov-intel . Ok, fixing that that.
|
New version of this API being implemented in #10349. Closing. |
4 participants
No description provided.