Skip to content

Add hAdapter parameter to urPlatformCreateWithNativeHandle #1692

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.

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

Conversation

aarongreig
Copy link
Contributor

@aarongreig aarongreig commented May 30, 2024

Other CreateWithNative entry points need a UR handle primarily so the loader has something to obtain a ddi table from. This new adapter parameter fulfils that purpose for PlatformCreateWithNative, allowing us to remove a weird hack that saw the loader getting (apparently succesfully somehow??) a loader object out of the native handle itself.

fixes #1068

@aarongreig aarongreig requested review from a team as code owners May 30, 2024 15:37
@aarongreig aarongreig requested a review from JackAKirk May 30, 2024 15:37
@github-actions github-actions bot added loader Loader related feature/bug conformance Conformance test suite issues. specification Changes or additions to the specification level-zero L0 adapter specific issues cuda CUDA adapter specific issues hip HIP adapter specific issues native-cpu Native CPU adapter specific issues labels May 30, 2024
@aarongreig aarongreig force-pushed the aaron/platformCreateWithNativeAdapter branch from 5566671 to f9638bc Compare May 31, 2024 09:32
@JackAKirk
Copy link
Contributor

JackAKirk commented May 31, 2024

I see that this patch makes sense. I'm not sure this really needs a cuda/hip review because I'm not sure that this interface is really ever going to be called by cuda/hip backend.
I'll put my reasoning below for future reference:

For cuda and hip (and I guess some other backends), the interface of urPlatformCreateWithNativeHandle doesn't really matter because I think there should only ever be one platform in these backends consisting of all available devices. And to get this list and construct the platform we don't need any inputs I think.
I may be not taking some requirement into account so I think I should ask @npmiller and @AerialMantis to give their opinions on this since they may have a different point of view.

This is my perspective:

  • There's no underlying native platform object in these backends
  • It is up to us to define how a platform is defined and can be constructed in these backends. Currently hip and cuda backends can have only one platform when it is constructed by the sycl runtime itself.

Therefore, IMO it should only be ever possible to construct a single platform in these backends, which includes all the devices that are available to the runtime.

There was a proposal that we should define interop native platform in these backends as a vector of devices, which could imply that users could create multiple platforms from a cuda or hip backend, even if the runtime can only ever create one. Having multiple platforms in cuda/hip backends doesn't make any sense to me. We had to do it temporarily in cuda for awkward sycl::context spec issue but this has now been fixed. The fact that we previously had to have multiple platforms in the cuda backend might have been a motivation for this list of devices definition of the native platform, but I think that is out of date now.

But @AerialMantis will know more about this

I don't imagine that other runtimes would behave differently, but I don't know this for sure.

If others agree then this might be a nice time to add some warning or comment in the cuda/hip adapter source stating this interface won't be called in cuda/hip for the above reasons. Just so in the future some poor soul doesn't get roped into trying to work out how to implement it for cuda/hip.

@JackAKirk
Copy link
Contributor

JackAKirk commented May 31, 2024

Changes look fine to me, I'll approve this. I made some related points about the relevance of the usage of this function in cuda/hip. IMO we should add a comment in the cuda/hip implementations saying something to the effect that these interop functions don't make sense for cuda/hip backends.
But I see that this is not the main issue of the changes made in this patch.

Copy link
Contributor

@PietroGhg PietroGhg left a comment

Choose a reason for hiding this comment

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

Native CPU LGTM, thank you

@aarongreig
Copy link
Contributor Author

added a comment explaining the unsupported error codes

@aarongreig
Copy link
Contributor Author

LLVM PR intel/llvm#14012

Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

You can also modify the helper script to get rid of now redundant types in the loader:
#1067 (comment)

@kbenzie kbenzie mentioned this pull request Jun 4, 2024
@aarongreig aarongreig force-pushed the aaron/platformCreateWithNativeAdapter branch 2 times, most recently from debd8e5 to a44e5e6 Compare June 6, 2024 08:59
@aarongreig
Copy link
Contributor Author

@oneapi-src/level-zero-maintainers please review, this is important for the sycl UR port

Other CreateWithNative entry points need a UR handle primarily so the
loader has something to obtain a ddi table from. This new adapter
parameter fulfils that purpose for PlatformCreateWithNative, allowing us
to remove a weird hack that saw the loader getting (apparently
succesfully somehow??) a loader object out of the native handle itself.
@aarongreig aarongreig force-pushed the aaron/platformCreateWithNativeAdapter branch from a44e5e6 to fcecc00 Compare June 11, 2024 09:14
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

The L0 adapter change is insignificant.

@pbalcer pbalcer added the ready to merge Added to PR's which are ready to merge label Jun 11, 2024
@kbenzie kbenzie merged commit a53f89d into oneapi-src:main Jun 14, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conformance Conformance test suite issues. cuda CUDA adapter specific issues hip HIP adapter specific issues level-zero L0 adapter specific issues loader Loader related feature/bug native-cpu Native CPU adapter specific issues ready to merge Added to PR's which are ready to merge specification Changes or additions to the specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

urPlatformCreateWithNativeHandle should accept adapter
6 participants