[HLSL] Add descriptor table metadata parsing #142492

joaosaffran · 2025-06-02T22:03:04Z

adds parsing from metadata into dxcontainer binary
adds validations as described in the spec
adds testing scenarios
Closes: #126640

llvm/include/llvm/BinaryFormat/DXContainerConstants.def

inbelic · 2025-06-05T21:55:47Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+  if (std::optional<uint32_t> Val = extractMdIntValue(RangeDescriptorNode, 1))
+    Range.NumDescriptors = *Val;
+  else
+    return reportError(Ctx, "Invalid value for Number of Descriptor in Range");


We could probably have a function like reportInvalidTypeError for all of these. Similar to reportValueError but with a message that will differentiate between the two.

Something like "Parameter: ... expected metadata node of type ... but got ..." I am not sure if you can retrieve the type of an MDNode?

Have addressed this in a separate branch, will send a follow-up PR

llvm/lib/Target/DirectX/DXILRootSignature.cpp

inbelic · 2025-06-05T22:09:40Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

@@ -391,14 +567,12 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
    OS << "Definition for '" << F.getName() << "':\n";

    // start root signature header
-    Space++;


These don't seem like a related change? I guess it is to conform to use explicit idents everywhere instead of incremeting/decrementing?

Yeah, it is just to add consistency in the idents

