[NFC] Refactoring MCDXBC to support out of order storage of root parameters #137284

joaosaffran · Apr 25, 2025

This PR refactors mcdxbc data structure for root signatures to support out of order storage of in memory root signature data.
closes: #139585

github-actions · Apr 25, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

llvmbot · May 2, 2025

@llvm/pr-subscribers-llvm-binary-utilities
@llvm/pr-subscribers-mc
@llvm/pr-subscribers-objectyaml

@llvm/pr-subscribers-backend-directx

Author: None (joaosaffran)

Changes

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

4 Files Affected:

(modified) llvm/include/llvm/MC/DXContainerRootSignature.h (+73-7)
(modified) llvm/lib/MC/DXContainerRootSignature.cpp (+42-35)
(modified) llvm/lib/ObjectYAML/DXContainerEmitter.cpp (+22-12)
(modified) llvm/lib/Target/DirectX/DXILRootSignature.cpp (+27-21)

diff --git a/llvm/include/llvm/MC/DXContainerRootSignature.h b/llvm/include/llvm/MC/DXContainerRootSignature.h
index 44e26c81eedc1..c8af613a57094 100644
--- a/llvm/include/llvm/MC/DXContainerRootSignature.h
+++ b/llvm/include/llvm/MC/DXContainerRootSignature.h
@@ -6,21 +6,87 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/STLForwardCompat.h"
 #include "llvm/BinaryFormat/DXContainer.h"
+#include <cstddef>
 #include <cstdint>
