-
Notifications
You must be signed in to change notification settings - Fork 634
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
Adding iree_hal_buffer_placement_t info to allocated HAL buffers. #19160
Conversation
9dbeec9
to
58ab7f1
Compare
58ab7f1
to
f3e1923
Compare
Thanks. Will review in a bit in detail. Let's hold landing until the release cut as one of those breaking APIs is used downstream and it would be better to not be in a wedged state over the weekend (so land on Monday). |
cool, just lmk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick review pass through runtime/src/iree/hal/buffer.[c,h]
. I can finish reviewing the rest tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's hold landing until the release cut as one of those breaking APIs is used downstream and it would be better to not be in a wedged state over the weekend (so land on Monday).
3.0.0 release has been cut and published: https://github.com/iree-org/iree/releases/tag/iree-3.0.0. If you want to mention any patch-notes-worthy changes in here, the 3.1.0 release tracker is open for business: #19192
We can probably just lift what you have in the PR description mostly as-is:
Breaking API changes:
iree_hal_buffer_subspan
now requires aniree_allocator_t host_allocator
that was previously implicit.iree_hal_subspan_buffer_initialize
removed as it was not safe and shouldn't have been used.iree_hal_deferred_buffer_t
removed as it is not possible to do the placement checks without an allocated buffer reference. This is used in a branch for async allocations on CPU but either a different approach is required there or it can be moved intoiree/hal/local/
where we can make assertions about universal accessibility.iree_hal_heap_buffer_wrap
and all other buffer creation now requires a placement.iree_hal_buffer_initialize
used by HAL implementations now requires a placement.
Maybe a bit of reformatting and a link to the commit or PR. Could also lead with the TLDR reasoning for the change, like "buffer APIs now allow for more efficient deallocation" (or something - so it doesn't look like the APIs just changed for the sake of changing them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
I'll need to adapt some minor uses once this lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the rest of the files. LGTM
This allows users to query for the device, queue affinity, and origin flags of an allocated buffer so that they can decide whether to copy buffers when moving across devices/queues, perform synchronous or asynchronous deallocations, and select queues to perform such operations. The base `iree_hal_buffer_t` structure was cleaned up a bit and though still ugly is better prepared for removal of the device allocator back reference. #19159 tracks removing the allocator reference that is currently only used by the caching allocator due to our lack of dynamic casts.
f3e1923
to
e767e0d
Compare
This allows users to query for the device, queue affinity, and origin flags of an allocated buffer so that they can decide whether to copy buffers when moving across devices/queues, perform synchronous or asynchronous deallocations, and select queues to perform such operations. The downside is that prior to this buffers could be defined as shared across multiple devices that share a single allocator - because it's still the case that they can be shared "placement" was used as it's not "availability" or "access permissions" but only indicating the origin of the buffer and where it's preferred location is. Buffer compatibility queries are still the way to determine access visibility.
The base
iree_hal_buffer_t
structure was cleaned up a bit and though still ugly is better prepared for removal of the device allocator back reference. #19159 tracks removing the allocator reference that is currently only used by the caching allocator due to our lack of dynamic casts.Breaking API changes:
iree_hal_buffer_subspan
now requires aniree_allocator_t host_allocator
that was previously implicit.iree_hal_subspan_buffer_initialize
removed as it was not safe and shouldn't have been used.iree_hal_deferred_buffer_t
removed as it is not possible to do the placement checks without an allocated buffer reference. This is used in a branch for async allocations on CPU but either a different approach is required there or it can be moved intoiree/hal/local/
where we can make assertions about universal accessibility.iree_hal_heap_buffer_wrap
and all other buffer creation now requires a placement.iree_hal_buffer_initialize
used by HAL implementations now requires a placement.