Skip to content

[UR][HIP] Unbundle hip conformance test binaries #18184

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

Closed
wants to merge 1 commit into from

Conversation

RossBrunton
Copy link
Contributor

The binaries used for conformance testing on hip are generated using
SYCL programs which output a "bundled object" rather than a raw amdhsa
binary. This unbundles the objects so that the HIP conformance tests are
given the raw binaries.

The binaries used for conformance testing on hip are generated using
SYCL programs which output a "bundled object" rather than a raw amdhsa
binary. This unbundles the objects so that the HIP conformance tests are
given the raw binaries.
@RossBrunton RossBrunton marked this pull request as ready for review April 25, 2025 13:15
@RossBrunton RossBrunton requested a review from a team as a code owner April 25, 2025 13:15
@RossBrunton RossBrunton requested review from a team, npmiller and jchlanda and removed request for a team and npmiller April 25, 2025 13:15
@RossBrunton
Copy link
Contributor Author

@intel/llvm-reviewers-cuda Apologies if you aren't the right team for this, but could I get eyes on this? There seems to be two main types of binaries that the hip backend can accept: An "offload bundle" and a raw elf. Offload doesn't seem to support the "offload bundle", so this change switches to using the raw elf for testing.

If I understand correctly, there should be no real difference between the actual binary data or behaviour.

@npmiller
Copy link
Contributor

Yes, that's the right team.

The change looks good to me, although I believe SYCL is passing the "offload bundle" version to the ur adapters at the moment.

Wouldn't the proper fix be to make offload also support that, what do you think? But I don't really know how offload works currently, so I'm happy to go with that for now if it makes more sense.

@RossBrunton
Copy link
Contributor Author

RossBrunton commented Apr 28, 2025

@npmiller Perhaps, yeah. But I think that would require a lot of work either on the AMD or (more likely) liboffload's end. I think such a thing should probably be done as a separate task and with a proper design. I'd rather just have this change for now and look at "offload bundle" support later.

Copy link
Contributor

@npmiller npmiller left a comment

Choose a reason for hiding this comment

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

After further discussions: offload for AMD is currently based on HSA so it can't easily support the HIP bundles. We may still want to support them in the future but this is a good workaround in the meantime.

@RossBrunton
Copy link
Contributor Author

Closing this: While it would have made sense as a temporary workaround, adding hip bundle support to the offload UR backend was easier than expected. Since SYCL provides hip bundles anyway, it's worth just merging that instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants