Starting rework of HAL allocator APIs.#8389
Merged
Conversation
benvanik
force-pushed
the
benvanik-memory-types
branch
2 times, most recently
from
7f19dd0 to
e8ff3b0
Compare
This allows for more parameters to be added without needing to update all the function signatures, particularly when cutting across layers (such as the buffer view helpers). The new iree_hal_buffer_params_t struct carries some additional fields for alignment and affinity that future allocator implementations can use for placement. The parameters are passed by-value today (which, given the amount means on-stack). That's fine as all these methods had enough params to pass on-stack anyway and by the time you're calling into the allocator to do something you're already off the fast path. The vtable/implementations use by-ref so that the stack copy is only done once where the ergonomics matter (iree_hal_* user API). Ergonomics from C++ will improve after follow-on changes simplify the memory type/buffer usage fields.
benvanik
force-pushed
the
benvanik-memory-types
branch
from
e8ff3b0 to
c337a4f
Compare
benvanik
requested review from
GMNGeoffrey,
ThomasRaoux,
antiagainst and
stellaraccident
as code owners
benvanik
removed request for
GMNGeoffrey,
ThomasRaoux,
antiagainst and
stellaraccident
ScottTodd
approved these changes
Feb 23, 2022
Comment on lines
-86
to
+91
| IREE_HAL_MEMORY_TYPE_HOST_LOCAL | IREE_HAL_MEMORY_TYPE_DEVICE_VISIBLE, | ||
| IREE_HAL_MEMORY_ACCESS_READ, IREE_HAL_BUFFER_USAGE_ALL, | ||
| (iree_hal_buffer_params_t){ | ||
| .type = IREE_HAL_MEMORY_TYPE_HOST_LOCAL | | ||
| IREE_HAL_MEMORY_TYPE_DEVICE_VISIBLE, | ||
| .access = IREE_HAL_MEMORY_ACCESS_READ, | ||
| .usage = IREE_HAL_BUFFER_USAGE_ALL, | ||
| }, |
Member
There was a problem hiding this comment.
I was a little worried about these functions getting even more verbose, but this syntax with named initializers for the struct reads pretty easily.
Collaborator
Author
There was a problem hiding this comment.
intellisense works pretty well too, and now (with the coming changes) that the fields are mostly optional these could even just be ..., {IREE_HAL_BUFFER_USAGE_FOO}, ... inline. C++ 20 will be able to do this named stuff too, but for now that unnamed style should at least make that cleaner 🤞
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the first part of a larger set of changes that reworks how buffers are allocated by both simplifying what the user needs to provide and adding more expression to what is provided. Memory types will still exist but in almost all cases be specified as
OPTIMALwhen allocating buffers enabling the implementation to choose the types based on availability and underlying API requirements. There are still cases where one would need to specify them (creating importable/exportable buffers, interoping with buffers allocated externally, etc) but usage will be the primary discriminator.As part of this the allocator methods now take a
iree_hal_buffer_params_tstruct containing the various flags and attributes required to perform an allocation, enabling us to associate location information by way of queue affinities and alignment requirements which are required for exportable buffers in some implementations and virtual memory management. The intent is that zero initialization will be fine (onceOPTIMALis in place) but today both type and usage are required.Future changes will significantly rework
iree_hal_memory_type_tandiree_hal_buffer_usage_tand add some queries that can be used by allocator shims to determine how usage interacts with memory types. This will require compiler changes as well in order to plumb through the required usage information from the HAL dialect ops.