Merging simplified HAL bindings branch. #18366
Merged
Conversation
This file contains 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
benvanik
force-pushed
the
shared/dispatch2
branch
2 times, most recently
from
86e0ae0
to
e48b8e3
Compare
benvanik
added
compiler/dialects
benvanik
changed the title
benvanik
force-pushed
the
shared/dispatch2
branch
from
6d64096
to
5aa006a
Compare
benvanik
requested review from
antiagainst,
ScottTodd,
nithinsubbiah,
kuhar,
MaheshRavishankar,
stellaraccident,
hanhanW,
IanWood1,
qedawkins and
Groverkss
as code owners
benvanik
requested review from
AWoloszyn
and removed request for
stellaraccident,
kuhar,
hanhanW,
nithinsubbiah,
qedawkins,
Groverkss and
IanWood1
benvanik
force-pushed
the
shared/dispatch2
branch
from
279bbb2
to
5aa006a
Compare
ScottTodd
approved these changes
Aug 27, 2024
There was a problem hiding this comment.
I triggered a run of the arm64 job on this branch at https://github.com/iree-org/iree/actions/runs/10584824089/job/29329981174. Could also trigger the other nightly jobs (https://iree.dev/developers/general/github-actions/#workflow-descriptions-and-status) if desired, but I expect we can fix-forward any issues in those.
That workflow run passed. |
antiagainst
approved these changes
Aug 27, 2024
|
|
||
| // Direct overlay of the VkDescriptorType enum. | ||
| enum VkDescriptorType:uint32 { | ||
| SAMPLER = 0, // VK_DESCRIPTOR_TYPE_SAMPLER |
There was a problem hiding this comment.
Do we (plan to) use sampler actually? Or is this for some other purposes?
There was a problem hiding this comment.
flatbuffers requires a 0 value so I had to have something and wanted to keep it the same as VkDescriptorType - though one day maybe :)
| // A SPIR-V shader module and runtime pipeline layout description. | ||
| // This information is used to create the VkShaderModule, VkPipelineLayout, and | ||
| // any required VkDescriptorSetLayouts. | ||
| table ExecutableDef { |
There was a problem hiding this comment.
Now this is more like DX12 root signature :D
There was a problem hiding this comment.
yep! the simplification lets us make cuda/hip/etc simpler (though we could still improve those - we're using the legacy void** argument packing instead of packing ourselves which would be more efficient) and is easier to map to d3d12!
(tables have Def, structs don't)
It's a HAL executable and will contain multiple kernel libraries.
This also adds source file publishing to all GPU targets. Basic support for export-specific debug info is added but switching targets to use it is left to a future change.
I don't expect us to have the same flatbuffers for different HAL implementations and do expect us to have alternative file formats for the same HAL implementation.
This produces a new flatbuffer that supports multiple hipModule_ts per HAL executable, reorganizes per-export information to be per-export, and removes HAL pipeline layouts and the existing stateful command recording.
This produces a new flatbuffer that supports multiple CUmodules per HAL executable, reorganizes per-export information to be per-export, and removes HAL pipeline layouts and the existing stateful command recording.
It already did for the inline HAL so this is mostly just removing the pipeline layouts from the full HAL implementation.
This produces a new flatbuffer that supports multiple shared modules per HAL executable, reorganizes per-export information to be per-export, and swaps HAL pipeline layouts with internal Vulkan pipeline layouts that are constructed from metadata in the flatbuffer. This is a startup time regression for as long as we are not linking modules because descriptor set and pipeline layout reuse now happens per executable and not per VM context. The fix is to link modules together.
This does not yet rename the methods and is just stripping all of the legacy ops and methods. Progress on #18154.
This produces a new flatbuffer that supports multiple shared libraries per HAL executable, reorganizes per-export information to be per-export, and swaps HAL pipeline layouts with the metadata required to setup Metal compute pipelines with proper binding attributes. Extra debug information is also plumbed through now - though we don't have tracy wired up yet it could be plumbed through to Metal tooling.
The pass is running on HAL IR and now that descriptor sets are being removed as part of #18154 needs to be rewritten. A new version would operate on SPIR-V IR in order to replace push constants with special binding loads instead.
* Renamed `push_constants` to `constants` (as there is no longer a `push_constants` API) * Dropped `#hal.descriptor_set.layout` * Removed ordinal from `#hal.descriptor_set.binding` (as ordinals are now implicit) * Renamed `#hal.descriptor_set.binding` to `#hal.pipeline.binding` * Removed `set` from `hal.interface.binding.subspan` * Removed `#hal.interface.binding` and the spooky action at a distance `hal.interface.binding` attr now that ordinals are implicit Progress on #18154.
benvanik
force-pushed
the
shared/dispatch2
branch
from
5aa006a
to
1acbedc
Compare
|
Test failure is the einsum thing disabled in #18379; merging so I can go to dinner :) |
qedawkins
added a commit
to qedawkins/iree
that referenced
this pull request
Aug 28, 2024
There was a mid-air collision between iree-org#18358 and iree-org#18366, fixing forward.
banach-space
added a commit
to banach-space/iree
that referenced
this pull request
Aug 30, 2024
I've just landed an update for the affected test (see iree-org#18369), but unfortunately forgot to re-base after the recent changes to HAL by Ben, see iree-org#18366 and iree-org#18154. This simply updates the test to align with the recent changes to HAL.
banach-space
added a commit
to banach-space/iree
that referenced
this pull request
Aug 30, 2024
I've just landed an update for the affected test (see iree-org#18369), but unfortunately forgot to re-base after the recent changes to HAL by Ben, see iree-org#18366 and iree-org#18154. This simply updates the test to align with the recent changes to HAL. Signed-off-by: Andrzej Warzynski <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 removes the concept of descriptor sets and pipeline layouts from the HAL and switches programs to using a flat list of bindings per dispatch as real programs rarely benefited and will do so less as command buffer reuse is enabled. Command buffer recording is now stateless as the information in push constants and push descriptor set commands are carried per dispatch and there's no need to track pipeline layouts. Pipeline layouts are still present in a reduced form in the compiler IR in order to handle dispatch ABI in a normalized way but it's up to the TargetBackends to encode them. Encoded metadata for pipeline layouts is now embedded in the target-specific executables for targets that require them (Metal/Vulkan/WebGPU/D3D12) and a reduced set of information is embedded for others. This simplifies the HAL API quite a bit, makes implementing the HAL easier as targets have more freedom in how constants and bindings are mapped to lower-level implementations, and in practice improves command buffer recording latency as there are fewer VM calls per dispatch on average.
Since the flatbuffers needed to change to include the new metadata that previously was handled via the HAL APIs this branch also modernizes and normalizes the flatbuffers across targets to both better match the implementation and support features like multiple shader modules/kernel libraries/etc per HAL executable (even if the compiler isn't linking them yet). This reorganization is required to effectively manage cached resources - before the compiler would deduplicate pipeline layouts across all executables but now that each executable is responsible for that having 1000 executables means that there will be 1000 pipeline layouts even if most are the same. Perhaps this will serve as good motivation to finally finish linking in all backends :) Debug info is also consistently added for all targets and processed for tracing and factored such that new debug info can be added per-exported function without needing to change per-target code.
There are many IR changes here and many test updates: most of the tests that were updated are in codegen and should not be using HAL ops at all. As codegen test cleanup continues to switch from HAL ops to basic functions future changes to the HAL IR will be easier. Notable changes include:
push_constantstoconstants(as there is no longer apush_constantsAPI)#hal.descriptor_set.layout#hal.descriptor_set.binding(as ordinals are now implicit)#hal.descriptor_set.bindingto#hal.pipeline.bindingsetfromhal.interface.binding.subspan#hal.interface.bindingand the spooky action at a distancehal.interface.bindingattr now that ordinals are implicitMetal/CUDA/Vulkan/HIP/CPU have all been updated to the new binding model. WebGPU has had some changes applied but needs some significant specialized work due to its existing push constant emulation requiring compiler-side descriptor sets. That's left for future work when that experimental backend is revived.
This bumps the HAL version to 5 (types removed and methods changed) and the CPU executable library version to 5 (added reserved per-export fields for future use).
Fixes #18154.