[DirectX][NFC] Refactoring DirectX backend to not use llvm::to_underlying in switch cases.
#151032
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
llvm:mc
|
@llvm/pr-subscribers-backend-directx @llvm/pr-subscribers-hlsl Author: None (joaosaffran) ChangesAs request in #150676, DirectX has some switch statements that use to_underlying to handle the cases, this prevents compiler warnings when the statements are not fully covered. Closes: #150676 Full diff: https://github.com/llvm/llvm-project/pull/151032.diff 6 Files Affected:
diff --git a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
index 53f59349ae029..742b2c55bcce9 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
@@ -12,6 +12,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/Frontend/HLSL/RootSignatureMetadata.h"
+#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/Frontend/HLSL/RootSignatureValidations.h"
#include "llvm/IR/DiagnosticInfo.h"
#include "llvm/IR/IRBuilder.h"
@@ -559,11 +560,17 @@ bool MetadataParser::validateRootSignature(
assert(dxbc::isValidParameterType(Info.Header.ParameterType) &&
"Invalid value for ParameterType");
- switch (Info.Header.ParameterType) {
+ dxbc::RootParameterType PT =
+ static_cast<dxbc::RootParameterType>(Info.Header.ParameterType);
- case llvm::to_underlying(dxbc::RootParameterType::CBV):
- case llvm::to_underlying(dxbc::RootParameterType::UAV):
- case llvm::to_underlying(dxbc::RootParameterType::SRV): {
+ switch (PT) {
+ case dxbc::RootParameterType::Constants32Bit:
+ // ToDo: Add proper validation.
+ continue;
+
+ case dxbc::RootParameterType::CBV:
+ case dxbc::RootParameterType::UAV:
+ case dxbc::RootParameterType::SRV: {
const dxbc::RTS0::v2::RootDescriptor &Descriptor =
RSD.ParametersContainer.getRootDescriptor(Info.Location);
if (!llvm::hlsl::rootsig::verifyRegisterValue(Descriptor.ShaderRegister))
@@ -580,7 +587,7 @@ bool MetadataParser::validateRootSignature(
}
break;
}
- case llvm::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/Frontend/HLSL/RootSignatureValidations.cpp b/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
index f11c7d2033bfb..e55d3ec15c19d 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
@@ -53,10 +53,9 @@ bool verifyRootDescriptorFlag(uint32_t Version, uint32_t FlagsVal) {
bool verifyRangeType(uint32_t Type) {
switch (Type) {
- case llvm::to_underlying(dxbc::DescriptorRangeType::CBV):
- case llvm::to_underlying(dxbc::DescriptorRangeType::SRV):
- case llvm::to_underlying(dxbc::DescriptorRangeType::UAV):
- case llvm::to_underlying(dxbc::DescriptorRangeType::Sampler):
+#define DESCRIPTOR_RANGE(Num, Val) \
+ case llvm::to_underlying(dxbc::DescriptorRangeType::Val):
+#include "llvm/BinaryFormat/DXContainerConstants.def"
return true;
};
diff --git a/llvm/lib/MC/DXContainerRootSignature.cpp b/llvm/lib/MC/DXContainerRootSignature.cpp
index 482280b5ef289..c94a39f80eeb2 100644
--- a/llvm/lib/MC/DXContainerRootSignature.cpp
+++ b/llvm/lib/MC/DXContainerRootSignature.cpp
@@ -8,6 +8,7 @@
#include "llvm/MC/DXContainerRootSignature.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/Support/EndianStream.h"
using namespace llvm;
@@ -35,20 +36,26 @@ 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):
+ if (!dxbc::isValidParameterType(I.Header.ParameterType))
+ continue;
+
+ dxbc::RootParameterType PT =
+ static_cast<dxbc::RootParameterType>(I.Header.ParameterType);
+
+ switch (PT) {
+ 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);
@@ -97,8 +104,12 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
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): {
+ if (!dxbc::isValidParameterType(Type))
+ continue;
+ dxbc::RootParameterType PT = static_cast<dxbc::RootParameterType>(Type);
+
+ switch (PT) {
+ case dxbc::RootParameterType::Constants32Bit: {
const dxbc::RTS0::v1::RootConstants &Constants =
ParametersContainer.getConstant(Loc);
support::endian::write(BOS, Constants.ShaderRegister,
@@ -109,9 +120,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 +134,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..35e1c3e3b5953 100644
--- a/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
+++ b/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
@@ -278,8 +278,19 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
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): {
+ if (!dxbc::isValidParameterType(L.Header.Type)) {
+ // 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);
+ continue;
+ }
+
+ dxbc::RootParameterType ParameterType =
+ static_cast<dxbc::RootParameterType>(L.Header.Type);
+
+ switch (ParameterType) {
+ case dxbc::RootParameterType::Constants32Bit: {
const DXContainerYAML::RootConstantsYaml &ConstantYaml =
P.RootSignature->Parameters.getOrInsertConstants(L);
dxbc::RTS0::v1::RootConstants Constants;
@@ -289,9 +300,9 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
RS.ParametersContainer.addParameter(Header, 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);
@@ -303,7 +314,7 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
RS.ParametersContainer.addParameter(Header, 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;
@@ -323,11 +334,6 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
RS.ParametersContainer.addParameter(Header, 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/ObjectYAML/DXContainerYAML.cpp b/llvm/lib/ObjectYAML/DXContainerYAML.cpp
index 263f7bdf37bca..03d5725c21491 100644
--- a/llvm/lib/ObjectYAML/DXContainerYAML.cpp
+++ b/llvm/lib/ObjectYAML/DXContainerYAML.cpp
@@ -424,22 +424,28 @@ void MappingContextTraits<DXContainerYAML::RootParameterLocationYaml,
IO.mapRequired("ParameterType", L.Header.Type);
IO.mapRequired("ShaderVisibility", L.Header.Visibility);
- switch (L.Header.Type) {
- case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+ if (!dxbc::isValidParameterType(L.Header.Type))
+ return;
+ dxbc::RootParameterType PT =
+ static_cast<dxbc::RootParameterType>(L.Header.Type);
+
+ // We allow ParameterType to be invalid here.
+ switch (PT) {
+ case dxbc::RootParameterType::Constants32Bit: {
DXContainerYAML::RootConstantsYaml &Constants =
S.Parameters.getOrInsertConstants(L);
IO.mapRequired("Constants", 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: {
DXContainerYAML::RootDescriptorYaml &Descriptor =
S.Parameters.getOrInsertDescriptor(L);
IO.mapRequired("Descriptor", Descriptor);
break;
}
- case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable): {
+ case dxbc::RootParameterType::DescriptorTable: {
DXContainerYAML::DescriptorTableYaml &Table =
S.Parameters.getOrInsertTable(L);
IO.mapRequired("Table", Table);
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.cpp b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
index ebdfcaa566b51..04c7c77953a5b 100644
--- a/llvm/lib/Target/DirectX/DXILRootSignature.cpp
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
@@ -175,8 +175,10 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
OS << "- Parameter Type: " << Type << "\n"
<< " Shader Visibility: " << Header.ShaderVisibility << "\n";
- switch (Type) {
- case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+ assert(dxbc::isValidParameterType(Type) && "Invalid Parameter Type");
+ dxbc::RootParameterType PT = static_cast<dxbc::RootParameterType>(Type);
+ switch (PT) {
+ case dxbc::RootParameterType::Constants32Bit: {
const dxbc::RTS0::v1::RootConstants &Constants =
RS.ParametersContainer.getConstant(Loc);
OS << " Register Space: " << Constants.RegisterSpace << "\n"
@@ -184,9 +186,9 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
<< " Num 32 Bit Values: " << Constants.Num32BitValues << "\n";
break;
}
- case llvm::to_underlying(dxbc::RootParameterType::CBV):
- case llvm::to_underlying(dxbc::RootParameterType::UAV):
- case llvm::to_underlying(dxbc::RootParameterType::SRV): {
+ case dxbc::RootParameterType::CBV:
+ case dxbc::RootParameterType::UAV:
+ case dxbc::RootParameterType::SRV: {
const dxbc::RTS0::v2::RootDescriptor &Descriptor =
RS.ParametersContainer.getRootDescriptor(Loc);
OS << " Register Space: " << Descriptor.RegisterSpace << "\n"
@@ -195,7 +197,7 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
OS << " Flags: " << Descriptor.Flags << "\n";
break;
}
- case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable): {
+ case dxbc::RootParameterType::DescriptorTable: {
const mcdxbc::DescriptorTable &Table =
RS.ParametersContainer.getDescriptorTable(Loc);
OS << " NumRanges: " << Table.Ranges.size() << "\n";
|
|
@llvm/pr-subscribers-objectyaml Author: None (joaosaffran) ChangesAs request in #150676, DirectX has some switch statements that use to_underlying to handle the cases, this prevents compiler warnings when the statements are not fully covered. Closes: #150676 Full diff: https://github.com/llvm/llvm-project/pull/151032.diff 6 Files Affected:
diff --git a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
index 53f59349ae029..742b2c55bcce9 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
@@ -12,6 +12,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/Frontend/HLSL/RootSignatureMetadata.h"
+#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/Frontend/HLSL/RootSignatureValidations.h"
#include "llvm/IR/DiagnosticInfo.h"
#include "llvm/IR/IRBuilder.h"
@@ -559,11 +560,17 @@ bool MetadataParser::validateRootSignature(
assert(dxbc::isValidParameterType(Info.Header.ParameterType) &&
"Invalid value for ParameterType");
- switch (Info.Header.ParameterType) {
+ dxbc::RootParameterType PT =
+ static_cast<dxbc::RootParameterType>(Info.Header.ParameterType);
- case llvm::to_underlying(dxbc::RootParameterType::CBV):
- case llvm::to_underlying(dxbc::RootParameterType::UAV):
- case llvm::to_underlying(dxbc::RootParameterType::SRV): {
+ switch (PT) {
+ case dxbc::RootParameterType::Constants32Bit:
+ // ToDo: Add proper validation.
+ continue;
+
+ case dxbc::RootParameterType::CBV:
+ case dxbc::RootParameterType::UAV:
+ case dxbc::RootParameterType::SRV: {
const dxbc::RTS0::v2::RootDescriptor &Descriptor =
RSD.ParametersContainer.getRootDescriptor(Info.Location);
if (!llvm::hlsl::rootsig::verifyRegisterValue(Descriptor.ShaderRegister))
@@ -580,7 +587,7 @@ bool MetadataParser::validateRootSignature(
}
break;
}
- case llvm::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/Frontend/HLSL/RootSignatureValidations.cpp b/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
index f11c7d2033bfb..e55d3ec15c19d 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
@@ -53,10 +53,9 @@ bool verifyRootDescriptorFlag(uint32_t Version, uint32_t FlagsVal) {
bool verifyRangeType(uint32_t Type) {
switch (Type) {
- case llvm::to_underlying(dxbc::DescriptorRangeType::CBV):
- case llvm::to_underlying(dxbc::DescriptorRangeType::SRV):
- case llvm::to_underlying(dxbc::DescriptorRangeType::UAV):
- case llvm::to_underlying(dxbc::DescriptorRangeType::Sampler):
+#define DESCRIPTOR_RANGE(Num, Val) \
+ case llvm::to_underlying(dxbc::DescriptorRangeType::Val):
+#include "llvm/BinaryFormat/DXContainerConstants.def"
return true;
};
diff --git a/llvm/lib/MC/DXContainerRootSignature.cpp b/llvm/lib/MC/DXContainerRootSignature.cpp
index 482280b5ef289..c94a39f80eeb2 100644
--- a/llvm/lib/MC/DXContainerRootSignature.cpp
+++ b/llvm/lib/MC/DXContainerRootSignature.cpp
@@ -8,6 +8,7 @@
#include "llvm/MC/DXContainerRootSignature.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/Support/EndianStream.h"
using namespace llvm;
@@ -35,20 +36,26 @@ 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):
+ if (!dxbc::isValidParameterType(I.Header.ParameterType))
+ continue;
+
+ dxbc::RootParameterType PT =
+ static_cast<dxbc::RootParameterType>(I.Header.ParameterType);
+
+ switch (PT) {
+ 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);
@@ -97,8 +104,12 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
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): {
+ if (!dxbc::isValidParameterType(Type))
+ continue;
+ dxbc::RootParameterType PT = static_cast<dxbc::RootParameterType>(Type);
+
+ switch (PT) {
+ case dxbc::RootParameterType::Constants32Bit: {
const dxbc::RTS0::v1::RootConstants &Constants =
ParametersContainer.getConstant(Loc);
support::endian::write(BOS, Constants.ShaderRegister,
@@ -109,9 +120,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 +134,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..35e1c3e3b5953 100644
--- a/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
+++ b/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
@@ -278,8 +278,19 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
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): {
+ if (!dxbc::isValidParameterType(L.Header.Type)) {
+ // 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);
+ continue;
+ }
+
+ dxbc::RootParameterType ParameterType =
+ static_cast<dxbc::RootParameterType>(L.Header.Type);
+
+ switch (ParameterType) {
+ case dxbc::RootParameterType::Constants32Bit: {
const DXContainerYAML::RootConstantsYaml &ConstantYaml =
P.RootSignature->Parameters.getOrInsertConstants(L);
dxbc::RTS0::v1::RootConstants Constants;
@@ -289,9 +300,9 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
RS.ParametersContainer.addParameter(Header, 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);
@@ -303,7 +314,7 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
RS.ParametersContainer.addParameter(Header, 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;
@@ -323,11 +334,6 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
RS.ParametersContainer.addParameter(Header, 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/ObjectYAML/DXContainerYAML.cpp b/llvm/lib/ObjectYAML/DXContainerYAML.cpp
index 263f7bdf37bca..03d5725c21491 100644
--- a/llvm/lib/ObjectYAML/DXContainerYAML.cpp
+++ b/llvm/lib/ObjectYAML/DXContainerYAML.cpp
@@ -424,22 +424,28 @@ void MappingContextTraits<DXContainerYAML::RootParameterLocationYaml,
IO.mapRequired("ParameterType", L.Header.Type);
IO.mapRequired("ShaderVisibility", L.Header.Visibility);
- switch (L.Header.Type) {
- case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+ if (!dxbc::isValidParameterType(L.Header.Type))
+ return;
+ dxbc::RootParameterType PT =
+ static_cast<dxbc::RootParameterType>(L.Header.Type);
+
+ // We allow ParameterType to be invalid here.
+ switch (PT) {
+ case dxbc::RootParameterType::Constants32Bit: {
DXContainerYAML::RootConstantsYaml &Constants =
S.Parameters.getOrInsertConstants(L);
IO.mapRequired("Constants", 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: {
DXContainerYAML::RootDescriptorYaml &Descriptor =
S.Parameters.getOrInsertDescriptor(L);
IO.mapRequired("Descriptor", Descriptor);
break;
}
- case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable): {
+ case dxbc::RootParameterType::DescriptorTable: {
DXContainerYAML::DescriptorTableYaml &Table =
S.Parameters.getOrInsertTable(L);
IO.mapRequired("Table", Table);
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.cpp b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
index ebdfcaa566b51..04c7c77953a5b 100644
--- a/llvm/lib/Target/DirectX/DXILRootSignature.cpp
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
@@ -175,8 +175,10 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
OS << "- Parameter Type: " << Type << "\n"
<< " Shader Visibility: " << Header.ShaderVisibility << "\n";
- switch (Type) {
- case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
+ assert(dxbc::isValidParameterType(Type) && "Invalid Parameter Type");
+ dxbc::RootParameterType PT = static_cast<dxbc::RootParameterType>(Type);
+ switch (PT) {
+ case dxbc::RootParameterType::Constants32Bit: {
const dxbc::RTS0::v1::RootConstants &Constants =
RS.ParametersContainer.getConstant(Loc);
OS << " Register Space: " << Constants.RegisterSpace << "\n"
@@ -184,9 +186,9 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
<< " Num 32 Bit Values: " << Constants.Num32BitValues << "\n";
break;
}
- case llvm::to_underlying(dxbc::RootParameterType::CBV):
- case llvm::to_underlying(dxbc::RootParameterType::UAV):
- case llvm::to_underlying(dxbc::RootParameterType::SRV): {
+ case dxbc::RootParameterType::CBV:
+ case dxbc::RootParameterType::UAV:
+ case dxbc::RootParameterType::SRV: {
const dxbc::RTS0::v2::RootDescriptor &Descriptor =
RS.ParametersContainer.getRootDescriptor(Loc);
OS << " Register Space: " << Descriptor.RegisterSpace << "\n"
@@ -195,7 +197,7 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
OS << " Flags: " << Descriptor.Flags << "\n";
break;
}
- case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable): {
+ case dxbc::RootParameterType::DescriptorTable: {
const mcdxbc::DescriptorTable &Table =
RS.ParametersContainer.getDescriptorTable(Loc);
OS << " NumRanges: " << Table.Ranges.size() << "\n";
|
|
|
inbelic
reviewed
Jul 28, 2025
Comment on lines
56
to
58
| case llvm::to_underlying(dxbc::DescriptorRangeType::CBV): | ||
| case llvm::to_underlying(dxbc::DescriptorRangeType::SRV): | ||
| case llvm::to_underlying(dxbc::DescriptorRangeType::UAV): | ||
| case llvm::to_underlying(dxbc::DescriptorRangeType::Sampler): | ||
| #define DESCRIPTOR_RANGE(Num, Val) \ | ||
| case llvm::to_underlying(dxbc::DescriptorRangeType::Val): | ||
| #include "llvm/BinaryFormat/DXContainerConstants.def" |
There was a problem hiding this comment.
This is not updating to not use to_underlying
There was a problem hiding this comment.
This specific case is necessary, to keep using llvm::to_underlying, since that is checking if an uint_32t is valid value for RootParametersType. However, I updated to use the tablegen definition, that way we will always have it covering all possible values.
Or we can change how the check is being done, if folks prefer.
There was a problem hiding this comment.
Seems to me like we get the same end result by doing the int->enum cast once before the switch and having the switch statement operate on the actual enumerations. I have a strong preference for not using to_underlying since it impairs the frontend's ability to generate warnings.
There was a problem hiding this comment.
In this point, we are not sure Type is a valid DescriptorRangeType so the casting here int->enum would cause undefined behaviour.
llvm-beanz
reviewed
Jul 29, 2025
| dxbc::RootParameterType PT = | ||
| static_cast<dxbc::RootParameterType>(L.Header.Type); | ||
|
|
||
| // We allow ParameterType to be invalid here. |
There was a problem hiding this comment.
This comment doesn't match the implementation. It returns above if the parameter type is invalid.
There was a problem hiding this comment.
Changed the comments to make it clearer what the current behaviour is.
bogner
requested changes
Aug 15, 2025
There was a problem hiding this comment.
As is, this mostly just shuffles the problem around. All of this would be much simpler if we just use the correct enum types in the data structures.
So, in DXContainerRootSignature.h:
struct RootParameterInfo {
dxbc::RootParameterType Type;
dxbc::ShaderVisibility Visibility;
uint32_t Offset;
size_t Location;
RootParameterInfo() = default;
RootParameterInfo(dxbc::RootParameterType Type,
dxbc::ShaderVisibility Visibility, uint32_t Offset,
size_t Location)
: Type(Type), Visibility(Visibility), Offset(Offset), Location(Location) {
}
};and similarly in DXContainerYAML.h:
struct RootParameterHeaderYaml {
dxbc::RootParameterType Type;
dxbc::ShaderVisibility Visibility;
uint32_t Offset;
};If you make those changes, then the rest of this should fall nicely into place.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
inbelic
reviewed
Aug 18, 2025
| char InvalidRSMetadataFormat::ID; | ||
| char InvalidRSMetadataValue::ID; | ||
| template <typename T> char RootSignatureValidationError<T>::ID; | ||
| template <typename T, typename ET> char RootSignatureValidationError<T, ET>::ID; |
There was a problem hiding this comment.
ET seems to be uint32_t in all the used cases, what is the purpose of this? Couldn't we just have the static_cast above use uint32_t?
There was a problem hiding this comment.
We don't want to always cast to uint32_t, only when dealing with enums. Otherwise, I want to preserve the original type.
joaosaffran
commented
Aug 18, 2025
| NumStaticSamplers: 0 | ||
| StaticSamplersOffset: 60 | ||
| Parameters: | ||
| - ParameterType: 2 # SRV |
There was a problem hiding this comment.
comment is wrong
joaosaffran
commented
Aug 18, 2025
There was a problem hiding this comment.
This test is impossible using Enums in yaml.
joaosaffran
commented
Aug 18, 2025
There was a problem hiding this comment.
This test is impossible using Enums in yaml.
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x52, 0x54, 0x53, 0x30, 0x4c, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, | ||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, | ||
| 0x18, 0x00, 0x00, 0x00, 0x11, 0x00, 0x00, 0x00, 0x0a, 0x00, 0x00, 0x00, |
There was a problem hiding this comment.
This was wrong originally.
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x52, 0x54, 0x53, 0x30, 0x4c, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, | ||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, | ||
| 0x18, 0x00, 0x00, 0x00, 0x11, 0x00, 0x00, 0x00, 0x0a, 0x00, 0x00, 0x00, |
There was a problem hiding this comment.
This was wrong originally
llvm-beanz
reviewed
Aug 18, 2025
| LLVM_ABI ArrayRef<EnumEntry<StaticBorderColor>> getStaticBorderColors(); | ||
|
|
||
| // D3D_ROOT_SIGNATURE_VERSION | ||
| enum class RootSignatureVersion { |
There was a problem hiding this comment.
Suggested change
| enum class RootSignatureVersion { | |
| enum class RootSignatureVersion : unit32_t { |
|
|
||
| void log(raw_ostream &OS) const override { | ||
| OS << "Invalid value for " << ParamName << ": " << Value; | ||
| OS << "Invalid value for " << ParamName << ": " << static_cast<ET>(Value); |
There was a problem hiding this comment.
We can probably eliminate the ET parameter type by doing something like this:
Suggested change
| OS << "Invalid value for " << ParamName << ": " << static_cast<ET>(Value); | |
| OS << "Invalid value for " << ParamName << ": "; | |
| if constexpr (std::is_enum<T>) | |
| OS << llvm::to_underlying(Value); | |
| else | |
| OS << Value; |
| return make_error<InvalidRSMetadataFormat>("Root Descriptor Element"); | ||
|
|
||
| dxbc::RTS0::v1::RootParameterHeader Header; | ||
| switch (ElementKind) { |
There was a problem hiding this comment.
This switch statement's pattern is going to grow to be a problem. One of the benefits of enums is that they give warnings for missed values, but they'll also complain if you cover all cases and have a default.
We should probably adopt a pattern where we have int->enum conversion functions that handle all cases that the enum is valid for and returns an optional which can be unset on an invalid value.
| const uint32_t VersionEnum = Version - 1; | ||
| const llvm::dxbc::RootSignatureVersion Version = | ||
| SemaRef.getLangOpts().HLSLRootSigVer; | ||
| const uint32_t VersionEnum = static_cast<uint32_t>(Version) - 1; |
There was a problem hiding this comment.
This seems like a hack used to print the minor version number. We should instead have a better enum->string conversion that can produce the correct version number.
| } | ||
|
|
||
| static const EnumEntry<RootSignatureVersion> RootSignatureVersionsValues[] = { | ||
| {"V1_0", RootSignatureVersion::V1_0}, |
There was a problem hiding this comment.
It might be nicer if these were V1.0. I'm not sure we're required to use _ in the places these strings appear.
| .Case("UAV", to_underlying(dxbc::DescriptorRangeType::UAV)) | ||
| .Case("Sampler", to_underlying(dxbc::DescriptorRangeType::Sampler)) | ||
| .Default(~0U); | ||
| Range.RangeType = StringSwitch<dxbc::DescriptorRangeType>(*ElementText) |
There was a problem hiding this comment.
Do we have a utility function that does this? It seems like this is something that should be a utility.
|
Chatted with @joaosaffran offline and the new plan is to break this up a little differently:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As request in #150676, DirectX has some switch statements that use to_underlying to handle the cases, this prevents compiler warnings when the statements are not fully covered.
Closes: #150676