[DirectX] Refactor RootSignature Backend to remove `to_underlying` from Root Parameter Header #154249

joaosaffran · 2025-08-19T01:53:07Z

This patch is refactoring Root Parameter Header in DX Container backend to remove the usage of to_underlying. This requires some changes: first, MC Root Signature should not depend on Object/DXContainer.h; Second, we need to assume data to be valid in scenarios where it was originally not expected, this made some tests be removed.

bogner · 2025-08-19T15:34:53Z

llvm/include/llvm/MC/DXContainerRootSignature.h

+struct RootParameterHeader {
+  dxbc::RootParameterType ParameterType;
+  dxbc::ShaderVisibility ShaderVisibility;
+  uint32_t ParameterOffset;
+};
+
 struct RootParameterInfo {
-  dxbc::RTS0::v1::RootParameterHeader Header;
+  RootParameterHeader Header;
  size_t Location;

  RootParameterInfo() = default;

-  RootParameterInfo(dxbc::RTS0::v1::RootParameterHeader Header, size_t Location)
+  RootParameterInfo(RootParameterHeader Header, size_t Location)
      : Header(Header), Location(Location) {}
 };


At this point I think it might simplify things to just have the Info object and forgo the local header type completely, as in

struct RootParameterInfo { dxbc::RootParameterType Type; dxbc::ShaderVisibility Visibility; uint32_t Offset; size_t Location; RootParameterInfo(dxbc::RootParameterType Type, dxbc::ShaderVisibility Visibility, uint32_t Offset, size_t Location) : Type(Type), Visibility(Visibility), Offset(Offset), Location(Location) { } };

then the addParameter functions below can just take Type/Visibility/Offset parameters directly instead of a Header object.

bogner · 2025-08-19T15:36:21Z

llvm/include/llvm/MC/DXContainerRootSignature.h


-  std::pair<uint32_t, uint32_t>
+  std::pair<dxbc::RootParameterType, uint32_t>
  getTypeAndLocForParameter(uint32_t Location) const {
    const RootParameterInfo &Info = ParametersInfo[Location];
    return {Info.Header.ParameterType, Info.Location};
  }