inbelic · 2025-06-05T22:13:38Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+
+  if (Version == 2 &&
+      Type != llvm::to_underlying(dxbc::DescriptorRangeType::Sampler)) {
+    switch (Flags) {


Personally, I find this switch statement really difficult to follow along and verify it is doing what we want.

I am pretty sure it is implementing the logic from here.

Maybe a templated isFlagSet function and an if-else chain might help?

Or a comment describing/linking to the docs to help.

alsepkow · 2025-06-09T23:04:00Z

diff

@@ -0,0 +1,34 @@
+diff --git a/llvm/include/llvm/BinaryFormat/DXContainer.h b/llvm/include/llvm/BinaryFormat/DXContainer.h


Accidentally included this file?

alsepkow · 2025-06-09T23:10:08Z

llvm/include/llvm/BinaryFormat/DXContainerConstants.def

@@ -98,6 +98,17 @@ DESCRIPTOR_RANGE_FLAG(16, DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS)
 #undef DESCRIPTOR_RANGE_FLAG
 #endif // DESCRIPTOR_RANGE_FLAG

+// DESCRIPTOR_RANGE(value, name).
+#ifdef DESCRIPTOR_RANGE


nit: the other ifdef blocks have a new line after the ifdef.

alsepkow · 2025-06-09T23:24:50Z

return !(RegisterSpace >= 0xFFFFFFF0 && RegisterSpace <= 0xFFFFFFFF);

nit: I suggest naming to constants for improved readability. It is pretty easy to infer in this case but I think its a good habit. Up to you though.

Refers to: llvm/lib/Target/DirectX/DXILRootSignature.cpp:326 in 72ca44b. [](commit_id = 72ca44b, deletion_comment = False)

alsepkow · 2025-06-09T23:27:28Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+
+static bool verifyDescriptorRangeFlag(uint32_t Version, uint32_t Type,
+                                      uint32_t Flags) {
+  bool IsSampler =


Suggested change

bool IsSampler =

const bool IsSampler =

alsepkow · 2025-06-10T01:27:23Z

Space--;

Looks like you forgot to remove this?

Refers to: llvm/lib/Target/DirectX/DXILRootSignature.cpp:636 in 90f7403. [](commit_id = 90f7403, deletion_comment = False)

alsepkow · 2025-06-10T01:35:39Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

@@ -391,14 +565,12 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
    OS << "Definition for '" << F.getName() << "':\n";

    // start root signature header
-    Space++;
    OS << indent(Space) << "Flags: " << format_hex(RS.Flags, 8) << "\n";


given that space is now always 0 is it still needed?
I think you can remove the calls that are effectively 'indent(0)'

alsepkow · 2025-06-10T01:37:38Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+      case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable): {
+        const mcdxbc::DescriptorTable &Table =
+            RS.ParametersContainer.getDescriptorTable(Loc);
+        OS << indent(Space + 2) << "NumRanges: " << Table.Ranges.size() << "\n";


nit: Suggest avoiding 'magic numbers' and instead use named constants. I assume the 2 is intended to be a tab? Could use 'const uint8_t TabInSpaces = 1'.

alsepkow

Some minor suggestions from me. But the rest looks good.

bogner · 2025-06-11T15:46:33Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+  Header.ParameterType =
+      llvm::to_underlying(dxbc::RootParameterType::DescriptorTable);
+
+  for (unsigned int I = 2; I < DescriptorTableNode->getNumOperands(); I++) {


We calculate NumOperands earlier, probably best to store that and reuse it here instead of calling getNumOperands each time through the loop.

See https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

bogner · 2025-06-11T16:03:13Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+  if (Version == 1) {
+    if (IsSampler) {
+      return Flags == 0;
+    }
+    return Flags ==
+           llvm::to_underlying(dxbc::DescriptorRangeFlag::DESCRIPTORS_VOLATILE);
+  }


This doesn't make a lot of sense - version 1 doesn't have a flags field, so why are we verifying it? Furthermore, if this is meaningful to do, it doesn't match the spec in https://github.com/llvm/wg-hlsl/blob/main/proposals/0002-root-signature-in-clang.md#all-the-values-should-be-legal, which states that samplers must have the DESCRIPTORS_VOLATILE flag and SRVs/CBVs/UAVs must have the DATA_VOLATILE flag.

The metadata representation, for descriptor tables, always contains a flag values, so the spec expect me to verify that. I will fix the values though.

bogner · 2025-06-11T17:15:09Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+    return Flags == 0 ||
+           isFlagSet(Flags, dxbc::DescriptorRangeFlag::DESCRIPTORS_VOLATILE) ||
+           isFlagSet(Flags, dxbc::DescriptorRangeFlag::DATA_VOLATILE) ||
+           isFlagSet(Flags, dxbc::DescriptorRangeFlag::DATA_STATIC) ||
+           isFlagSet(
+               Flags,
+               dxbc::DescriptorRangeFlag::DATA_STATIC_WHILE_SET_AT_EXECUTE) ||
+           isFlagSet(Flags,
+                     dxbc::DescriptorRangeFlag::
+                         DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS) ||
+           isFlagSet(Flags, dxbc::DescriptorRangeFlag::DESCRIPTORS_VOLATILE,
+                     dxbc::DescriptorRangeFlag::DATA_VOLATILE) ||
+           isFlagSet(
+               Flags, dxbc::DescriptorRangeFlag::DESCRIPTORS_VOLATILE,
+               dxbc::DescriptorRangeFlag::DATA_STATIC_WHILE_SET_AT_EXECUTE) ||
+           isFlagSet(Flags,
+                     dxbc::DescriptorRangeFlag::
+                         DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS,
+                     dxbc::DescriptorRangeFlag::DATA_VOLATILE) ||
+           isFlagSet(Flags,
+                     dxbc::DescriptorRangeFlag::
+                         DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS,
+                     dxbc::DescriptorRangeFlag::DATA_STATIC) ||
+           isFlagSet(
+               Flags,
+               dxbc::DescriptorRangeFlag::
+                   DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS,
+               dxbc::DescriptorRangeFlag::DATA_STATIC_WHILE_SET_AT_EXECUTE);


I find this extremely hard to follow. I think it'd make things quite a bit simpler if we broke this down based on which DESCRIPTORS_ behaviours are set, and then check the DATA_ bits based on that.

That is, if DESCRIPTORS_VOLATILE is set, then the DATA_ flags can't contain DATA_STATIC, but for DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS or the default, the three DATA_* values are all fine. We'll also need to check that the DATA_* values are mutually exclusive presumably.

This also all becomes a little bit more readable if we (1) use BitmaskEnum.h so we can avoid a bunch of to_underlying calls:

// Earlier in DXContainer.h: LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); // Then define DescriptorRangeFlag like so: enum class DescriptorRangeFlag : uint32_t { #define DESCRIPTOR_RANGE_FLAG(Num, Val) Val = Num, #include "DXContainerConstants.def" LLVM_MARK_AS_BITMASK_ENUM(DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS) };

and then (2), define a local alias for dxbc::DescriptorRangeFlag to shorten things up.

Putting it all together, we get something like:

static bool verifyDescriptorRangeFlag(uint32_t Version, uint32_t Type, uint32_t FlagsVal) { using FlagT = dxbc::DescriptorRangeFlag; FlagT Flags = FlagT(FlagsVal); const bool IsSampler = (Type == llvm::to_underlying(dxbc::DescriptorRangeType::Sampler)); // The data-specific flags are mutually exclusive. FlagT DataFlags = FlagT::DATA_VOLATILE | FlagT::DATA_STATIC | FlagT::DATA_STATIC_WHILE_SET_AT_EXECUTE; if (popcount(llvm::to_underlying(Flags & DataFlags)) > 1) return false; // For volatile descriptors, DATA_STATIC is never valid. if ((Flags & FlagT::DESCRIPTORS_VOLATILE) == FlagT::DESCRIPTORS_VOLATILE) { FlagT Mask = FlagT::DESCRIPTORS_VOLATILE; if (!IsSampler) { Mask |= FlagT::DATA_VOLATILE; Mask |= FlagT::DATA_STATIC_WHILE_SET_AT_EXECUTE; } return (Flags & ~Mask) == FlagT::NONE; } // For default (static) or "STATIC_KEEPING_BUFFER_BOUNDS_CHECKS" descriptors, // the other data-specific flags may all be set. FlagT Mask = FlagT::DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS; if (!IsSampler) { Mask |= FlagT::DATA_VOLATILE; Mask |= FlagT::DATA_STATIC; Mask |= FlagT::DATA_STATIC_WHILE_SET_AT_EXECUTE; } return (Flags & ~Mask) == FlagT::NONE; }

Note that I don't think any of the current tests check for the exclusivity of DATA_ flags.

bogner · 2025-06-11T17:17:46Z

llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable.ll

+; DXC-NEXT:                BaseShaderRegister: 1
+; DXC-NEXT:                RegisterSpace:   10
+; DXC-NEXT:                OffsetInDescriptorsFromTableStart: 5
+; DXC-NEXT:                DESCRIPTORS_VOLATILE: true


Are the DESCRIPTORS_VOLATILE and DATA_VOLATILE values here correct? Might want to get this change in after #143201 to avoid conflicts.

…a/descriptor-table

inbelic · 2025-06-12T15:40:01Z

llvm/include/llvm/BinaryFormat/DXContainer.h

 enum class DescriptorRangeFlag : uint32_t {
 #include "DXContainerConstants.def"
+
+  LLVM_MARK_AS_BITMASK_ENUM(DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS)


review note: the value used in the macro is required to be the highest value

inbelic · 2025-06-12T15:41:08Z

llvm/include/llvm/BinaryFormat/DXContainerConstants.def

+DESCRIPTOR_RANGE(4, ERROR)
+DESCRIPTOR_RANGE(0, SRV)
+DESCRIPTOR_RANGE(1, UAV)
+DESCRIPTOR_RANGE(2, CBV)
+DESCRIPTOR_RANGE(3, Sampler)
+DESCRIPTOR_RANGE(0, NONE)
+#undef DESCRIPTOR_RANGE


I assume this is an artifact of not rebasing onto #143201?

Otherwise, the values here seem very odd.

inbelic · 2025-06-12T15:47:14Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+  if (popcount(llvm::to_underlying(Flags & DataFlags)) == 1)
+    return true;


I don't think this is correct. It seems we would skip further validation.

If Flags = DESCRIPTORS_VOLATILE | DATA_STATIC, then this will return early as valid.

I don't think this check is needed at all - once we've assured that at most one of these flags is set the logic below should handle the other cases.

This test fails llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-AllValidFlagCombinations.ll, because the DATA_* are allowed to be set without setting any DESCRIPTOR_* flag, so for example the metadata below errors with: error: Invalid value for DescriptorFlag: 2, even it is expected to be valid according to the spec: https://github.com/llvm/wg-hlsl/blob/main/proposals/0002-root-signature-in-clang.md#all-the-things-validated-in-sema.

!12 = !{ !"CBV", i32 5, i32 1, i32 7, i32 5, i32 2 }

bogner · 2025-06-12T16:21:47Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+          .Case("UAV", llvm::to_underlying(dxbc::DescriptorRangeType::UAV))
+          .Case("Sampler",
+                llvm::to_underlying(dxbc::DescriptorRangeType::Sampler))
+          .Default(llvm::to_underlying(dxbc::DescriptorRangeType::ERROR));


This doesn't look right:

I don't think we want an "ERROR" value in the DescriptorRangeType enum

Shouldn't we report an error here?

bogner · 2025-06-12T16:24:48Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+  if (popcount(llvm::to_underlying(Flags & DataFlags)) == 1)
+    return true;


I don't think this check is needed at all - once we've assured that at most one of these flags is set the logic below should handle the other cases.

bogner · 2025-06-12T16:27:01Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+
+  const bool IsSampler =
+      (Type == llvm::to_underlying(dxbc::DescriptorRangeType::Sampler));
+


In #142492 (comment) you pointed out that we do want the (simple) validation for version 1, but I don't see that happening any more. Shouldn't we be doing that here?

joaosaffran added 30 commits April 24, 2025 22:06

adding support for Root Descriptors

0abacfc

clean up

8b8c02a

addressing comments

7ac9641

formating

c105458

try fix test

efe76aa

addressing comments

a928e9d

refactoring mcdxbc struct to store root parameters out of order

a38f10b

changing name

9a7c359

changing variant to host pointers

d6c2b55

clean up

93e4cf2

fix

b45b1b6

fix

f804a23

fix naming

15eb6f5

fix naming

b9d7f07

addressing comments

46cc8c1

addressing comments

1b3e10a

addressing comments

1f31957

clean up

e8fbfce

removing v parameter

a31e5a5

Merge branch 'obj2yaml/root-descriptors' into refactoring/remove-union

a394ad0

clean up

ad415a7

Merge branch 'main' into refactoring/remove-union

8ff4845

adding support for root descriptors

f875555

removing none as a flag option

4f7f998

adding tests

3eb5e10

clean up and add more tests

58e1789

addressing comments

81915ad

Merge branch 'main' into metadata/root-descriptors

a515e28

clean

0d54162

cleanup

7f70dc5

inbelic reviewed Jun 5, 2025

View reviewed changes

addressing comments

72b8dbc

joaosaffran requested a review from inbelic June 9, 2025 18:28

joaosaffran added 2 commits June 9, 2025 18:33

addressing comments

440566c

address coments

72ca44b

alsepkow reviewed Jun 9, 2025

View reviewed changes

joaosaffran added 2 commits June 9, 2025 23:40

remove diff

90f7403

address comments

7a4382e

joaosaffran requested a review from alsepkow June 9, 2025 23:45

alsepkow reviewed Jun 10, 2025

View reviewed changes

bogner reviewed Jun 11, 2025

View reviewed changes

joaosaffran added 5 commits June 11, 2025 18:47

address comments

1c608f4

Merge branch 'bugfix/wrong-values-root-descriptor-flags' into metadat…

3b8b2e1

…a/descriptor-table

fix

e0bc5e3

Merge branch 'bugfix/wrong-values-root-descriptor-flags' into metadat…

655b3b4

…a/descriptor-table

adding more tests

91346a7

joaosaffran requested review from alsepkow and bogner June 11, 2025 23:27

inbelic reviewed Jun 12, 2025

View reviewed changes

bogner reviewed Jun 12, 2025

View reviewed changes

address comments

e8066df

joaosaffran changed the base branch from main to users/joaosaffran/143201 June 12, 2025 18:05

joaosaffran requested review from bogner and inbelic June 12, 2025 18:06

		@@ -0,0 +1,34 @@
		diff --git a/llvm/include/llvm/BinaryFormat/DXContainer.h b/llvm/include/llvm/BinaryFormat/DXContainer.h

		if (popcount(llvm::to_underlying(Flags & DataFlags)) == 1)
		return true;


		const bool IsSampler =
		(Type == llvm::to_underlying(dxbc::DescriptorRangeType::Sampler));

[HLSL] Add descriptor table metadata parsing #142492

Are you sure you want to change the base?

[HLSL] Add descriptor table metadata parsing #142492

Uh oh!

Conversation

joaosaffran commented Jun 2, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alsepkow commented Jun 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alsepkow commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alsepkow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!