[DirectX] Fix order of `v2::DescriptorRange` #145555

joaosaffran · 2025-06-24T17:23:58Z

As pointed in #145438, the order of elements in v2::DescriptorRange is wrong according to the root signature file format. This changes the order and updates the code and test to continue to pass.

Closes: #145438

llvmbot · 2025-06-24T17:24:26Z

@llvm/pr-subscribers-llvm-binary-utilities
@llvm/pr-subscribers-backend-directx

@llvm/pr-subscribers-mc

Author: None (joaosaffran)

Changes

As pointed in #145438, the order of elements in v2::DescriptorRange is wrong according to the root signature file format. This changes the order and updates the code and test to continue to pass.

Closes: #145438

Full diff: https://github.com/llvm/llvm-project/pull/145555.diff

6 Files Affected:

(modified) llvm/include/llvm/BinaryFormat/DXContainer.h (+11-2)
(modified) llvm/include/llvm/Object/DXContainer.h (+7-14)
(modified) llvm/lib/MC/DXContainerRootSignature.cpp (+2-2)
(modified) llvm/lib/ObjectYAML/DXContainerYAML.cpp (+49-28)
(modified) llvm/unittests/Object/DXContainerTest.cpp (+4-4)
(modified) llvm/unittests/ObjectYAML/DXContainerYAMLTest.cpp (+2-2)

diff --git a/llvm/include/llvm/BinaryFormat/DXContainer.h b/llvm/include/llvm/BinaryFormat/DXContainer.h
index 56c9e53308674..960720858f21b 100644
--- a/llvm/include/llvm/BinaryFormat/DXContainer.h
+++ b/llvm/include/llvm/BinaryFormat/DXContainer.h
@@ -740,10 +740,19 @@ struct RootDescriptor : public v1::RootDescriptor {
   }
 };
 
-struct DescriptorRange : public v1::DescriptorRange {
+struct DescriptorRange {
+  uint32_t RangeType;
+  uint32_t NumDescriptors;
+  uint32_t BaseShaderRegister;
+  uint32_t RegisterSpace;
   uint32_t Flags;
+  uint32_t OffsetInDescriptorsFromTableStart;
   void swapBytes() {
-    v1::DescriptorRange::swapBytes();
+    sys::swapByteOrder(RangeType);
+    sys::swapByteOrder(NumDescriptors);
+    sys::swapByteOrder(BaseShaderRegister);
+    sys::swapByteOrder(RegisterSpace);
+    sys::swapByteOrder(OffsetInDescriptorsFromTableStart);
     sys::swapByteOrder(Flags);
   }
 };
diff --git a/llvm/include/llvm/Object/DXContainer.h b/llvm/include/llvm/Object/DXContainer.h
index 4417061da936c..0fd8333c2e448 100644
--- a/llvm/include/llvm/Object/DXContainer.h
+++ b/llvm/include/llvm/Object/DXContainer.h
@@ -178,19 +178,14 @@ struct RootDescriptorView : RootParameterView {
     return readParameter<dxbc::RTS0::v2::RootDescriptor>();
   }
 };
