[0002] RootSignatures - Add missing Descriptor Ranges offset
#191
Conversation
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
- it appears the `offset` was mistakenly dropped as discarded this information no longer allows the user to provide a manual offset from the start of the descriptors heap and allow for register aliasing
|
This was noted when working on implementation here: https://github.com/llvm/llvm-project/pull/125131/files#r1985869019. It may be the case that it was intentional for |
V-FEXrt
approved these changes
Mar 10, 2025
|
Nice catch! D3D12_DESCRIPTOR_RANGE_OFFSET_APPEND is a magic value as well that should be mentioned in the proposal (see https://learn.microsoft.com/en-us/windows/win32/api/d3d12/ns-d3d12-d3d12_descriptor_range) |
inbelic
commented
Mar 17, 2025
inbelic
force-pushed
the
inbelic/rootsig-ranges-offset
branch
from
1d2a329 to
767e3d8
Compare
|
Whoops, accidently pushed my other branch. No changes were made. |
damyanp
reviewed
Mar 20, 2025
|
|
||
| - `offset`: Can be assigned the value `DESCRIPTOR_RANGE_OFFSET_APPEND`, which | ||
| denotes that the described descriptor range immediately follows the preceding | ||
| range. This will be resolved to a fixed numerical offset during serialization. |
There was a problem hiding this comment.
Do we need to pass 'append' all the way through? If we fix this up in the frontend and don't have a way to represent 'append' in the metadata, then this makes the metadata requirements simpler.
There was a problem hiding this comment.
We are able to compute this when we generate the metadata (computed as part of register range overlap anyways). So we would then be able to omit this from the metadata spec. However, a user would still be able to specify -1 in the metadata and then the backend will interpret this as an append not as the literal offset value.
I am personally happy with that
There was a problem hiding this comment.
I guess what I'm suggesting here is that we don't allow -1 in the metadata. So it's up to the frontend to specify the right offsets in the metadata.
There was a problem hiding this comment.
Ah I see. I have created an extra testcase here: https://github.com/llvm-beanz/offload-test-suite/pull/48. It demonstrates that we do not need to compute the discrete offset before passing it to the D3D CreateRootSignature api.
From offline discussion, this implies that we should not compute this in the frontend of clang so that we will generate the same dxil container. And so it follows that the metadata representation should allow -1 to mean append.
Updated the spec to properly reflect this behaviour.
joaosaffran
reviewed
Mar 21, 2025
joaosaffran
added a commit
to joaosaffran/wg-hlsl
that referenced
this pull request
Jun 18, 2025
* [0002] RootSignatures - Versioning proposal (llvm#115) This PR is adding the versioning information to Root Signature specs. It includes: - adding a new attribute to clang driver to specify root signature versions. - updating metadata representation to hold root signature versions. Closes: llvm#113 * [0002] RootSignatures - Add default values to proposal (llvm#173) - adds default value information to the Root Signature specification Resolves llvm#172 * Proposal for mapping resource attributes to DXIL and SPIR-V (llvm#76) * Constant buffers design document (llvm#94) Design document describing how HLSL constant buffers will be handled in Clang. Contains the design for parsing, codegen a lowering of `cbuffer` declarations, including the default constant buffer `$Globals`. Design for the `ConstantBuffer<T>` resource class is still work in progress. * Update proposal number on constant buffers proposal (llvm#188) * [NNNN] Target Extension Types for Inline SPIR-V and Decorated Types (llvm#105) This adds target extension types to represent `SpirvType` from [HLSL 0011, Inline SPIR-V](https://github.com/microsoft/hlsl-specs/blob/main/proposals/0011-inline-spirv.md). * [0018] SPIRV resource representation (llvm#98) Adds a proposal for how HLSL resources will be represented in llvm-ir when targeting SPIR-V. * [SPIR-V] Add proposal for I/O builtin in Clang (llvm#97) Initial proposal to implement Input/Output built ins with both semantic & inline SPIR-V * [SPIR-V] Add proposal for global & local variable address spaces (llvm#111) This commit adds a proposal on how to implement local and global variables in the SPIR-V backend given HLSL put them in the same address space while SPIR-V requires them to be in 2 distinct ones. --------- Signed-off-by: Nathan Gauër <[email protected]> Co-authored-by: Steven Perron <[email protected]> * [0002] RootSignatures - Add missing Descriptor Ranges `offset` (llvm#191) - it appears the `offset` was mistakenly dropped as discarding this information no longer allows the user to provide a manual offset from the start of the descriptors heap and allow for register aliasing - additionally, we do a small fix-up to update the flag values to be their correct default as we adjust the parameter values Resolves llvm#190 --------- Co-authored-by: Finn Plummer <[email protected]> * [0002] RootSignatures - Misc updates to spec from testing (llvm#195) This issue addresses the discrepancies found from testing [here](llvm/llvm-project#130826). The pr has been broken up to commits that address the bullet points in the original issue. Briefly: - First 2 commits work to have a concrete EBNF description of the grammar that denotes that all parameter args can be specified in any order - It is also addressed that `NUMBER` can be more restrictive with respect to what the dxc/clang compiler will accept - Missing documentation for `unbounded` is added - Add additional validation of some parameters that surface as errors in dxc - Remove the specification of 'AllowLowTierReservedHwCbLimit' Resolves llvm#192 --------- Co-authored-by: Finn Plummer <[email protected]> * [0021] Proposal for handling member function address spaces. (llvm#187) As we expose new address spaces in HLSL, we will have a problem with the address space for the `this` pointer in member functions. * Resource Instance Analysis (llvm#207) Fixes llvm#179 Proposes a generic architecture for organizing, resolving, and validating Resource Instance metadata --------- Co-authored-by: Damyan Pepper <[email protected]> Co-authored-by: Helena Kotas <[email protected]> * [0023] Proposal for separate counter resource handle (llvm#208) This is a proposal to create a separte handle for the counter resource in RWStructureBuffer and other that have associated counters. * [0002] RootSignatures - NFC Formatting Grammar to EBNF (llvm#198) Update the formatting of the root signature grammar to use EBNF in a consistent manner: - The first commit just goes through to use `=` instead of `:` and denote statements with ';' to comply with EBNF - The second commit comes from a suggestion [here](llvm#195 (comment)) to just use the `|` notation to specify the parameters in any order but not explicitly denote that they can only occur once Clean up pr to resolve llvm#192 --------- Co-authored-by: Finn Plummer <[email protected]> * Implicit resource binding - document current DXC behavior (llvm#193) Initial commit of the Implicit Resource Binding proposal that includes: - brief overview section on resource bindings - number of examples documenting how are implicit resource bindings are assigned in DXC Closes llvm#176 * Implicit resource bindings design (llvm#196) Proposed design for implicit resource bindings in Clang. Closes llvm#177 * Resource constructors proposal (llvm#197) Proposal for resource classes constructors. Closes llvm#209 * Remove tools/hlslpm (llvm#228) As the process evolves, it becomes more effort to keep tools/hlslpm up to date. This change removes tools/hlslpm rather than keep a stale version around. * Update issue_tracking.md for latest thinking (llvm#229) Describe epics, scenarios, deliverables and tasks, and the custom fields used in the HLSL Support project. --------- Co-authored-by: Finn Plummer <[email protected]> * Add "Review" value for the "blocked" field (llvm#257) Some issues are blocked on design review. This value can be used to identify them. * Resource binding: add another example and notes about resources in structs (llvm#214) Add one more example showing binding of resource arrays in user-defined structs. Add a note to re-evaluate whether to bind resource arrays in structs individually or as a continuous descriptor range later on when we start working on the resources in structs design (llvm/wg-hlsl#llvm#212). * [0026] Add proposal for symbol visibility. (llvm#272) Section 3.6 of the [HLSL specification](https://microsoft.github.io/hlsl-specs/specs/hlsl.pdf) defined the possible linkages for names. This proposal updates how these linkages are represented in LLVM IR. The current implementation presents challenges for the SPIR-V backend due to inconsistencies with OpenCL. In HLSL, a name can have external linkage and program linkage, among others. If a name has external linkage, it is visible outside the translation unit, but not outside a linked program. A name with program linkage is visible outside a partially linked program. We propose that names with program linkage in HLSL should have external linkage and default visibility in LLVM IR, while names with external linkage in HLSL should have external linkage and hidden visibility in LLVM IR. They both have external linkage because they are visible outside the translation unit. Default visibility means the name is visible outside a shared library (program). Hidden visibility means the name is not visible outside the shared library (program). * Update proposals for implicit binding and resource constructors to match implementation (llvm#282) * Add missing titles to proposals (llvm#283) Adding title to proposals that were missing one. * Unify new line endings to LF (llvm#286) Add .gitattributes and set new line endings to LF. Normalize existing files by running `git add --renormalize`. * [NNNN] Implementation for `vk::constant_id` (llvm#287) Design for the Clang implmentation of`vk::constant_id`. --------- Signed-off-by: Nathan Gauër <[email protected]> Co-authored-by: Finn Plummer <[email protected]> Co-authored-by: Justin Bogner <[email protected]> Co-authored-by: Helena Kotas <[email protected]> Co-authored-by: Cassandra Beckley <[email protected]> Co-authored-by: Steven Perron <[email protected]> Co-authored-by: Nathan Gauër <[email protected]> Co-authored-by: Finn Plummer <[email protected]> Co-authored-by: Ashley Coleman <[email protected]> Co-authored-by: Damyan Pepper <[email protected]> Co-authored-by: Finn Plummer <[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.
it appears the
offsetwas mistakenly dropped as discarding this information no longer allows the user to provide a manual offset from the start of the descriptors heap and allow for register aliasingadditionally, we do a small fix-up to update the flag values to be their correct default as we adjust the parameter values
Resolves #190