Skip to content

[SYCL][UR] Add stub for urGetLastResult #8602

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
jandres742 wants to merge 1 commit into from
Closed

[SYCL][UR] Add stub for urGetLastResult #8602

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
10 changes: 9 additions & 1 deletion sycl/plugins/unified_runtime/pi2ur.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,15 @@ inline pi_result piDeviceRelease(pi_device Device) {
return PI_SUCCESS;
}

inline pi_result piPluginGetLastError(char **message) { return PI_SUCCESS; }
inline pi_result piPluginGetLastError(char **Message) {
uint32_t Count = 1u;
uint32_t NumPlatforms = 0;
ur_platform_handle_t Platform {};
HANDLE_ERRORS(urPlatformGet(Count, &Platform, &NumPlatforms));
HANDLE_ERRORS(urGetLastResult(Platform,
const_cast<const char **>(Message)));
return PI_SUCCESS;
}

inline pi_result piDeviceGetInfo(pi_device Device, pi_device_info ParamName,
size_t ParamValueSize, void *ParamValue,
Expand Down
9 changes: 9 additions & 0 deletions sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_level_zero.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,15 @@ UR_APIEXPORT ur_result_t UR_APICALL urPlatformGet(
return UR_RESULT_SUCCESS;
}

ur_result_t UR_APICALL urGetLastResult(
ur_platform_handle_t Platform, ///< [in] handle of the platform instance
Copy link
Contributor

@smaslov-intel smaslov-intel Mar 10, 2023

Choose a reason for hiding this comment

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

why is there the Platform argument in UR unlike in PI?

Copy link
Contributor Author

@jandres742 jandres742 Mar 10, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@smaslov-intel smaslov-intel Mar 10, 2023

Choose a reason for hiding this comment

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

This interface was meant to be per-plugin (and not per-platform). And it's implementation is such now too:

llvm/sycl/plugins/unified_runtime/ur/ur.cpp

Line 41 in fea99cc

ur_result_t zerPluginGetLastError(char **message) {

@pbalcer, @jandres742 : I am not against make it per-platform, but then more changes are needed than just these.

Copy link
Contributor

@pbalcer pbalcer Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@smaslov-intel smaslov-intel Mar 10, 2023

Choose a reason for hiding this comment

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

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:

llvm/sycl/plugins/unified_runtime/ur/ur.cpp

Line 31 in fea99cc

thread_local char ErrorMessage[MaxMessageSize];

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)

Copy link
Contributor Author

@jandres742 jandres742 Mar 10, 2023

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

@smaslov-intel smaslov-intel Mar 10, 2023

Choose a reason for hiding this comment

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

@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:

llvm/sycl/plugins/unified_runtime/ur/ur.cpp

Line 41 in fea99cc

ur_result_t zerPluginGetLastError(char **message) {

The 2 need to converge

Copy link
Contributor Author

@jandres742 jandres742 Mar 10, 2023

Choose a reason for hiding this comment

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

I see @smaslov-intel . Ok, fixing that that.

const char **Message ///< [out] pointer to a string containing adapter
///< specific result in string representation.
) {
ur_result_t result = UR_RESULT_SUCCESS;
return result;
}

UR_APIEXPORT ur_result_t UR_APICALL urPlatformGetInfo(
ur_platform_handle_t Platform, ///< [in] handle of the platform
ur_platform_info_t ParamName, ///< [in] type of the info to retrieve
Expand Down