-  const dxbc::RTS0::v1::RootParameterHeader &getHeader(size_t Location) const {
+  const RootParameterHeader &getHeader(size_t Location) const {
    const RootParameterInfo &Info = ParametersInfo[Location];
    return Info.Header;
  }


Could probably replace both of these with a getInfo and access the fields of the struct directly.

bogner · 2025-08-19T15:51:00Z

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

+    if (!dxbc::isValidShaderVisibility(*Val))
+      return make_error<RootSignatureValidationError<uint32_t>>(
+          "ShaderVisibility", *Val);
+    return static_cast<dxbc::ShaderVisibility>(*Val);


I think constructor syntax is more usual for creating an enum from an underlying value than static cast:

Suggested change

return static_cast<dxbc::ShaderVisibility>(*Val);

return dxbc::ShaderVisibility(*Val);

bogner · 2025-08-19T16:02:20Z

llvm/lib/ObjectYAML/DXContainerEmitter.cpp

+        mcdxbc::RootParameterHeader Header{
+            static_cast<dxbc::RootParameterType>(L.Header.Type),
+            static_cast<dxbc::ShaderVisibility>(L.Header.Visibility),
+            L.Header.Offset};


This will become moot after the next step (updating the YAML representation), but it would be good to make our assumptions clear here. A couple of asserts should do:

assert(dxbc::isValidParameterType(L.Header.Type && "invalid DXContainer YAML"); assert(dxbc::isValidShaderVisibility(L.Header.Visibility && "invalid DXContainer YAML"); dxbc::RootParameterType Type(L.Header.Type); dxbc::ShaderVisibility Visibility(L.Header.Visibility);

I've added those assert in some point. The problem I faced was: this asserts would make me remove 2 tests, those would be removed eventually since, the yaml representation makes them impossible, but in order to keep the NFC in this PR I opted to keep the invalid parameter handling. But I do agree, it is better to make those assumptions explicit.

That's fair, but since we're casting without the check here those tests are presumably hitting UB now anyway - I think it'll be better to assert and just preemptively remove those tests rather than jump through hoops here.

bogner · 2025-08-19T16:02:58Z

llvm/lib/ObjectYAML/DXContainerEmitter.cpp

+            static_cast<dxbc::ShaderVisibility>(L.Header.Visibility),
+            L.Header.Offset};
+
+        switch (Header.ParameterType) {


Note that the default label in this switch becomes unreachable when we're switching over the enum itself, so it will need to be removed.

bogner · 2025-08-19T16:04:54Z

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

+;CHECK-NEXT: Flags: 0x000001
+;CHECK-NEXT: Version: 2
+;CHECK-NEXT: RootParametersOffset: 24
+;CHECK-NEXT: NumParameters: 3
+;CHECK-NEXT: - Parameter Type: Constants32Bit
+;CHECK-NEXT:   Shader Visibility: All


Try not to change the whitespace here, it makes it harder to tell what's changed.

llvmbot · 2025-08-19T17:55:36Z

@llvm/pr-subscribers-backend-directx

@llvm/pr-subscribers-objectyaml

Author: None (joaosaffran)

Changes

This patch is refactoring Root Parameter Header in DX Container backend to remove the usage of to_underlying. This requires some changes: first, MC Root Signature should not depend on Object/DXContainer.h; Second, we need to assume data to be valid in scenarios where it was originally not expected, this made some tests be removed.

Patch is 26.00 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/154249.diff

9 Files Affected:

(modified) llvm/include/llvm/MC/DXContainerRootSignature.h (+25-24)
(modified) llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp (+43-40)
(modified) llvm/lib/MC/DXContainerRootSignature.cpp (+18-18)
(modified) llvm/lib/ObjectYAML/DXContainerEmitter.cpp (+18-16)
(modified) llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp (+5-7)
(modified) llvm/lib/Target/DirectX/DXILRootSignature.cpp (+14-14)
(modified) llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Parameters.ll (+6-6)
(removed) llvm/test/ObjectYAML/DXContainer/RootSignature-InvalidType.yaml (-29)
(removed) llvm/test/ObjectYAML/DXContainer/RootSignature-InvalidVisibility.yaml (-33)

diff --git a/llvm/include/llvm/MC/DXContainerRootSignature.h b/llvm/include/llvm/MC/DXContainerRootSignature.h
index 3c7c886e79fc3..149de9fbd8b47 100644
--- a/llvm/include/llvm/MC/DXContainerRootSignature.h
+++ b/llvm/include/llvm/MC/DXContainerRootSignature.h
@@ -19,14 +19,22 @@ namespace llvm {
 class raw_ostream;
 namespace mcdxbc {
 
+struct RootParameterHeader {
+  dxbc::RootParameterType ParameterType;
+  dxbc::ShaderVisibility ShaderVisibility;
+  uint32_t ParameterOffset;
+};
+
 struct RootParameterInfo {
-  dxbc::RTS0::v1::RootParameterHeader Header;
+  dxbc::RootParameterType Type;
+  dxbc::ShaderVisibility Visibility;
   size_t Location;
 
   RootParameterInfo() = default;
 
-  RootParameterInfo(dxbc::RTS0::v1::RootParameterHeader Header, size_t Location)
-      : Header(Header), Location(Location) {}
+  RootParameterInfo(dxbc::RootParameterType Type,
+                    dxbc::ShaderVisibility Visibility, size_t Location)
+      : Type(Type), Visibility(Visibility), Location(Location) {}
 };
 
 struct DescriptorTable {
@@ -46,41 +54,34 @@ struct RootParametersContainer {
   SmallVector<dxbc::RTS0::v2::RootDescriptor> Descriptors;
   SmallVector<DescriptorTable> Tables;
 
-  void addInfo(dxbc::RTS0::v1::RootParameterHeader Header, size_t Location) {
-    ParametersInfo.push_back(RootParameterInfo(Header, Location));
+  void addInfo(dxbc::RootParameterType Type, dxbc::ShaderVisibility Visibility,
+               size_t Location) {
+    ParametersInfo.push_back(RootParameterInfo(Type, Visibility, Location));
   }
 
-  void addParameter(dxbc::RTS0::v1::RootParameterHeader Header,
+  void addParameter(dxbc::RootParameterType Type,
+                    dxbc::ShaderVisibility Visibility,
                     dxbc::RTS0::v1::RootConstants Constant) {
-    addInfo(Header, Constants.size());
+    addInfo(Type, Visibility, Constants.size());
     Constants.push_back(Constant);
   }
 
-  void addInvalidParameter(dxbc::RTS0::v1::RootParameterHeader Header) {
-    addInfo(Header, -1);
-  }
-
-  void addParameter(dxbc::RTS0::v1::RootParameterHeader Header,
+  void addParameter(dxbc::RootParameterType Type,
+                    dxbc::ShaderVisibility Visibility,
                     dxbc::RTS0::v2::RootDescriptor Descriptor) {
-    addInfo(Header, Descriptors.size());
+    addInfo(Type, Visibility, Descriptors.size());
     Descriptors.push_back(Descriptor);
   }
 
-  void addParameter(dxbc::RTS0::v1::RootParameterHeader Header,
-                    DescriptorTable Table) {
-    addInfo(Header, Tables.size());
+  void addParameter(dxbc::RootParameterType Type,
+                    dxbc::ShaderVisibility Visibility, DescriptorTable Table) {
+    addInfo(Type, Visibility, Tables.size());
     Tables.push_back(Table);
   }
 
-  std::pair<uint32_t, uint32_t>
-  getTypeAndLocForParameter(uint32_t Location) const {
-    const RootParameterInfo &Info = ParametersInfo[Location];
-    return {Info.Header.ParameterType, Info.Location};
-  }
-
-  const dxbc::RTS0::v1::RootParameterHeader &getHeader(size_t Location) const {
+  const RootParameterInfo &getInfo(uint32_t Location) const {
     const RootParameterInfo &Info = ParametersInfo[Location];
-    return Info.Header;
+    return Info;
   }
 
   const dxbc::RTS0::v1::RootConstants &getConstant(size_t Index) const {
diff --git a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
index 157bfc665b207..4dbcf05fa5668 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
@@ -51,6 +51,17 @@ static std::optional<StringRef> extractMdStringValue(MDNode *Node,
   return NodeText->getString();
 }
 
+static Expected<dxbc::ShaderVisibility>
+extractShaderVisibility(MDNode *Node, unsigned int OpId) {
+  if (std::optional<uint32_t> Val = extractMdIntValue(Node, OpId)) {
+    if (!dxbc::isValidShaderVisibility(*Val))
+      return make_error<RootSignatureValidationError<uint32_t>>(
+          "ShaderVisibility", *Val);
+    return dxbc::ShaderVisibility(*Val);
+  }
+  return make_error<InvalidRSMetadataValue>("ShaderVisibility");
+}
+
 namespace {
 
 // We use the OverloadVisit with std::visit to ensure the compiler catches if a
@@ -224,15 +235,12 @@ Error MetadataParser::parseRootConstants(mcdxbc::RootSignatureDesc &RSD,
   if (RootConstantNode->getNumOperands() != 5)
     return make_error<InvalidRSMetadataFormat>("RootConstants Element");
 
-  dxbc::RTS0::v1::RootParameterHeader Header;
-  // The parameter offset doesn't matter here - we recalculate it during
-  // serialization  Header.ParameterOffset = 0;
-  Header.ParameterType = to_underlying(dxbc::RootParameterType::Constants32Bit);
+  mcdxbc::RootParameterHeader Header;
 
-  if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 1))
-    Header.ShaderVisibility = *Val;
-  else
-    return make_error<InvalidRSMetadataValue>("ShaderVisibility");
+  Expected<dxbc::ShaderVisibility> VisibilityOrErr =
+      extractShaderVisibility(RootConstantNode, 1);
+  if (auto E = VisibilityOrErr.takeError())
+    return Error(std::move(E));
 
   dxbc::RTS0::v1::RootConstants Constants;
   if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 2))
@@ -250,7 +258,8 @@ Error MetadataParser::parseRootConstants(mcdxbc::RootSignatureDesc &RSD,
   else
     return make_error<InvalidRSMetadataValue>("Num32BitValues");
 
-  RSD.ParametersContainer.addParameter(Header, Constants);
+  RSD.ParametersContainer.addParameter(dxbc::RootParameterType::Constants32Bit,
+                                       *VisibilityOrErr, Constants);
 
   return Error::success();
 }
@@ -266,26 +275,26 @@ Error MetadataParser::parseRootDescriptors(
   if (RootDescriptorNode->getNumOperands() != 5)
     return make_error<InvalidRSMetadataFormat>("Root Descriptor Element");
 
-  dxbc::RTS0::v1::RootParameterHeader Header;
+  dxbc::RootParameterType Type;
   switch (ElementKind) {
   case RootSignatureElementKind::SRV:
-    Header.ParameterType = to_underlying(dxbc::RootParameterType::SRV);
+    Type = dxbc::RootParameterType::SRV;
     break;
   case RootSignatureElementKind::UAV:
-    Header.ParameterType = to_underlying(dxbc::RootParameterType::UAV);
+    Type = dxbc::RootParameterType::UAV;
     break;
   case RootSignatureElementKind::CBV:
-    Header.ParameterType = to_underlying(dxbc::RootParameterType::CBV);
+    Type = dxbc::RootParameterType::CBV;
     break;
   default:
     llvm_unreachable("invalid Root Descriptor kind");
     break;
   }
 
-  if (std::optional<uint32_t> Val = extractMdIntValue(RootDescriptorNode, 1))
-    Header.ShaderVisibility = *Val;
-  else
-    return make_error<InvalidRSMetadataValue>("ShaderVisibility");
+  Expected<dxbc::ShaderVisibility> VisibilityOrErr =
+      extractShaderVisibility(RootDescriptorNode, 1);
+  if (auto E = VisibilityOrErr.takeError())
+    return Error(std::move(E));
 
   dxbc::RTS0::v2::RootDescriptor Descriptor;
   if (std::optional<uint32_t> Val = extractMdIntValue(RootDescriptorNode, 2))
@@ -299,7 +308,7 @@ Error MetadataParser::parseRootDescriptors(
     return make_error<InvalidRSMetadataValue>("RegisterSpace");
 
   if (RSD.Version == 1) {
-    RSD.ParametersContainer.addParameter(Header, Descriptor);
+    RSD.ParametersContainer.addParameter(Type, *VisibilityOrErr, Descriptor);
     return Error::success();
   }
   assert(RSD.Version > 1);
@@ -309,7 +318,7 @@ Error MetadataParser::parseRootDescriptors(
   else
     return make_error<InvalidRSMetadataValue>("Root Descriptor Flags");
 
-  RSD.ParametersContainer.addParameter(Header, Descriptor);
+  RSD.ParametersContainer.addParameter(Type, *VisibilityOrErr, Descriptor);
   return Error::success();
 }
 
@@ -375,15 +384,14 @@ Error MetadataParser::parseDescriptorTable(mcdxbc::RootSignatureDesc &RSD,
   if (NumOperands < 2)
     return make_error<InvalidRSMetadataFormat>("Descriptor Table");
 
-  dxbc::RTS0::v1::RootParameterHeader Header;
-  if (std::optional<uint32_t> Val = extractMdIntValue(DescriptorTableNode, 1))
-    Header.ShaderVisibility = *Val;
-  else
-    return make_error<InvalidRSMetadataValue>("ShaderVisibility");
+  mcdxbc::RootParameterHeader Header;
+
+  Expected<dxbc::ShaderVisibility> VisibilityOrErr =
+      extractShaderVisibility(DescriptorTableNode, 1);
+  if (auto E = VisibilityOrErr.takeError())
+    return Error(std::move(E));
 
   mcdxbc::DescriptorTable Table;
-  Header.ParameterType =
-      to_underlying(dxbc::RootParameterType::DescriptorTable);
 
   for (unsigned int I = 2; I < NumOperands; I++) {
     MDNode *Element = dyn_cast<MDNode>(DescriptorTableNode->getOperand(I));
@@ -395,7 +403,8 @@ Error MetadataParser::parseDescriptorTable(mcdxbc::RootSignatureDesc &RSD,
       return Err;
   }
 
-  RSD.ParametersContainer.addParameter(Header, Table);
+  RSD.ParametersContainer.addParameter(dxbc::RootParameterType::DescriptorTable,
+                                       *VisibilityOrErr, Table);
   return Error::success();
 }
 
@@ -531,20 +540,14 @@ Error MetadataParser::validateRootSignature(
   }
 
   for (const mcdxbc::RootParameterInfo &Info : RSD.ParametersContainer) {
-    if (!dxbc::isValidShaderVisibility(Info.Header.ShaderVisibility))
-      DeferredErrs =
-          joinErrors(std::move(DeferredErrs),
-                     make_error<RootSignatureValidationError<uint32_t>>(
-                         "ShaderVisibility", Info.Header.ShaderVisibility));
-
-    assert(dxbc::isValidParameterType(Info.Header.ParameterType) &&
-           "Invalid value for ParameterType");
 
-    switch (Info.Header.ParameterType) {
+    switch (Info.Type) {
+    case dxbc::RootParameterType::Constants32Bit:
+      break;
 
-    case to_underlying(dxbc::RootParameterType::CBV):
-    case to_underlying(dxbc::RootParameterType::UAV):
-    case to_underlying(dxbc::RootParameterType::SRV): {
+    case dxbc::RootParameterType::CBV:
+    case dxbc::RootParameterType::UAV:
+    case dxbc::RootParameterType::SRV: {
       const dxbc::RTS0::v2::RootDescriptor &Descriptor =
           RSD.ParametersContainer.getRootDescriptor(Info.Location);
       if (!hlsl::rootsig::verifyRegisterValue(Descriptor.ShaderRegister))
@@ -569,7 +572,7 @@ Error MetadataParser::validateRootSignature(
       }
       break;
     }
-    case to_underlying(dxbc::RootParameterType::DescriptorTable): {
+    case dxbc::RootParameterType::DescriptorTable: {
       const mcdxbc::DescriptorTable &Table =
           RSD.ParametersContainer.getDescriptorTable(Info.Location);
       for (const dxbc::RTS0::v2::DescriptorRange &Range : Table) {
diff --git a/llvm/lib/MC/DXContainerRootSignature.cpp b/llvm/lib/MC/DXContainerRootSignature.cpp
index 482280b5ef289..45ecd9a8d5e01 100644
--- a/llvm/lib/MC/DXContainerRootSignature.cpp
+++ b/llvm/lib/MC/DXContainerRootSignature.cpp
@@ -9,6 +9,7 @@
 #include "llvm/MC/DXContainerRootSignature.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/EndianStream.h"
+#include <cstdint>
 
 using namespace llvm;
 using namespace llvm::mcdxbc;
@@ -35,20 +36,20 @@ size_t RootSignatureDesc::getSize() const {
       StaticSamplers.size() * sizeof(dxbc::RTS0::v1::StaticSampler);
 
   for (const RootParameterInfo &I : ParametersContainer) {
-    switch (I.Header.ParameterType) {
-    case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
+    switch (I.Type) {
+    case dxbc::RootParameterType::Constants32Bit:
       Size += sizeof(dxbc::RTS0::v1::RootConstants);
       break;
-    case llvm::to_underlying(dxbc::RootParameterType::CBV):
-    case llvm::to_underlying(dxbc::RootParameterType::SRV):
-    case llvm::to_underlying(dxbc::RootParameterType::UAV):
+    case dxbc::RootParameterType::CBV:
+    case dxbc::RootParameterType::SRV:
+    case dxbc::RootParameterType::UAV:
       if (Version == 1)
         Size += sizeof(dxbc::RTS0::v1::RootDescriptor);
       else
         Size += sizeof(dxbc::RTS0::v2::RootDescriptor);
 
       break;
-    case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable):
+    case dxbc::RootParameterType::DescriptorTable:
       const DescriptorTable &Table =
           ParametersContainer.getDescriptorTable(I.Location);
 
@@ -84,11 +85,9 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
   support::endian::write(BOS, Flags, llvm::endianness::little);
 
   SmallVector<uint32_t> ParamsOffsets;
-  for (const RootParameterInfo &P : ParametersContainer) {
-    support::endian::write(BOS, P.Header.ParameterType,
-                           llvm::endianness::little);
-    support::endian::write(BOS, P.Header.ShaderVisibility,
-                           llvm::endianness::little);
+  for (const RootParameterInfo &I : ParametersContainer) {
+    support::endian::write(BOS, I.Type, llvm::endianness::little);
+    support::endian::write(BOS, I.Visibility, llvm::endianness::little);
 
     ParamsOffsets.push_back(writePlaceholder(BOS));
   }
@@ -96,9 +95,10 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
   assert(NumParameters == ParamsOffsets.size());
   for (size_t I = 0; I < NumParameters; ++I) {
     rewriteOffsetToCurrentByte(BOS, ParamsOffsets[I]);
-    const auto &[Type, Loc] = ParametersContainer.getTypeAndLocForParameter(I);
-    switch (Type) {
-    case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+    const auto Info = ParametersContainer.getInfo(I);
+    const uint32_t &Loc = Info.Location;
+    switch (Info.Type) {
+    case dxbc::RootParameterType::Constants32Bit: {
       const dxbc::RTS0::v1::RootConstants &Constants =
           ParametersContainer.getConstant(Loc);
       support::endian::write(BOS, Constants.ShaderRegister,
@@ -109,9 +109,9 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
                              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): {
+    case dxbc::RootParameterType::CBV:
+    case dxbc::RootParameterType::SRV:
+    case dxbc::RootParameterType::UAV: {
       const dxbc::RTS0::v2::RootDescriptor &Descriptor =
           ParametersContainer.getRootDescriptor(Loc);
 
@@ -123,7 +123,7 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
         support::endian::write(BOS, Descriptor.Flags, llvm::endianness::little);
       break;
     }
-    case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable): {
+    case dxbc::RootParameterType::DescriptorTable: {
       const DescriptorTable &Table =
           ParametersContainer.getDescriptorTable(Loc);
       support::endian::write(BOS, (uint32_t)Table.Ranges.size(),
diff --git a/llvm/lib/ObjectYAML/DXContainerEmitter.cpp b/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
index 043b575a43b11..b112c6f21ee5a 100644
--- a/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
+++ b/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
@@ -275,23 +275,30 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
 
       for (DXContainerYAML::RootParameterLocationYaml &L :
            P.RootSignature->Parameters.Locations) {
-        dxbc::RTS0::v1::RootParameterHeader Header{L.Header.Type, L.Header.Visibility,
-                                         L.Header.Offset};
 
-        switch (L.Header.Type) {
-        case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+        assert(dxbc::isValidParameterType(L.Header.Type) &&
+               "invalid DXContainer YAML");
+        assert(dxbc::isValidShaderVisibility(L.Header.Visibility) &&
+               "invalid DXContainer YAML");
+        dxbc::RootParameterType Type = dxbc::RootParameterType(L.Header.Type);
+        dxbc::ShaderVisibility Visibility =
+            dxbc::ShaderVisibility(L.Header.Visibility);
+
+        switch (Type) {
+        case dxbc::RootParameterType::Constants32Bit: {
           const DXContainerYAML::RootConstantsYaml &ConstantYaml =
               P.RootSignature->Parameters.getOrInsertConstants(L);
           dxbc::RTS0::v1::RootConstants Constants;
+
           Constants.Num32BitValues = ConstantYaml.Num32BitValues;
           Constants.RegisterSpace = ConstantYaml.RegisterSpace;
           Constants.ShaderRegister = ConstantYaml.ShaderRegister;
-          RS.ParametersContainer.addParameter(Header, Constants);
+          RS.ParametersContainer.addParameter(Type, Visibility, Constants);
           break;
         }
-        case llvm::to_underlying(dxbc::RootParameterType::CBV):
-        case llvm::to_underlying(dxbc::RootParameterType::SRV):
-        case llvm::to_underlying(dxbc::RootParameterType::UAV): {
+        case dxbc::RootParameterType::CBV:
+        case dxbc::RootParameterType::SRV:
+        case dxbc::RootParameterType::UAV: {
           const DXContainerYAML::RootDescriptorYaml &DescriptorYaml =
               P.RootSignature->Parameters.getOrInsertDescriptor(L);
 
@@ -300,10 +307,10 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
           Descriptor.ShaderRegister = DescriptorYaml.ShaderRegister;
           if (RS.Version > 1)
             Descriptor.Flags = DescriptorYaml.getEncodedFlags();
-          RS.ParametersContainer.addParameter(Header, Descriptor);
+          RS.ParametersContainer.addParameter(Type, Visibility, Descriptor);
           break;
         }
-        case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable): {
+        case dxbc::RootParameterType::DescriptorTable: {
           const DXContainerYAML::DescriptorTableYaml &TableYaml =
               P.RootSignature->Parameters.getOrInsertTable(L);
           mcdxbc::DescriptorTable Table;
@@ -320,14 +327,9 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
               Range.Flags = R.getEncodedFlags();
             Table.Ranges.push_back(Range);
           }
-          RS.ParametersContainer.addParameter(Header, Table);
+          RS.ParametersContainer.addParameter(Type, Visibility, Table);
           break;
         }
-        default:
-          // 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.addInvalidParameter(Header);
         }
       }
 
diff --git a/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp b/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
index be2c7d1ddff3f..fc0afb9a0efdf 100644
--- a/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
+++ b/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
@@ -164,12 +164,11 @@ static void validateRootSignature(Module &M,
 
   for (const mcdxbc::RootParameterInfo &ParamInfo : RSD.ParametersContainer) {
     dxbc::ShaderVisibility ParamVisibility =
-        static_cast<dxbc::ShaderVisibility>(ParamInfo.Header.ShaderVisibility);
+        dxbc::ShaderVisibility(ParamInfo.Visibility);
     if (ParamVisibility != dxbc::ShaderVisibility::All &&
         ParamVisibility != Visibility)
       continue;
-    dxbc::RootParameterType ParamType =
-        static_cast<dxbc::RootParameterType>(ParamInfo.Header.ParameterType);
+    dxbc::RootParameterType ParamType = dxbc::RootParameterType(ParamInfo.Type);
     switch (ParamType) {
     case dxbc::RootParameterType::Constants32Bit: {
       dxbc::RTS0::v1::RootConstants Const =
@@ -185,10 +184,9 @@ static void validateRootSignature(Module &M,
     case dxbc::RootParameterType::CBV: {
       dxbc::RTS0::v2::RootDescriptor Desc =
           RSD.ParametersContainer.getRootDescriptor(ParamInfo.Location);
-      Builder.trackBinding(toResourceClass(static_cast<dxbc::RootParameterType>(
-                               ParamInfo.Header.ParameterType)),
-                           Desc.RegisterSpace, Desc.ShaderRegister,
-                           Desc.ShaderRegister, &ParamInfo);
+      Builder.trackBinding(toResourceClass(ParamInfo.Type), Desc.RegisterSpace,
+                           Desc.ShaderRegister, Desc.ShaderRegister,
+                           &ParamInfo);
 
       break;
     }
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.cpp b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
index a4f5086c2f428..159cf7eda2482 100644
--- a/llvm/lib/Target/DirectX/DXILRootSignature.cpp
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
@@ -171,16 +171,16 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
       ...
[truncated]

bogner

Looks basically good. A few minor comments.

bogner · 2025-08-19T18:03:12Z

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

-  // The parameter offset doesn't matter here - we recalculate it during
-  // serialization  Header.ParameterOffset = 0;
-  Header.ParameterType = to_underlying(dxbc::RootParameterType::Constants32Bit);
+  mcdxbc::RootParameterHeader Header;


No longer used - please check your compiler warnings

bogner · 2025-08-19T18:03:23Z

llvm/include/llvm/MC/DXContainerRootSignature.h

+struct RootParameterHeader {
+  dxbc::RootParameterType ParameterType;
+  dxbc::ShaderVisibility ShaderVisibility;
+  uint32_t ParameterOffset;
+};


No longer used

bogner · 2025-08-19T18:05:31Z

llvm/include/llvm/MC/DXContainerRootSignature.h

-    ParametersInfo.push_back(RootParameterInfo(Header, Location));
+  void addInfo(dxbc::RootParameterType Type, dxbc::ShaderVisibility Visibility,
+               size_t Location) {
+    ParametersInfo.push_back(RootParameterInfo(Type, Visibility, Location));


Would save a copy to use emplace_back here:

Suggested change

ParametersInfo.push_back(RootParameterInfo(Type, Visibility, Location));

ParametersInfo.emplace_back(Type, Visibility, Location);

bogner · 2025-08-19T18:06:19Z

llvm/include/llvm/MC/DXContainerRootSignature.h

+  dxbc::ShaderVisibility Visibility;
  size_t Location;

  RootParameterInfo() = default;


Unrelated to the change, but is this actually used? I think the default constructor can be removed.

bogner · 2025-08-19T18:08:02Z

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

+  Expected<dxbc::ShaderVisibility> VisibilityOrErr =
+      extractShaderVisibility(RootConstantNode, 1);


Nitpick: I don't think the "OrErr" in this name is adding anything - that's usually used when we immediately store to something without the suffix (So we have Foo = *FooOrErr). Just Visibility is fine for all of these.

bogner · 2025-08-19T18:11:34Z

llvm/lib/MC/DXContainerRootSignature.cpp

 #include "llvm/MC/DXContainerRootSignature.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/EndianStream.h"
+#include <cstdint>


I don't see where this is used. Do we need it?

bogner · 2025-08-19T18:14:40Z

llvm/lib/MC/DXContainerRootSignature.cpp

-    switch (Type) {
-    case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+    const auto Info = ParametersContainer.getInfo(I);
+    const uint32_t &Loc = Info.Location;


const uint32_t & is a weird type, this could just be uint32_t. I'd probably just update the users of Loc to use Info.Location directly though - I think it's clearer with one less variable here.

bogner · 2025-08-21T17:21:19Z

llvm/lib/MC/DXContainerRootSignature.cpp

-    const auto &[Type, Loc] = ParametersContainer.getTypeAndLocForParameter(I);
-    switch (Type) {
-    case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+    const auto Info = ParametersContainer.getInfo(I);


We want a reference and not a copy here, so this needs at least const auto &. Better to just spell out the type explicitly though.

Suggested change

const auto Info = ParametersContainer.getInfo(I);

const RootParameterInfo &Info = ParametersContainer.getInfo(I);

bogner · 2025-08-21T17:24:08Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

+         << "  Shader Visibility: "
+         << enumToStringRef(Info.Visibility, dxbc::getShaderVisibility())
+         << "\n";
+      const uint32_t &Loc = Info.Location;


Looks like you missed this spot when you were making the changes for #154249 (comment)

bogner · 2025-08-21T17:24:33Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

-
-      switch (Type) {
-      case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+      const auto &Info = RS.ParametersContainer.getInfo(I);


Better to explicitly spell the type here.

inbelic

LGTM, just a note about the dependency

inbelic · 2025-08-21T23:44:05Z

llvm/include/llvm/MC/DXContainerRootSignature.h

IIUC, this pr is meant to remove any dependency from MC onto Object/DXContainer.h?

Shouldn't that mean that some include is removed?

It not from Object/DXContainer.h, it is from some values defined inside BinaryFormat/DXContainer.h mainly the RTS0 namespace

jurahul · 2025-08-26T15:54:54Z

Note that this change is resulting in a linker error:

ld.lld: error: undefined symbol: llvm::dxbc::getRootParameterTypes()
>>> referenced by DXILRootSignature.cpp
>>>               lib/Target/DirectX/CMakeFiles/LLVMDirectXCodeGen.dir/DXILRootSignature.cpp.o:(llvm::dxil::RootSignatureAnalysisPrinter::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&))

ld.lld: error: undefined symbol: llvm::dxbc::getShaderVisibility()
>>> referenced by DXILRootSignature.cpp
>>>               lib/Target/DirectX/CMakeFiles/LLVMDirectXCodeGen.dir/DXILRootSignature.cpp.o:(llvm::dxil::RootSignatureAnalysisPrinter::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&))
>>> referenced by DXILRootSignature.cpp
>>>               lib/Target/DirectX/CMakeFiles/LLVMDirectXCodeGen.dir/DXILRootSignature.cpp.o:(llvm::dxil::RootSignatureAnalysisPrinter::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&))

Fix seems:

--- a/llvm/lib/Target/DirectX/CMakeLists.txt
+++ b/llvm/lib/Target/DirectX/CMakeLists.txt
@@ -41,6 +41,7 @@ add_llvm_target(DirectXCodeGen
   LINK_COMPONENTS
   Analysis
   AsmPrinter
+  BinaryFormat
   CodeGen
   CodeGenTypes
   Core

jurahul · 2025-08-26T15:55:29Z

@joaosaffran @bogner FYI

jurahul · 2025-08-26T16:00:39Z

#155441

bogner · 2025-08-26T17:46:06Z

@jurahul Yeah that looks like the right fix - looks like we changed a header-only dependency to a real dependency without noticing we didn't have the dependency in the CMakeLists already.

Add `BinaryFormat` to `LINK_COMPONENTS` to fix the following linker error: ``` ld.lld: error: undefined symbol: llvm::dxbc::getRootParameterTypes() >>> referenced by DXILRootSignature.cpp >>> lib/Target/DirectX/CMakeFiles/LLVMDirectXCodeGen.dir/DXILRootSignature.cpp.o:(llvm::dxil::RootSignatureAnalysisPrinter::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&)) ld.lld: error: undefined symbol: llvm::dxbc::getShaderVisibility() >>> referenced by DXILRootSignature.cpp >>> lib/Target/DirectX/CMakeFiles/LLVMDirectXCodeGen.dir/DXILRootSignature.cpp.o:(llvm::dxil::RootSignatureAnalysisPrinter::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&)) >>> referenced by DXILRootSignature.cpp >>> lib/Target/DirectX/CMakeFiles/LLVMDirectXCodeGen.dir/DXILRootSignature.cpp.o:(llvm::dxil::RootSignatureAnalysisPrinter::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&)) ``` Root cause: #154249 changed a header-only dependency to a real dependency without noticing that the dependency was missing in CMakeLists.txt

Add `BinaryFormat` to `LINK_COMPONENTS` to fix the following linker error: ``` ld.lld: error: undefined symbol: llvm::dxbc::getRootParameterTypes() >>> referenced by DXILRootSignature.cpp >>> lib/Target/DirectX/CMakeFiles/LLVMDirectXCodeGen.dir/DXILRootSignature.cpp.o:(llvm::dxil::RootSignatureAnalysisPrinter::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&)) ld.lld: error: undefined symbol: llvm::dxbc::getShaderVisibility() >>> referenced by DXILRootSignature.cpp >>> lib/Target/DirectX/CMakeFiles/LLVMDirectXCodeGen.dir/DXILRootSignature.cpp.o:(llvm::dxil::RootSignatureAnalysisPrinter::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&)) >>> referenced by DXILRootSignature.cpp >>> lib/Target/DirectX/CMakeFiles/LLVMDirectXCodeGen.dir/DXILRootSignature.cpp.o:(llvm::dxil::RootSignatureAnalysisPrinter::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&)) ``` Root cause: llvm/llvm-project#154249 changed a header-only dependency to a real dependency without noticing that the dependency was missing in CMakeLists.txt

joaosaffran added 2 commits August 18, 2025 18:49

making parameter type and shader visibility use enums

31ec5e5

clean up

1690a9c

bogner reviewed Aug 19, 2025

View reviewed changes

joaosaffran added 3 commits August 19, 2025 10:34

removing root parameter header from MC

f6f2e61

clean up

6539364

fix whitespace in test

1d29111

joaosaffran requested a review from bogner August 19, 2025 17:47

joaosaffran changed the title ~~[Draft] Remove to_underlying from root parameter header~~ [DirectX] Refactor RootSignature Backend to remove to_underlying from Root Parameter Header Aug 19, 2025

joaosaffran requested review from inbelic and llvm-beanz August 19, 2025 17:54

joaosaffran marked this pull request as ready for review August 19, 2025 17:55

llvmbot added llvm:mc Machine (object) code backend:DirectX HLSL HLSL Language Support objectyaml labels Aug 19, 2025

adding missing import

8eb82fd

bogner reviewed Aug 19, 2025

View reviewed changes

joaosaffran added 6 commits August 19, 2025 16:33

remove unused

d38c00d

save a copy

3b25b34

remove default constructor

567a3d4

rename visibility

fb248da

remove cstdint

dc436d5

remove Loc

8353fe0

joaosaffran requested a review from bogner August 19, 2025 23:39

joaosaffran linked an issue Aug 20, 2025 that may be closed by this pull request

[DirectX] Clean up to_underlying in case labels #150676

Closed

bogner reviewed Aug 21, 2025

View reviewed changes

inbelic reviewed Aug 21, 2025

View reviewed changes

inbelic mentioned this pull request Aug 21, 2025

[DirectX] Removing dxbc RootSignature and RootDescriptor from mcbxdc #154585

Merged

addressing comments from bogner

3a30581

joaosaffran requested review from bogner and inbelic August 25, 2025 17:51

bogner approved these changes Aug 25, 2025

View reviewed changes

joaosaffran merged commit c6dfbc5 into llvm:main Aug 25, 2025
10 checks passed

jurahul mentioned this pull request Aug 26, 2025

[NFC][DirectX] Fix build failure #155441

Merged

aemerson mentioned this pull request Aug 30, 2025

[NFC] [clangd] [Modules] remove dot in log #156207

Closed

	return static_cast<dxbc::ShaderVisibility>(*Val);
	return dxbc::ShaderVisibility(*Val);

	ParametersInfo.push_back(RootParameterInfo(Type, Visibility, Location));
	ParametersInfo.emplace_back(Type, Visibility, Location);

		Expected<dxbc::ShaderVisibility> VisibilityOrErr =
		extractShaderVisibility(RootConstantNode, 1);

	const auto Info = ParametersContainer.getInfo(I);
	const RootParameterInfo &Info = ParametersContainer.getInfo(I);

[DirectX] Refactor RootSignature Backend to remove to_underlying from Root Parameter Header #154249

[DirectX] Refactor RootSignature Backend to remove to_underlying from Root Parameter Header #154249

Uh oh!

Conversation

joaosaffran commented Aug 19, 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!

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!

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

Uh oh!

Uh oh!

bogner 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!

inbelic 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!

Uh oh!

jurahul commented Aug 26, 2025

Uh oh!

jurahul commented Aug 26, 2025

Uh oh!

jurahul commented Aug 26, 2025

Uh oh!

bogner commented Aug 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[DirectX] Refactor RootSignature Backend to remove `to_underlying` from Root Parameter Header #154249

[DirectX] Refactor RootSignature Backend to remove `to_underlying` from Root Parameter Header #154249

joaosaffran commented Aug 19, 2025 •

edited

Loading

llvmbot commented Aug 19, 2025 •

edited

Loading