-
-struct DescriptorTable {
+template <typename T> struct DescriptorTable {
   uint32_t NumRanges;
   uint32_t RangesOffset;
-  ViewArray<dxbc::RTS0::v2::DescriptorRange> Ranges;
+  ViewArray<T> Ranges;
 
-  typename ViewArray<dxbc::RTS0::v2::DescriptorRange>::iterator begin() const {
-    return Ranges.begin();
-  }
+  typename ViewArray<T>::iterator begin() const { return Ranges.begin(); }
 
-  typename ViewArray<dxbc::RTS0::v2::DescriptorRange>::iterator end() const {
-    return Ranges.end();
-  }
+  typename ViewArray<T>::iterator end() const { return Ranges.end(); }
 };
 
 struct DescriptorTableView : RootParameterView {
@@ -200,9 +195,9 @@ struct DescriptorTableView : RootParameterView {
   }
 
   // Define a type alias to access the template parameter from inside classof
-  llvm::Expected<DescriptorTable> read(uint32_t Version) {
+  template <typename T> llvm::Expected<DescriptorTable<T>> read() {
     const char *Current = ParamData.begin();
-    DescriptorTable Table;
+    DescriptorTable<T> Table;
 
     Table.NumRanges =
         support::endian::read<uint32_t, llvm::endianness::little>(Current);
@@ -212,9 +207,7 @@ struct DescriptorTableView : RootParameterView {
         support::endian::read<uint32_t, llvm::endianness::little>(Current);
     Current += sizeof(uint32_t);
 
-    size_t RangeSize = sizeof(dxbc::RTS0::v1::DescriptorRange);
-    if (Version > 1)
-      RangeSize = sizeof(dxbc::RTS0::v2::DescriptorRange);
+    size_t RangeSize = sizeof(T);
 
     Table.Ranges.Stride = RangeSize;
     Table.Ranges.Data =
diff --git a/llvm/lib/MC/DXContainerRootSignature.cpp b/llvm/lib/MC/DXContainerRootSignature.cpp
index 6c71823a51f85..0d51d96cfc7bf 100644
--- a/llvm/lib/MC/DXContainerRootSignature.cpp
+++ b/llvm/lib/MC/DXContainerRootSignature.cpp
@@ -133,10 +133,10 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
                                llvm::endianness::little);
         support::endian::write(BOS, Range.RegisterSpace,
                                llvm::endianness::little);
-        support::endian::write(BOS, Range.OffsetInDescriptorsFromTableStart,
-                               llvm::endianness::little);
         if (Version > 1)
           support::endian::write(BOS, Range.Flags, llvm::endianness::little);
+        support::endian::write(BOS, Range.OffsetInDescriptorsFromTableStart,
+                               llvm::endianness::little);
       }
       break;
     }
diff --git a/llvm/lib/ObjectYAML/DXContainerYAML.cpp b/llvm/lib/ObjectYAML/DXContainerYAML.cpp
index 3bca25f3fc6c6..bae837c0b9986 100644
--- a/llvm/lib/ObjectYAML/DXContainerYAML.cpp
+++ b/llvm/lib/ObjectYAML/DXContainerYAML.cpp
@@ -33,6 +33,39 @@ DXContainerYAML::ShaderFeatureFlags::ShaderFeatureFlags(uint64_t FlagData) {
 #include "llvm/BinaryFormat/DXContainerConstants.def"
 }
 
+template <typename T>
+static void
+addDescriptorRanges(DXContainerYAML::RootParameterHeaderYaml &Header,
+                    DXContainerYAML::RootSignatureYamlDesc &RootSigDesc,
+                    const object::DirectX::DescriptorTable<T> &Table) {
+
+  DXContainerYAML::RootParameterLocationYaml Location(Header);
+  DXContainerYAML::DescriptorTableYaml &TableYaml =
+      RootSigDesc.Parameters.getOrInsertTable(Location);
+  RootSigDesc.Parameters.insertLocation(Location);
+
+  TableYaml.NumRanges = Table.NumRanges;
+  TableYaml.RangesOffset = Table.RangesOffset;
+
+  for (const auto &R : Table.Ranges) {
+    DXContainerYAML::DescriptorRangeYaml NewR;
+    NewR.OffsetInDescriptorsFromTableStart =
+        R.OffsetInDescriptorsFromTableStart;
+    NewR.NumDescriptors = R.NumDescriptors;
+    NewR.BaseShaderRegister = R.BaseShaderRegister;
+    NewR.RegisterSpace = R.RegisterSpace;
+    NewR.RangeType = R.RangeType;
+    if constexpr (std::is_same_v<T, dxbc::RTS0::v2::DescriptorRange>) {
+      // Set all flag fields for v2
+#define DESCRIPTOR_RANGE_FLAG(Num, Val)                                        \
+  NewR.Val =                                                                   \
+      (R.Flags & llvm::to_underlying(dxbc::DescriptorRangeFlag::Val)) != 0;
+#include "llvm/BinaryFormat/DXContainerConstants.def"
+    }
+    TableYaml.Ranges.push_back(NewR);
+  }
+}
+
 llvm::Expected<DXContainerYAML::RootSignatureYamlDesc>
 DXContainerYAML::RootSignatureYamlDesc::create(
     const object::DirectX::RootSignature &Data) {
@@ -107,35 +140,23 @@ DXContainerYAML::RootSignatureYamlDesc::create(
       }
     } else if (auto *DTV =
                    dyn_cast<object::DirectX::DescriptorTableView>(&ParamView)) {
-      llvm::Expected<object::DirectX::DescriptorTable> TableOrErr =
-          DTV->read(Version);
-      if (Error E = TableOrErr.takeError())
-        return std::move(E);
-      auto Table = *TableOrErr;
-      RootParameterLocationYaml Location(Header);
-      DescriptorTableYaml &TableYaml =
-          RootSigDesc.Parameters.getOrInsertTable(Location);
-      RootSigDesc.Parameters.insertLocation(Location);
-
-      TableYaml.NumRanges = Table.NumRanges;
-      TableYaml.RangesOffset = Table.RangesOffset;
-
-      for (const auto &R : Table) {
-        DescriptorRangeYaml NewR;
 
-        NewR.OffsetInDescriptorsFromTableStart =
-            R.OffsetInDescriptorsFromTableStart;
-        NewR.NumDescriptors = R.NumDescriptors;
-        NewR.BaseShaderRegister = R.BaseShaderRegister;
-        NewR.RegisterSpace = R.RegisterSpace;
-        NewR.RangeType = R.RangeType;
-        if (Version > 1) {
-#define DESCRIPTOR_RANGE_FLAG(Num, Val)                                        \
-  NewR.Val =                                                                   \
-      (R.Flags & llvm::to_underlying(dxbc::DescriptorRangeFlag::Val)) > 0;
-#include "llvm/BinaryFormat/DXContainerConstants.def"
-        }
-        TableYaml.Ranges.push_back(NewR);
+      if (Version == 1) {
+        llvm::Expected<
+            object::DirectX::DescriptorTable<dxbc::RTS0::v1::DescriptorRange>>
+            TableOrErr = DTV->read<dxbc::RTS0::v1::DescriptorRange>();
+        if (Error E = TableOrErr.takeError())
+          return std::move(E);
+        auto Table = *TableOrErr;
+        addDescriptorRanges(Header, RootSigDesc, Table);
+      } else {
+        llvm::Expected<
+            object::DirectX::DescriptorTable<dxbc::RTS0::v2::DescriptorRange>>
+            TableOrErr = DTV->read<dxbc::RTS0::v2::DescriptorRange>();
+        if (Error E = TableOrErr.takeError())
+          return std::move(E);
+        auto Table = *TableOrErr;
+        addDescriptorRanges(Header, RootSigDesc, Table);
       }
     }
   }
diff --git a/llvm/unittests/Object/DXContainerTest.cpp b/llvm/unittests/Object/DXContainerTest.cpp
index 28012073d3c78..396d060a75bfd 100644
--- a/llvm/unittests/Object/DXContainerTest.cpp
+++ b/llvm/unittests/Object/DXContainerTest.cpp
@@ -1062,8 +1062,8 @@ TEST(RootSignature, ParseDescriptorTable) {
         0x3c, 0x00, 0x00, 0x00, 0x11, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
         0x03, 0x00, 0x00, 0x00, 0x24, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
         0x2c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff,
-        0x2a, 0x00, 0x00, 0x00, 0x2b, 0x00, 0x00, 0x00, 0x29, 0x00, 0x00, 0x00,
-        0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+        0x2a, 0x00, 0x00, 0x00, 0x2b, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00,
+        0x29, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
         0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
         0x00};
     DXContainer C =
@@ -1088,7 +1088,7 @@ TEST(RootSignature, ParseDescriptorTable) {
     auto *DescriptorTableView =
         dyn_cast<DirectX::DescriptorTableView>(&*ParamView);
     ASSERT_TRUE(DescriptorTableView != nullptr);
-    auto Table = DescriptorTableView->read(2);
+    auto Table = DescriptorTableView->read<dxbc::RTS0::v2::DescriptorRange>();
 
     ASSERT_THAT_ERROR(Table.takeError(), Succeeded());
 
@@ -1140,7 +1140,7 @@ TEST(RootSignature, ParseDescriptorTable) {
     auto *DescriptorTableView =
         dyn_cast<DirectX::DescriptorTableView>(&*ParamView);
     ASSERT_TRUE(DescriptorTableView != nullptr);
-    auto Table = DescriptorTableView->read(1);
+    auto Table = DescriptorTableView->read<dxbc::RTS0::v1::DescriptorRange>();
 
     ASSERT_THAT_ERROR(Table.takeError(), Succeeded());
 
diff --git a/llvm/unittests/ObjectYAML/DXContainerYAMLTest.cpp b/llvm/unittests/ObjectYAML/DXContainerYAMLTest.cpp
index 879712f833e13..9e6f7cc4f7dbd 100644
--- a/llvm/unittests/ObjectYAML/DXContainerYAMLTest.cpp
+++ b/llvm/unittests/ObjectYAML/DXContainerYAMLTest.cpp
@@ -459,8 +459,8 @@ TEST(RootSignature, ParseDescriptorTableV11) {
       0x3c, 0x00, 0x00, 0x00, 0x11, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
       0x03, 0x00, 0x00, 0x00, 0x24, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
       0x2c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff,
-      0x2a, 0x00, 0x00, 0x00, 0x2b, 0x00, 0x00, 0x00, 0x29, 0x00, 0x00, 0x00,
-      0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+      0x2a, 0x00, 0x00, 0x00, 0x2b, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00,
+      0x29, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
       0x00};

bogner · 2025-06-24T17:32:49Z

llvm/include/llvm/Object/DXContainer.h

+    size_t RangeSize = sizeof(T);

    Table.Ranges.Stride = RangeSize;


This shouldn't be necessary any more - I believe ViewArray sets the Stride correctly

bogner · 2025-06-24T18:09:35Z

llvm/lib/ObjectYAML/DXContainerYAML.cpp

+      if (Version == 1) {
+        llvm::Expected<
+            object::DirectX::DescriptorTable<dxbc::RTS0::v1::DescriptorRange>>
+            TableOrErr = DTV->read<dxbc::RTS0::v1::DescriptorRange>();
+        if (Error E = TableOrErr.takeError())
+          return std::move(E);
+        auto Table = *TableOrErr;
+        addDescriptorRanges(Header, RootSigDesc, Table);


It would simplify things a bit to move the call to read into the templated function. That is,

template <typename T> static llvm::Error readDescriptorRanges(DXContainerYAML::RootParameterHeaderYaml &Header, DXContainerYAML::RootSignatureYamlDesc &RootSigDesc, object::DirectX::DescriptorTableView *DTV) { llvm::Expected<object::DirectX::DescriptorTable<T>> Table = DTV->read<T>(); if (Error E = Table.takeError()) return std::move(E); // Use Table->NumRanges, Table->Ranges, etc. return Error::success();

bogner · 2025-06-24T18:11:22Z

llvm/lib/ObjectYAML/DXContainerYAML.cpp

+          return std::move(E);
+        auto Table = *TableOrErr;
+        addDescriptorRanges(Header, RootSigDesc, Table);
+      } else {


Might be better to explicitly not handle versions other than 1 or 2 so that if we add versions in the future and forget to update this it's very obvious. At this point we've validated that the version is something we expect, right? In that case, we can just throw an unreachable here.

if (Version == 1) { if (Error E = readDescriptorRanges<dxbc::RTS0::v1::DescriptorRange>( Header, RootSigDesc, DTV)) return std::move(E); } else if (Version == 2) { if (Error E = readDescriptorRanges<dxbc::RTS0::v2::DescriptorRange>( Header, RootSigDesc, DTV)) return std::move(E); } else llvm_unreachable("Unknown version for DescriptorRanges");

bogner · 2025-06-24T19:52:44Z

llvm/include/llvm/Object/DXContainer.h

    Table.Ranges.Data =
-        ParamData.substr(2 * sizeof(uint32_t), Table.NumRanges * RangeSize);
+        ParamData.substr(2 * sizeof(uint32_t), Table.NumRanges * sizeof(T));


Would it be clearer to use Table.NumRanges * Tabled.Ranges.Stride here?

inbelic · 2025-06-24T19:55:40Z

llvm/lib/ObjectYAML/DXContainerYAML.cpp

+                Header, RootSigDesc, DTV))
+          return std::move(E);
+      } else
+        llvm_unreachable("Unknown version for DescriptorRanges");


Not a change, but maybe such cases will be a nice reason to use the explicitly defined root signature enum when it is merged in.

inbelic · 2025-06-24T19:57:06Z

llvm/include/llvm/BinaryFormat/DXContainer.h

@@ -740,10 +740,19 @@ struct RootDescriptor : public v1::RootDescriptor {
  }
 };

-struct DescriptorRange : public v1::DescriptorRange {
+struct DescriptorRange {


Ah I see now. The old struct is still defined in the v1 namespace and this is defined in the v2 namespace.

As pointed in llvm#145438, the order of elements in `v2::DescriptorRange` is wrong according to the root signature file format. This changes the order and updates the code and test to continue to pass. Closes: llvm#145438 --------- Co-authored-by: joaosaffran <joao.saffran@microsoft.com>

joaosaffran added 3 commits June 24, 2025 16:51

fix bug

24cbe8b

make code more llvmish

f5949b7

clean up

cc55404

joaosaffran requested a review from bogner June 24, 2025 17:24

llvmbot added mc Machine (object) code backend:DirectX objectyaml llvm:binary-utilities labels Jun 24, 2025

joaosaffran requested a review from inbelic June 24, 2025 17:24

bogner reviewed Jun 24, 2025

View reviewed changes

address comments

e86dc8e

joaosaffran requested a review from bogner June 24, 2025 19:48

bogner approved these changes Jun 24, 2025

View reviewed changes

inbelic approved these changes Jun 24, 2025

View reviewed changes

address comments

ced4e8f

joaosaffran merged commit 770d0b0 into llvm:main Jun 24, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DirectX] Fix order of `v2::DescriptorRange` #145555

[DirectX] Fix order of `v2::DescriptorRange` #145555

Uh oh!

joaosaffran commented Jun 24, 2025

Uh oh!

llvmbot commented Jun 24, 2025 •

edited

Loading

Uh oh!

bogner Jun 24, 2025

Uh oh!

bogner Jun 24, 2025

Uh oh!

bogner Jun 24, 2025

Uh oh!

bogner Jun 24, 2025

Uh oh!

inbelic Jun 24, 2025

Uh oh!

inbelic Jun 24, 2025

Uh oh!

Uh oh!

Uh oh!

		size_t RangeSize = sizeof(T);

		Table.Ranges.Stride = RangeSize;

[DirectX] Fix order of v2::DescriptorRange #145555

[DirectX] Fix order of v2::DescriptorRange #145555

Uh oh!

Conversation

joaosaffran commented Jun 24, 2025

Uh oh!

llvmbot commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bogner Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

bogner Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

bogner Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

bogner Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

inbelic Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

inbelic Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[DirectX] Fix order of `v2::DescriptorRange` #145555

[DirectX] Fix order of `v2::DescriptorRange` #145555

llvmbot commented Jun 24, 2025 •

edited

Loading