-#include <limits>
+#include <optional>
+#include <utility>
+#include <variant>
 
 namespace llvm {
 
 class raw_ostream;
 namespace mcdxbc {
 
-struct RootParameter {
+struct RootParameterInfo {
   dxbc::RootParameterHeader Header;
-  union {
-    dxbc::RootConstants Constants;
-    dxbc::RST0::v1::RootDescriptor Descriptor;
-  };
+  size_t Location;
+
+  RootParameterInfo() = default;
+
+  RootParameterInfo(dxbc::RootParameterHeader H, size_t L)
+      : Header(H), Location(L) {}
+};
+
+using RootDescriptor = std::variant<dxbc::RST0::v0::RootDescriptor,
+                                    dxbc::RST0::v1::RootDescriptor>;
+using ParametersView = std::variant<const dxbc::RootConstants *,
+                                    const dxbc::RST0::v0::RootDescriptor *,
+                                    const dxbc::RST0::v1::RootDescriptor *>;
+struct RootParametersContainer {
+  SmallVector<RootParameterInfo> ParametersInfo;
+
+  SmallVector<dxbc::RootConstants> Constants;
+  SmallVector<RootDescriptor> Descriptors;
+
+  void addInfo(dxbc::RootParameterHeader H, size_t L) {
+    ParametersInfo.push_back(RootParameterInfo(H, L));
+  }
+
+  void addParameter(dxbc::RootParameterHeader H, dxbc::RootConstants C) {
+    addInfo(H, Constants.size());
+    Constants.push_back(C);
+  }
+
+  void addParameter(dxbc::RootParameterHeader H,
+                    dxbc::RST0::v0::RootDescriptor D) {
+    addInfo(H, Descriptors.size());
+    Descriptors.push_back(D);
+  }
+
+  void addParameter(dxbc::RootParameterHeader H,
+                    dxbc::RST0::v1::RootDescriptor D) {
+    addInfo(H, Descriptors.size());
+    Descriptors.push_back(D);
+  }
+
+  std::optional<ParametersView> getParameter(const RootParameterInfo *H) const {
+    switch (H->Header.ParameterType) {
+    case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
+      return &Constants[H->Location];
+    case llvm::to_underlying(dxbc::RootParameterType::CBV):
+    case llvm::to_underlying(dxbc::RootParameterType::SRV):
+    case llvm::to_underlying(dxbc::RootParameterType::UAV):
+      const RootDescriptor &VersionedParam = Descriptors[H->Location];
+      if (std::holds_alternative<dxbc::RST0::v0::RootDescriptor>(
+              VersionedParam)) {
+        return &std::get<dxbc::RST0::v0::RootDescriptor>(VersionedParam);
+      }
+      return &std::get<dxbc::RST0::v1::RootDescriptor>(VersionedParam);
+    }
+
+    return std::nullopt;
+  }
+
+  size_t size() const { return ParametersInfo.size(); }
+
+  SmallVector<RootParameterInfo>::const_iterator begin() const {
+    return ParametersInfo.begin();
+  }
+  SmallVector<RootParameterInfo>::const_iterator end() const {
+    return ParametersInfo.end();
+  }
 };
 struct RootSignatureDesc {
 
@@ -29,7 +95,7 @@ struct RootSignatureDesc {
   uint32_t RootParameterOffset = 0U;
   uint32_t StaticSamplersOffset = 0u;
   uint32_t NumStaticSamplers = 0u;
-  SmallVector<mcdxbc::RootParameter> Parameters;
+  mcdxbc::RootParametersContainer ParametersContainer;
 
   void write(raw_ostream &OS) const;
 
diff --git a/llvm/lib/MC/DXContainerRootSignature.cpp b/llvm/lib/MC/DXContainerRootSignature.cpp
index 2693cb9943d5e..641c2f5fa1b1b 100644
--- a/llvm/lib/MC/DXContainerRootSignature.cpp
+++ b/llvm/lib/MC/DXContainerRootSignature.cpp
@@ -8,7 +8,9 @@
 
 #include "llvm/MC/DXContainerRootSignature.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/BinaryFormat/DXContainer.h"
 #include "llvm/Support/EndianStream.h"
+#include <variant>
 
 using namespace llvm;
 using namespace llvm::mcdxbc;
@@ -30,24 +32,20 @@ static void rewriteOffsetToCurrentByte(raw_svector_ostream &Stream,
 
 size_t RootSignatureDesc::getSize() const {
   size_t Size = sizeof(dxbc::RootSignatureHeader) +
-                Parameters.size() * sizeof(dxbc::RootParameterHeader);
+                ParametersContainer.size() * sizeof(dxbc::RootParameterHeader);
 
-  for (const mcdxbc::RootParameter &P : Parameters) {
-    switch (P.Header.ParameterType) {
-    case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
-      Size += sizeof(dxbc::RootConstants);
-      break;
-    case llvm::to_underlying(dxbc::RootParameterType::CBV):
-    case llvm::to_underlying(dxbc::RootParameterType::SRV):
-    case llvm::to_underlying(dxbc::RootParameterType::UAV):
-      if (Version == 1)
-        Size += sizeof(dxbc::RST0::v0::RootDescriptor);
-      else
-        Size += sizeof(dxbc::RST0::v1::RootDescriptor);
-
-      break;
-    }
+  for (const auto &I : ParametersContainer) {
+    std::optional<ParametersView> P = ParametersContainer.getParameter(&I);
+    if (!P)
+      continue;
+    std::visit(
+        [&Size](auto &Value) -> void {
+          using T = std::decay_t<decltype(*Value)>;
+          Size += sizeof(T);
+        },
+        *P);
   }
+
   return Size;
 }
 
@@ -56,7 +54,7 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
   raw_svector_ostream BOS(Storage);
   BOS.reserveExtraSpace(getSize());
 
-  const uint32_t NumParameters = Parameters.size();
+  const uint32_t NumParameters = ParametersContainer.size();
 
   support::endian::write(BOS, Version, llvm::endianness::little);
   support::endian::write(BOS, NumParameters, llvm::endianness::little);
@@ -66,7 +64,7 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
   support::endian::write(BOS, Flags, llvm::endianness::little);
 
   SmallVector<uint32_t> ParamsOffsets;
-  for (const mcdxbc::RootParameter &P : Parameters) {
+  for (const auto &P : ParametersContainer) {
     support::endian::write(BOS, P.Header.ParameterType,
                            llvm::endianness::little);
     support::endian::write(BOS, P.Header.ShaderVisibility,
@@ -76,29 +74,38 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
   }
 
   assert(NumParameters == ParamsOffsets.size());
-  for (size_t I = 0; I < NumParameters; ++I) {
+  const RootParameterInfo *H = ParametersContainer.begin();
+  for (size_t I = 0; I < NumParameters; ++I, H++) {
     rewriteOffsetToCurrentByte(BOS, ParamsOffsets[I]);
-    const mcdxbc::RootParameter &P = Parameters[I];
-
-    switch (P.Header.ParameterType) {
-    case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
-      support::endian::write(BOS, P.Constants.ShaderRegister,
+    auto P = ParametersContainer.getParameter(H);
+    if (!P)
+      continue;
+    if (std::holds_alternative<const dxbc::RootConstants *>(P.value())) {
+      auto *Constants = std::get<const dxbc::RootConstants *>(P.value());
+      support::endian::write(BOS, Constants->ShaderRegister,
                              llvm::endianness::little);
-      support::endian::write(BOS, P.Constants.RegisterSpace,
+      support::endian::write(BOS, Constants->RegisterSpace,
                              llvm::endianness::little);
-      support::endian::write(BOS, P.Constants.Num32BitValues,
+      support::endian::write(BOS, Constants->Num32BitValues,
                              llvm::endianness::little);
-      break;
-    case llvm::to_underlying(dxbc::RootParameterType::CBV):
-    case llvm::to_underlying(dxbc::RootParameterType::SRV):
-    case llvm::to_underlying(dxbc::RootParameterType::UAV):
-      support::endian::write(BOS, P.Descriptor.ShaderRegister,
+    } else if (std::holds_alternative<const dxbc::RST0::v0::RootDescriptor *>(
+                   *P)) {
+      auto *Descriptor =
+          std::get<const dxbc::RST0::v0::RootDescriptor *>(P.value());
+      support::endian::write(BOS, Descriptor->ShaderRegister,
+                             llvm::endianness::little);
+      support::endian::write(BOS, Descriptor->RegisterSpace,
+                             llvm::endianness::little);
+    } else if (std::holds_alternative<const dxbc::RST0::v1::RootDescriptor *>(
+                   *P)) {
+      auto *Descriptor =
+          std::get<const dxbc::RST0::v1::RootDescriptor *>(P.value());
+
+      support::endian::write(BOS, Descriptor->ShaderRegister,
                              llvm::endianness::little);
-      support::endian::write(BOS, P.Descriptor.RegisterSpace,
+      support::endian::write(BOS, Descriptor->RegisterSpace,
                              llvm::endianness::little);
-      if (Version > 1)
-        support::endian::write(BOS, P.Descriptor.Flags,
-                               llvm::endianness::little);
+      support::endian::write(BOS, Descriptor->Flags, llvm::endianness::little);
     }
   }
   assert(Storage.size() == getSize());
diff --git a/llvm/lib/ObjectYAML/DXContainerEmitter.cpp b/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
index 239ee9e3de9b1..b8ea1b048edfe 100644
--- a/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
+++ b/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
@@ -274,27 +274,37 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
       RS.StaticSamplersOffset = P.RootSignature->StaticSamplersOffset;
 
       for (const auto &Param : P.RootSignature->Parameters) {
-        mcdxbc::RootParameter NewParam;
-        NewParam.Header = dxbc::RootParameterHeader{
-            Param.Type, Param.Visibility, Param.Offset};
+        auto Header = dxbc::RootParameterHeader{Param.Type, Param.Visibility,
+                                                Param.Offset};
 
         switch (Param.Type) {
         case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
-          NewParam.Constants.Num32BitValues = Param.Constants.Num32BitValues;
-          NewParam.Constants.RegisterSpace = Param.Constants.RegisterSpace;
-          NewParam.Constants.ShaderRegister = Param.Constants.ShaderRegister;
+          dxbc::RootConstants Constants;
+          Constants.Num32BitValues = Param.Constants.Num32BitValues;
+          Constants.RegisterSpace = Param.Constants.RegisterSpace;
+          Constants.ShaderRegister = Param.Constants.ShaderRegister;
+          RS.ParametersContainer.addParameter(Header, Constants);
           break;
         case llvm::to_underlying(dxbc::RootParameterType::SRV):
         case llvm::to_underlying(dxbc::RootParameterType::UAV):
         case llvm::to_underlying(dxbc::RootParameterType::CBV):
-          NewParam.Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace;
-          NewParam.Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister;
-          if (P.RootSignature->Version > 1)
-            NewParam.Descriptor.Flags = Param.Descriptor.getEncodedFlags();
+          if (RS.Version == 1) {
+            dxbc::RST0::v0::RootDescriptor Descriptor;
+            Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace;
+            Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister;
+            RS.ParametersContainer.addParameter(Header, Descriptor);
+          } else {
+            dxbc::RST0::v1::RootDescriptor Descriptor;
+            Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace;
+            Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister;
+            Descriptor.Flags = Param.Descriptor.getEncodedFlags();
+          RS.ParametersContainer.addParameter(Header, Descriptor);
+          }
           break;
+        default:
+          // Handling invalid parameter type edge case
+          RS.ParametersContainer.addInfo(Header, -1);
         }
-
-        RS.Parameters.push_back(NewParam);
       }
 
       RS.write(OS);
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.cpp b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
index ef299c17baf76..30ca4d8f7c8ed 100644
--- a/llvm/lib/Target/DirectX/DXILRootSignature.cpp
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
@@ -30,6 +30,7 @@
 #include <cstdint>
 #include <optional>
 #include <utility>
+#include <variant>
 
 using namespace llvm;
 using namespace llvm::dxil;
@@ -75,31 +76,32 @@ static bool parseRootConstants(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD,
   if (RootConstantNode->getNumOperands() != 5)
     return reportError(Ctx, "Invalid format for RootConstants Element");
 
-  mcdxbc::RootParameter NewParameter;
-  NewParameter.Header.ParameterType =
+  dxbc::RootParameterHeader Header;
+  Header.ParameterType =
       llvm::to_underlying(dxbc::RootParameterType::Constants32Bit);
 
   if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 1))
-    NewParameter.Header.ShaderVisibility = *Val;
+    Header.ShaderVisibility = *Val;
   else
     return reportError(Ctx, "Invalid value for ShaderVisibility");
 
+  dxbc::RootConstants Constants;
   if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 2))
-    NewParameter.Constants.ShaderRegister = *Val;
+    Constants.ShaderRegister = *Val;
   else
     return reportError(Ctx, "Invalid value for ShaderRegister");
 
   if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 3))
-    NewParameter.Constants.RegisterSpace = *Val;
+    Constants.RegisterSpace = *Val;
   else
     return reportError(Ctx, "Invalid value for RegisterSpace");
 
   if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 4))
-    NewParameter.Constants.Num32BitValues = *Val;
+    Constants.Num32BitValues = *Val;
   else
     return reportError(Ctx, "Invalid value for Num32BitValues");
 
-  RSD.Parameters.push_back(NewParameter);
+  RSD.ParametersContainer.addParameter(Header, Constants);
 
   return false;
 }
@@ -164,12 +166,12 @@ static bool validate(LLVMContext *Ctx, const mcdxbc::RootSignatureDesc &RSD) {
     return reportValueError(Ctx, "RootFlags", RSD.Flags);
   }
 
-  for (const mcdxbc::RootParameter &P : RSD.Parameters) {
-    if (!dxbc::isValidShaderVisibility(P.Header.ShaderVisibility))
+  for (const llvm::mcdxbc::RootParameterInfo &Info : RSD.ParametersContainer) {
+    if (!dxbc::isValidShaderVisibility(Info.Header.ShaderVisibility))
       return reportValueError(Ctx, "ShaderVisibility",
-                              P.Header.ShaderVisibility);
+                              Info.Header.ShaderVisibility);
 
-    assert(dxbc::isValidParameterType(P.Header.ParameterType) &&
+    assert(dxbc::isValidParameterType(Info.Header.ParameterType) &&
            "Invalid value for ParameterType");
   }
 
@@ -287,22 +289,26 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
     OS << indent(Space) << "Version: " << RS.Version << "\n";
     OS << indent(Space) << "RootParametersOffset: " << RS.RootParameterOffset
        << "\n";
-    OS << indent(Space) << "NumParameters: " << RS.Parameters.size() << "\n";
+    OS << indent(Space) << "NumParameters: " << RS.ParametersContainer.size()
+       << "\n";
     Space++;
-    for (auto const &P : RS.Parameters) {
-      OS << indent(Space) << "- Parameter Type: " << P.Header.ParameterType
+    for (auto const &Info : RS.ParametersContainer) {
+      OS << indent(Space) << "- Parameter Type: " << Info.Header.ParameterType
          << "\n";
       OS << indent(Space + 2)
-         << "Shader Visibility: " << P.Header.ShaderVisibility << "\n";
-      switch (P.Header.ParameterType) {
-      case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
+         << "Shader Visibility: " << Info.Header.ShaderVisibility << "\n";
+      std::optional<mcdxbc::ParametersView> P =
+          RS.ParametersContainer.getParameter(&Info);
+      if (!P)
+        continue;
+      if (std::holds_alternative<const dxbc::RootConstants *>(*P)) {
+        auto *Constants = std::get<const dxbc::RootConstants *>(*P);
         OS << indent(Space + 2)
-           << "Register Space: " << P.Constants.RegisterSpace << "\n";
+           << "Register Space: " << Constants->RegisterSpace << "\n";
         OS << indent(Space + 2)
-           << "Shader Register: " << P.Constants.ShaderRegister << "\n";
+           << "Shader Register: " << Constants->ShaderRegister << "\n";
         OS << indent(Space + 2)
-           << "Num 32 Bit Values: " << P.Constants.Num32BitValues << "\n";
-        break;
+           << "Num 32 Bit Values: " << Constants->Num32BitValues << "\n";
       }
     }
     Space--;

inbelic · May 9, 2025

llvm/include/llvm/MC/DXContainerRootSignature.h

+  }
+
+  std::optional<ParametersView> getParameter(const RootParameterInfo *H) const {
+    switch (H->Header.ParameterType) {


Is this switch exhaustive? If it is then do we need to make the return optional?

Yes, is using an optional necessary here? Is the nullopt case reachable? If it should not be reachable likely best to use an llvm_unreachable instead.

Optional is necessary here, because yaml2obj needs to be able to write invalid root signatures representations, since we used it as a testing tool.

I don't understand, can you elaborate. What does writing an invalid root signature look like and how does it interact with this code?

Sure. Some context, Root Parameters are split into a Header and a Data section. The Header contains a RootParameter type field, specifying which kind of data is being stored: Root Constants, Root Descriptors or Descriptor Tables. The header also contains an offset, pointing to the exact location of the data in the binary file. Here is an test example showing what an invalid root signature look like: https://github.com/llvm/llvm-project/blob/038d357dde4907d39f6a3fabbaf48dc39cf9dc60/llvm/test/ObjectYAML/DXContainer/RootSignature-InvalidType.yaml.

Notice that in such test there is no data section, only the header section. Since I don't know what kind of data it is/the data is not supported, it is not possible to write it.

spall

Is this actually a strictly NFC change?

llvm/lib/MC/DXContainerRootSignature.cpp

llvm/lib/ObjectYAML/DXContainerEmitter.cpp

bogner · May 14, 2025

llvm/include/llvm/MC/DXContainerRootSignature.h

+  SmallVector<RootParameterInfo> ParametersInfo;
+
+  SmallVector<dxbc::RootConstants> Constants;
+  SmallVector<RootDescriptor> Descriptors;


We're not tied to the binary format itself here, so I think it would be quite a bit simpler to just have this vector contain the v2 descriptors:

SmallVector<dxbc::RTS0::v2::RootDescriptor> Descriptors;

This should end up simplifying RootSignatureDesc::write, as we have the version there, so we can just check the version for the later fields when we write it out:

support::endian::write(BOS, Descriptor.ShaderRegister, llvm::endianness::little); support::endian::write(BOS, Descriptor.RegisterSpace, llvm::endianness::little); if (Version > 1) support::endian::write(BOS, Descriptor.Flags, llvm::endianness::little);

bogner · May 14, 2025

llvm/include/llvm/MC/DXContainerRootSignature.h

+  std::optional<ParametersView> getParameter(const RootParameterInfo *H) const {
+    switch (H->Header.ParameterType) {
+    case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
+      return &Constants[H->Location];
+    case llvm::to_underlying(dxbc::RootParameterType::CBV):
+    case llvm::to_underlying(dxbc::RootParameterType::SRV):
+    case llvm::to_underlying(dxbc::RootParameterType::UAV):
+      const RootDescriptor &VersionedParam = Descriptors[H->Location];
+      if (std::holds_alternative<dxbc::RTS0::v1::RootDescriptor>(
+              VersionedParam)) {
+        return &std::get<dxbc::RTS0::v1::RootDescriptor>(VersionedParam);
+      }
+      return &std::get<dxbc::RTS0::v2::RootDescriptor>(VersionedParam);
+    }
+
+    return std::nullopt;
+  }


I'm not really convinced that the abstraction that the std::variant and the generic getParameter are providing are really helping much here. Consider this simplified API:

const std::pair<uint32_t, uint32_t> getTypeAndLocForParameter(uint32_t Index) const { const RootParameterInfo &Info = ParametersInfo[Index]; return {Info.Header.ParameterType, Info.Location}; } const dxbc::RootConstants &getConstant(size_t Index) const { return Constants[Index]; } const dxbc::RTS0::v2::RootDescriptor &getRootDescriptor(size_t Index) const { return Descriptors[Index]; }

The logic to use this is more or less the same - instead of the holds_alternative type checks we simply check against the type enum we already have:

- auto P = ParametersContainer.getParameter(ParametersContainer[I]); - if (std::holds_alternative<const dxbc::RootConstants *>(P.value())) { - auto *Constants = std::get<const dxbc::RootConstants *>(P.value()); - support::endian::write(BOS, Constants->ShaderRegister, - llvm::endianness::little); + const auto &[Type, Loc] = ParametersContainer.getTypeAndLocForParameter(I); + switch (Type) { + case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): { + const dxbc::RootConstants &Constants = + ParametersContainer.getConstant(Loc); + support::endian::write(BOS, Constants.ShaderRegister, + llvm::endianness::little);

I think std::variant has its uses when we need something akin to a type safe union or we want to use visitor patterns to handle a large number of cases, but if we're just going to switch over the types anyway I think it just adds a layer of abstraction that needs to be looked through when reading the code to understand it.

bogner · May 14, 2025

llvm/include/llvm/ObjectYAML/DXContainerYAML.h

-  RootParameterYamlDesc() {};
+  RootParameterYamlDesc(){};


Looks like an accidental reformat here.

bogner · May 14, 2025

llvm/lib/MC/DXContainerRootSignature.cpp

-  for (const mcdxbc::RootParameter &P : Parameters) {
-    switch (P.Header.ParameterType) {
-    case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
-      Size += sizeof(dxbc::RootConstants);
-      break;
-    case llvm::to_underlying(dxbc::RootParameterType::CBV):
-    case llvm::to_underlying(dxbc::RootParameterType::SRV):
-    case llvm::to_underlying(dxbc::RootParameterType::UAV):
-      if (Version == 1)
-        Size += sizeof(dxbc::RTS0::v1::RootDescriptor);
-      else
-        Size += sizeof(dxbc::RTS0::v2::RootDescriptor);
-
-      break;
-    }
+  for (const auto &I : ParametersContainer) {
+    std::optional<ParametersView> P = ParametersContainer.getParameter(&I);
+    if (!P)
+      continue;
+    std::visit(
+        [&Size](auto &Value) -> void {
+          using T = std::decay_t<decltype(*Value)>;
+          Size += sizeof(T);
+        },
+        *P);
  }


If you end up taking my suggestions for the simplified API above this will probably just revert to what it was before.

bogner · May 14, 2025

llvm/lib/MC/DXContainerRootSignature.cpp

-  for (const mcdxbc::RootParameter &P : Parameters) {
+  for (const auto &P : ParametersContainer) {


I don't think auto is better than writing out the type here.

llvm/lib/ObjectYAML/DXContainerEmitter.cpp

bogner · May 14, 2025

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+  dxbc::RootParameterHeader Header;
+  Header.ParameterType =
      llvm::to_underlying(dxbc::RootParameterType::Constants32Bit);

  if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 1))
-    NewParameter.Header.ShaderVisibility = *Val;
+    Header.ShaderVisibility = *Val;
  else
    return reportError(Ctx, "Invalid value for ShaderVisibility");


Header.ParameterOffset is left uninitialized. Bug?

There is no need to initialized Header.ParameterOffset, since those will be calculated when writing, in RootSignatureDesc::write

It looks like a bug when the structure is created here but then only partially initialized. We should probably have a comment here, or even write a zero to it with a comment that it will be overwritten later.

llvm/lib/Target/DirectX/DXILRootSignature.cpp

bogner · May 14, 2025

llvm/lib/MC/DXContainerRootSignature.cpp

@@ -76,29 +77,34 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
  }

  assert(NumParameters == ParamsOffsets.size());
-  for (size_t I = 0; I < NumParameters; ++I) {
+  const RootParameterInfo *H = ParametersContainer.begin();


H isn't used any more (it's merely incremented)

bogner · May 14, 2025

llvm/lib/MC/DXContainerRootSignature.cpp

-        support::endian::write(BOS, P.Descriptor.Flags,
-                               llvm::endianness::little);
+        support::endian::write(BOS, Descriptor.Flags, llvm::endianness::little);
+    }


missing break;?

bogner · May 14, 2025

llvm/lib/MC/DXContainerRootSignature.cpp

+      const dxbc::RootConstants Constants =
+          ParametersContainer.getConstant(Loc);


This should be const dxbc::RootConstants &Constants - no need to copy the object here. Same for Descriptor below.

bogner · May 14, 2025

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+
+      switch (Type) {
+      case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+        auto Constants = RS.ParametersContainer.getConstant(Loc);


Better to spell the type out here (also we should make this a const ref instead of copying it):

Suggested change

auto Constants = RS.ParametersContainer.getConstant(Loc);

const dxbc::RootConstants &Constants =

RS.ParametersContainer.getConstant(Loc);

bogner · May 15, 2025

llvm/include/llvm/MC/DXContainerRootSignature.h

+  RootParameterInfo(dxbc::RootParameterHeader H, size_t L)
+      : Header(H), Location(L) {}


Clearer to call these Header and Location rather than abbreviating here

Suggested change

RootParameterInfo(dxbc::RootParameterHeader H, size_t L)

: Header(H), Location(L) {}

RootParameterInfo(dxbc::RootParameterHeader Header, size_t Location)

: Header(Header), Location(Location) {}

bogner · May 15, 2025

llvm/include/llvm/MC/DXContainerRootSignature.h

+  void addInfo(dxbc::RootParameterHeader H, size_t L) {
+    ParametersInfo.push_back(RootParameterInfo(H, L));
+  }
+
+  void addParameter(dxbc::RootParameterHeader H, dxbc::RootConstants C) {
+    addInfo(H, Constants.size());
+    Constants.push_back(C);
+  }
+
+  void addParameter(dxbc::RootParameterHeader H,
+                    dxbc::RTS0::v2::RootDescriptor D) {
+    addInfo(H, Descriptors.size());
+    Descriptors.push_back(D);
+  }


Give all of these parameters actual names please

bogner · May 15, 2025

llvm/include/llvm/MC/DXContainerRootSignature.h

+  const dxbc::RootConstants &getConstant(size_t Index) const {
+    return Constants[Index];
+  }
+
+  const dxbc::RTS0::v2::RootDescriptor &getRootDescriptor(size_t Index) const {
+    return Descriptors[Index];
+  }


(I thought I commented on this already, but I think github ate it...)

Seeing these next to getTypeAndLocForParameter and getHeader, "Index" is maybe ambiguous as a variable name. Let's call these ones "Location" instead to differentiate that these are indices into the particular type of object not the index into headers used elsewhere

bogner · May 15, 2025

llvm/lib/MC/DXContainerRootSignature.cpp

                             llvm::endianness::little);
-      break;
+    } break;


The break statement should be inside the block here.

Suggested change

} break;

break;

}

bogner · May 15, 2025

llvm/lib/MC/DXContainerRootSignature.cpp

-        support::endian::write(BOS, P.Descriptor.Flags,
-                               llvm::endianness::little);
+        support::endian::write(BOS, Descriptor.Flags, llvm::endianness::little);
+    } break;


Suggested change

} break;

break;

}

bogner · May 15, 2025

llvm/lib/ObjectYAML/DXContainerEmitter.cpp

+        auto Header = dxbc::RootParameterHeader{Param.Type, Param.Visibility,
+                                                Param.Offset};


Suggested change

auto Header = dxbc::RootParameterHeader{Param.Type, Param.Visibility,

Param.Offset};

dxbc::RootParameterHeader Header{Param.Type, Param.Visibility,

Param.Offset};

bogner · May 15, 2025

llvm/lib/ObjectYAML/DXContainerEmitter.cpp

+          if (RS.Version == 1) {
+            dxbc::RTS0::v1::RootDescriptor Descriptor;
+            Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace;
+            Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister;
+            RS.ParametersContainer.addParameter(Header, Descriptor);
+          } else {
+            dxbc::RTS0::v2::RootDescriptor Descriptor;
+            Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace;
+            Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister;
+            Descriptor.Flags = Param.Descriptor.getEncodedFlags();
+            RS.ParametersContainer.addParameter(Header, Descriptor);
+          }


This seems clearer than the implicit cast to from the v1 to the v2 descriptor:

dxbc::RTS0::v2::RootDescriptor Descriptor; Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace; Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister; if (RS.Version > 1) Descriptor.Flags = Param.Descriptor.getEncodedFlags(); RS.ParametersContainer.addParameter(Header, Descriptor);

aside: We should probably make the v2::RootDescriptor constructor that takes a v1::RootDescriptor explicit - this implicit cast being legal seems dangerous

bogner · May 15, 2025

llvm/lib/ObjectYAML/DXContainerEmitter.cpp

+          // Handling invalid parameter type edge case. We intentionally let
+          // obj2yaml/yaml2obj parse and emit invalid dxcontainer data, in order
+          // for that to be used as a testing tool more effectively.
+          RS.ParametersContainer.addInfo(Header, -1);


I'm not sure, but do you think it's worth adding a addUnknownParameter or addInvalidParameter method instead of calling addInfo here directly for clarity?

I thinks that is a good idea. It can make the code more consistent. And reduce a little of the confusion regarding this edge case.

bogner · May 15, 2025

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+  // this will be properly calculated when writing it.
+  Header.ParameterOffset = 0;


I think this wording might be a little bit too brief. How about "The parameter offset doesn't matter here - we recalculate it during serialization"

joaosaffran added 2 commits April 24, 2025 22:06

adding support for Root Descriptors

0abacfc

clean up

8b8c02a

joaosaffran changed the title ~~[NFC] Refactoring MCDXBC to support out of order store of root parameters~~ [NFC] Refactoring MCDXBC to support out of order storage of root parameters Apr 25, 2025

joaosaffran assigned bogner Apr 25, 2025

joaosaffran added 8 commits April 25, 2025 22:28

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

joaosaffran force-pushed the refactoring/remove-union branch from 28aad01 to 93e4cf2 Compare April 28, 2025 22:48

fix

b45b1b6

joaosaffran changed the base branch from users/joaosaffran/137259 to main April 28, 2025 22:50

joaosaffran changed the base branch from main to users/joaosaffran/137259 April 28, 2025 22:51

fix

f804a23

joaosaffran closed this Apr 29, 2025

joaosaffran deleted the refactoring/remove-union branch April 29, 2025 18:50

joaosaffran restored the refactoring/remove-union branch April 29, 2025 18:50

joaosaffran reopened this Apr 29, 2025

joaosaffran marked this pull request as ready for review May 2, 2025 17:37

llvmbot added mc Machine (object) code backend:DirectX objectyaml labels May 2, 2025

joaosaffran assigned inbelic May 2, 2025

joaosaffran added 2 commits May 5, 2025 18:26

fix naming

15eb6f5

fix naming

b9d7f07

joaosaffran added 4 commits May 8, 2025 21:48

addressing comments

1f31957

clean up

e8fbfce

removing v parameter

a31e5a5

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

a394ad0

joaosaffran changed the base branch from users/joaosaffran/137259 to main May 9, 2025 00:25

joaosaffran changed the base branch from main to users/joaosaffran/137259 May 9, 2025 00:25

clean up

ad415a7

joaosaffran changed the base branch from users/joaosaffran/137259 to main May 9, 2025 06:18

Merge branch 'main' into refactoring/remove-union

8ff4845

joaosaffran requested review from inbelic and bogner May 9, 2025 06:36

inbelic reviewed May 9, 2025

View reviewed changes

spall reviewed May 12, 2025

View reviewed changes

llvm/lib/MC/DXContainerRootSignature.cpp Outdated Show resolved Hide resolved

llvm/lib/MC/DXContainerRootSignature.cpp Outdated Show resolved Hide resolved

llvm/lib/ObjectYAML/DXContainerEmitter.cpp Outdated Show resolved Hide resolved

bogner reviewed May 14, 2025

View reviewed changes

llvm/lib/Target/DirectX/DXILRootSignature.cpp Outdated Show resolved Hide resolved

joaosaffran added 2 commits May 14, 2025 22:19

addressing comment

d67f7d3

clean up

5453ad0

joaosaffran requested review from bogner and inbelic May 14, 2025 22:29

joaosaffran added 2 commits May 14, 2025 22:58

format

836a8a8

adding comment

5bd57a6

bogner reviewed May 14, 2025

View reviewed changes

joaosaffran added 3 commits May 14, 2025 23:19

adrresing comments

960cb9c

clean up

a60c7a3

formatting

2a4c2cb

bogner reviewed May 15, 2025

View reviewed changes

addressing comments

c29d3f2

bogner approved these changes May 15, 2025

View reviewed changes

joaosaffran merged commit 8d63afb into llvm:main May 16, 2025
10 of 12 checks passed

bogner mentioned this pull request May 23, 2025

[NFC] Refactoring DXContainerYaml Root Parameter representation #138318

Merged

		for (const mcdxbc::RootParameter &P : Parameters) {
		for (const auto &P : ParametersContainer) {

		const dxbc::RootConstants Constants =
		ParametersContainer.getConstant(Loc);

	auto Constants = RS.ParametersContainer.getConstant(Loc);
	const dxbc::RootConstants &Constants =
	RS.ParametersContainer.getConstant(Loc);

		RootParameterInfo(dxbc::RootParameterHeader H, size_t L)
		: Header(H), Location(L) {}

		auto Header = dxbc::RootParameterHeader{Param.Type, Param.Visibility,
		Param.Offset};

		// this will be properly calculated when writing it.
		Header.ParameterOffset = 0;

Search code, repositories, users, issues, pull requests...

[NFC] Refactoring MCDXBC to support out of order storage of root parameters #137284

[NFC] Refactoring MCDXBC to support out of order storage of root parameters #137284

Uh oh!

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

spall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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!

Uh oh!

joaosaffran commented Apr 25, 2025 •

edited

Loading

github-actions bot commented Apr 25, 2025 •

edited

Loading

llvmbot commented May 2, 2025 •

edited

Loading