Skip to content

[UR] Replace loader handles with field at start of handle data #17118

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
merged 1 commit into from
May 13, 2025

Conversation

RossBrunton
Copy link
Contributor

@RossBrunton RossBrunton commented Feb 21, 2025

Migrated from oneapi-src/unified-runtime#2622

All handles from all backends are now required to implement ddi_getter and their first field must be a pointer to a ur_ddi_table_t (which also implies that they must not have a vtable).

Instead of wrapping handles in a special wrapper type, we instead query the DDI table stored in the handle itself. This simplifies the loader greatly.

@RossBrunton RossBrunton force-pushed the ross/new-nohandle branch 2 times, most recently from 9690a1c to 33d3736 Compare February 26, 2025 14:30
@RossBrunton RossBrunton force-pushed the ross/new-nohandle branch 2 times, most recently from 407b51b to 9df489a Compare March 21, 2025 13:43
@RossBrunton RossBrunton force-pushed the ross/new-nohandle branch from b9f1993 to 00bd9e2 Compare May 9, 2025 15:51
@kbenzie kbenzie marked this pull request as ready for review May 12, 2025 12:25
@kbenzie
Copy link
Contributor

kbenzie commented May 12, 2025

@intel/dpcpp-nativecpu-reviewers please review, the native cpu changes are pretty minimal.

Copy link
Contributor

@uwedolinsky uwedolinsky left a comment

Choose a reason for hiding this comment

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

Nativecpu updates look ok

@kbenzie kbenzie force-pushed the ross/new-nohandle branch from 00bd9e2 to dc23a0a Compare May 12, 2025 15:09
@kbenzie kbenzie temporarily deployed to WindowsCILock May 12, 2025 15:10 — with GitHub Actions Inactive
@kbenzie kbenzie temporarily deployed to WindowsCILock May 12, 2025 16:47 — with GitHub Actions Inactive
@kbenzie kbenzie temporarily deployed to WindowsCILock May 12, 2025 16:47 — with GitHub Actions Inactive
All handles from all backends are now required to implement `ddi_getter` and their first field must be a pointer to a
`ur_ddi_table_t` (which also implies that they must not have a vtable).

Instead of wrapping handles in a special wrapper type, we instead query the DDI table stored in the handle itself. This
simplifies the loader greatly.
@kbenzie kbenzie force-pushed the ross/new-nohandle branch from dc23a0a to 5984920 Compare May 13, 2025 09:34
@kbenzie kbenzie temporarily deployed to WindowsCILock May 13, 2025 09:34 — with GitHub Actions Inactive
@kbenzie kbenzie temporarily deployed to WindowsCILock May 13, 2025 09:53 — with GitHub Actions Inactive
@kbenzie kbenzie temporarily deployed to WindowsCILock May 13, 2025 09:53 — with GitHub Actions Inactive
@kbenzie
Copy link
Contributor

kbenzie commented May 13, 2025

@kbenzie kbenzie merged commit 603feac into intel:sycl May 13, 2025
30 of 32 checks passed
EwanC added a commit to reble/llvm that referenced this pull request May 14, 2025
The `ur_exp_command_buffer_handle_t_` definition in the CUDA
adapter was missed in intel#17118 in
the changes to inherit from a base handle.

Discovered by seeing segfaults in the UR CTS tests locally, and
git bisecting back to that change.
EwanC added a commit to reble/llvm that referenced this pull request May 14, 2025
The `ur_exp_command_buffer_handle_t_` definition in the CUDA
adapter was missed in intel#17118 in
the changes to inherit from a base handle.

Discovered by seeing segfaults in the UR CTS tests locally, and
git bisecting back to that change.
kbenzie pushed a commit that referenced this pull request May 14, 2025
The `ur_exp_command_buffer_handle_t_` definition in the CUDA adapter was
missed in #17118 in the changes to
inherit from a base handle.

Discovered by seeing segfaults in the UR CTS tests locally, and git
bisecting back to that change.
github-actions bot pushed a commit to oneapi-src/unified-runtime that referenced this pull request May 15, 2025
The `ur_exp_command_buffer_handle_t_` definition in the CUDA adapter was
missed in intel/llvm#17118 in the changes to
inherit from a base handle.

Discovered by seeing segfaults in the UR CTS tests locally, and git
bisecting back to that change.
aarongreig pushed a commit to oneapi-src/unified-runtime that referenced this pull request May 15, 2025
The `ur_exp_command_buffer_handle_t_` definition in the CUDA adapter was
missed in intel/llvm#17118 in the changes to
inherit from a base handle.

Discovered by seeing segfaults in the UR CTS tests locally, and git
bisecting back to that change.
igchor added a commit to igchor/llvm that referenced this pull request May 16, 2025
ur_queue_handle_t_ was not inheriting from ur::handle_base
resulting in *reinterpret_cast<ur_dditable_t **>(hQueue)
not working correctly.

For some reason this worked on some platforms, and only
manifested on PVC.
igchor added a commit to igchor/llvm that referenced this pull request May 16, 2025
ur_queue_handle_t_ was not inheriting from ur::handle_base
resulting in *reinterpret_cast<ur_dditable_t **>(hQueue)
not working correctly.

For some reason this worked on some platforms, and only
manifested on PVC.
sarnex pushed a commit that referenced this pull request May 19, 2025
ur_queue_handle_t_ was not inheriting from ur::handle_base resulting in
*reinterpret_cast<ur_dditable_t **>(hQueue) not working correctly.

For some reason this worked on some platforms, and only manifested on
PVC.
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.

6 participants