[0002] RootSignatures - Misc updates to spec from testing #195

inbelic · 2025-03-20T18:19:09Z

This issue addresses the discrepancies found from testing here.

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 #192

inbelic · 2025-03-20T18:20:54Z

proposals/0002-root-signature-in-clang.md

-               'LOCAL_ROOT_SIGNATURE' |
-               'CBV_SRV_UAV_HEAP_DIRECTLY_INDEXED' |
-               'SAMPLER_HEAP_DIRECTLY_INDEXED' |
-               'AllowLowTierReservedHwCbLimit'


We remove AllowLowTierReservedHwCbLimit from the specification in this pr. This was done as you can't specify the flag in dxc. I am not 100% sure this is correct and would like reviewer input.

I think it's ok to leave this out.

damyanp

Submitting my comments so far, but this would be easier to review if the reformatting change were pulled out into a separate PR so I don't have to look at individual commits.

damyanp · 2025-03-20T18:31:56Z

proposals/0002-root-signature-in-clang.md

-               'LOCAL_ROOT_SIGNATURE' |
-               'CBV_SRV_UAV_HEAP_DIRECTLY_INDEXED' |
-               'SAMPLER_HEAP_DIRECTLY_INDEXED' |
-               'AllowLowTierReservedHwCbLimit'


I think it's ok to leave this out.

damyanp · 2025-03-20T18:34:16Z

proposals/0002-root-signature-in-clang.md

+- RootConstants: `num32BitConstants`
+  A value of `num32BitConstants` that will cause the root signature size to
+  exceed 64 DWORDs of size can be validated. We can ensure that
+  `num32BitConstants <= 64 - (# of descriptors)`.


What does "# of descriptors" mean here? Is it the number of root parameters? Can you point me to where this root signature size limit is? I think I've seen root signatures much larger than 256 bytes.

Updated to be more descriptive and provide the source. For reference.

damyanp · 2025-03-20T18:34:55Z

proposals/0002-root-signature-in-clang.md

 - Resource ranges must not overlap.

  `CBV(b2), DescriptorTable(CBV(b0, numDescriptors=5))` will result in an error
  due to overlapping at b2.

+  Note that a valid value for `numDescriptors` is `unbounded` add requires


Typo - I think "add" should be "and"?

inbelic · 2025-03-20T19:01:48Z

Okay, it should be properly formatted now, for review. Created a very strange git diff initially. I will open the formatting pr once this one is resolved.

joaosaffran

This PR has the same changes as #191. Should it be targeting that branch instead?

proposals/0002-root-signature-in-clang.md

- through investigation it is better to let the D3D api handle validation of root signature size as it is target dependent and it is more capable to providing more context in error messages

inbelic · 2025-03-21T20:58:49Z

Good catch, yes it had the commits for offset as well. Have rebased to remove those commits

bogner · 2025-03-25T16:24:09Z

proposals/0002-root-signature-in-clang.md

+The root signature DSL is defined using a slightly modified version of Extended
+Backus-Naur form. We define the additional symbol `:` to denote a
+comma-seperated list of components in any order:  `A : B = (A ',' B | B ',' A)`.
+Additionally, all keywords and enums are case-insensitive.


Can't we define constructions like the following to do this kind of thing without extending EBNF?

value = 'A' | 'B' | 'C'; list = value, {',', value};

I suspect that will make this a little bit easier to read.

However, it probably makes sense to do that in a follow up as reading the diff of the grammar will be easier if it's NFC

Not quite, as the new notation tries to capture that they can be specified in any order and only once. We could ignore the only once this way in the grammar? Either way, we can move that discussion to the follow-up NFC.

joaosaffran

LGTM

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](#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 #192 --------- Co-authored-by: Finn Plummer <[email protected]>

* [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]>

github-project-automation bot added this to HLSL Support Mar 20, 2025

inbelic commented Mar 20, 2025

View reviewed changes

inbelic force-pushed the inbelic/rootsig-test-updates branch from dd1f616 to 8b9ad09 Compare March 20, 2025 18:33

damyanp reviewed Mar 20, 2025

View reviewed changes

inbelic force-pushed the inbelic/rootsig-test-updates branch from 8b9ad09 to 644eb3c Compare March 20, 2025 19:00

joaosaffran reviewed Mar 21, 2025

View reviewed changes

proposals/0002-root-signature-in-clang.md Outdated Show resolved Hide resolved

proposals/0002-root-signature-in-clang.md Show resolved Hide resolved

Finn Plummer added 8 commits March 21, 2025 20:56

use EBNF []{} optional notation

0146506

define a way to denote parameters in any-order

0a7e08b

make NUMBER more restrictive when possible

73ee00e

add unbounded to numDescriptors

0c78c3d

add validation of additional params

4e63545

remove 'AllowLowTierReservedHwCbLimit'

6bd7e98

self-review: move defintion of unbounded descriptors

abcfd7f

review: remove num32BitConstants from compiler validation

548704d

- through investigation it is better to let the D3D api handle validation of root signature size as it is target dependent and it is more capable to providing more context in error messages

inbelic force-pushed the inbelic/rootsig-test-updates branch from 644eb3c to 548704d Compare March 21, 2025 20:58

bogner approved these changes Mar 25, 2025

View reviewed changes

joaosaffran approved these changes Mar 25, 2025

View reviewed changes

inbelic merged commit 7a5d513 into llvm:main Mar 25, 2025

inbelic mentioned this pull request Mar 28, 2025

[0002] RootSignatures - NFC Formatting Grammar to EBNF #198

Merged

damyanp moved this to Closed in HLSL Support Apr 25, 2025

damyanp removed this from HLSL Support Jun 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[0002] RootSignatures - Misc updates to spec from testing #195

[0002] RootSignatures - Misc updates to spec from testing #195

Uh oh!

inbelic commented Mar 20, 2025 •

edited

Loading

Uh oh!

inbelic Mar 20, 2025 •

edited

Loading

Uh oh!

damyanp Mar 20, 2025

Uh oh!

damyanp left a comment

Uh oh!

damyanp Mar 20, 2025

Uh oh!

damyanp Mar 20, 2025

Uh oh!

inbelic Mar 20, 2025

Uh oh!

damyanp Mar 20, 2025

Uh oh!

inbelic commented Mar 20, 2025

Uh oh!

joaosaffran left a comment

Uh oh!

Uh oh!

Uh oh!

inbelic commented Mar 21, 2025

Uh oh!

bogner Mar 25, 2025

Uh oh!

inbelic Mar 25, 2025 •

edited

Loading

Uh oh!

joaosaffran left a comment

Uh oh!

Uh oh!

[0002] RootSignatures - Misc updates to spec from testing #195

[0002] RootSignatures - Misc updates to spec from testing #195

Uh oh!

Conversation

inbelic commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inbelic Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damyanp Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

damyanp Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

damyanp Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

inbelic Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

damyanp Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

inbelic commented Mar 20, 2025

Uh oh!

joaosaffran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

inbelic commented Mar 21, 2025

Uh oh!

bogner Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

inbelic Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaosaffran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

inbelic commented Mar 20, 2025 •

edited

Loading

inbelic Mar 20, 2025 •

edited

Loading

inbelic Mar 25, 2025 •

edited

Loading