Skip to content
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

Remove ur_base_desc_t, use ur_base_properties_t instead. #1134

Closed

Conversation

aarongreig
Copy link
Contributor

The definitions of these two base structs are slightly different which potentially has weird consequences if a given pNext chain contains extension structs derived from both. Unifying these structs removes the possibility of their definitions drifting further apart and prevents the mixed pNext chain scenario.

@aarongreig aarongreig marked this pull request as ready for review November 29, 2023 16:30
@aarongreig aarongreig requested review from a team as code owners November 29, 2023 16:30
@aarongreig
Copy link
Contributor Author

@jandres742 it looks like UR inherited this design from level zero, do you have any insight on why two base structs were used, and if there's a good reason to keep them for UR?

@jandres742
Copy link

@jandres742 it looks like UR inherited this design from level zero, do you have any insight on why two base structs were used, and if there's a good reason to keep them for UR?

@aarongreig : I dont recall exactly, but I think original idea is that base_desc and base_properties could have other base fields added in the future, specific to descriptors and properties, besides pNext and stype. but as you mention.

I think change is good and would reduce code cluttering.

Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

I don't have an opinion on the change as a whole, but the modifications to command-buffers are good.

@fabiomestre fabiomestre changed the base branch from adapters to main December 5, 2023 16:44
@fabiomestre
Copy link
Contributor

I have updated the target branch of this PR from the adapters branch to the main branch.
Development in UR is moving back to main. The adapters branch will soon be deleted.

Copy link
Contributor

@nrspruit nrspruit left a comment

Choose a reason for hiding this comment

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

LGTM for level zero

The definitions of these two base structs are slightly different which
potentially has weird consequences if a given pNext chain contains
extension structs derived from both. Unifying these structs removes the
possibility of their definitions drifting further apart and prevents the
mixed pNext chain scenario.
@aarongreig aarongreig force-pushed the aaron/unifyBaseStructs branch from ce3ac87 to ba78d27 Compare January 23, 2024 14:21
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.

LGTM for HIP and CUDA

Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

LGTM

@kbenzie kbenzie added loader Loader related feature/bug specification Changes or additions to the specification level-zero L0 adapter specific issues cuda CUDA adapter specific issues hip HIP adapter specific issues opencl OpenCL adapter specific issues labels Apr 10, 2024
@martygrant
Copy link
Contributor

Unified Runtime -> intel/llvm Repo Move Notice

Information

The source code of Unified Runtime has been moved to intel/llvm under the unified-runtime top-level directory,
all future development will now be carried out there. This was done in intel/llvm#17043.

The code will be mirrored to oneapi-src/unified-runtime and the specification will continue to be hosted at oneapi-src.github.io/unified-runtime.

The contribution guide has been updated with new instructions for contributing to Unified Runtime.

PR Migration

All open PRs including this one will be labelled auto-close and shall be automatically closed after 30 days.
To allow for some breathing space, this automation will not be enabled until next week (27/02/2025).

Should you wish to continue with your PR you will need to migrate it to intel/llvm.
We have provided a script to help automate this process.


This is an automated comment.

@martygrant
Copy link
Contributor

Unified Runtime -> intel/llvm Repo Move Notice

Following on from the previous notice, we have now enabled workflows to automatically label and close PRs because the Unified Runtime source code has moved to intel/llvm.

This PR has now been marked with the auto-close label and will be automatically closed after 30 days.

Please review the previous notice for more information, including assistance with migrating your PR to intel/llvm.

Should there be a reason for this PR to remain open, manually remove the auto-close label.


This is an automated comment.

@github-actions github-actions bot closed this Mar 31, 2025
Copy link

Automatic PR Closure Notice

Information

This PR has been closed automatically. It was marked with the auto-close label 30 days ago as part of the Unified Runtime source code migration to the intel/llvm repository - intel/llvm#17043.

All Unified Runtime development should be done in intel/llvm, details can be found in the updated contribution guide.
This repository will continue to exist as a mirror and will host the specification documentation.

Next Steps

Should you wish to re-open this PR it must be moved to intel/llvm. We have provided a script to help automate this process, otherwise no actions are required.


This is an automated comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-close cuda CUDA adapter specific issues hip HIP adapter specific issues level-zero L0 adapter specific issues loader Loader related feature/bug opencl OpenCL adapter specific issues specification Changes or additions to the specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.