-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[DirectX] Removing dxbc DescriptorRange from mcbxdc #154629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DirectX] Removing dxbc DescriptorRange from mcbxdc #154629
Conversation
This reverts commit 8c143ba.
@llvm/pr-subscribers-llvm-binary-utilities @llvm/pr-subscribers-backend-directx Author: None (joaosaffran) ChangesMC Descriptor Range Representation currently depend on Object structures. This PR removes that dependency and in order to facilitate removing to_underlying usage in follow-up PRs. Full diff: https://github.com/llvm/llvm-project/pull/154629.diff 12 Files Affected:
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index 47a4874721db8..85dc5eb2b42f8 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -1268,11 +1268,34 @@ bool SemaHLSL::handleRootSignatureElements(
// value
ReportError(Loc, 1, 0xfffffffe);
}
+ switch (Clause->Type) {
- if (!llvm::hlsl::rootsig::verifyDescriptorRangeFlag(
- Version, llvm::to_underlying(Clause->Type),
- llvm::to_underlying(Clause->Flags)))
- ReportFlagError(Loc);
+ case llvm::dxil::ResourceClass::SRV:
+ if (!llvm::hlsl::rootsig::verifyDescriptorRangeFlag(
+ Version, llvm::dxbc::DescriptorRangeType::SRV,
+ llvm::to_underlying(Clause->Flags)))
+ ReportFlagError(Loc);
+ break;
+ case llvm::dxil::ResourceClass::UAV:
+ if (!llvm::hlsl::rootsig::verifyDescriptorRangeFlag(
+ Version, llvm::dxbc::DescriptorRangeType::UAV,
+ llvm::to_underlying(Clause->Flags)))
+ ReportFlagError(Loc);
+ break;
+ case llvm::dxil::ResourceClass::CBuffer:
+ if (!llvm::hlsl::rootsig::verifyDescriptorRangeFlag(
+ Version, llvm::dxbc::DescriptorRangeType::CBV,
+ llvm::to_underlying(Clause->Flags)))
+ ReportFlagError(Loc);
+ break;
+ case llvm::dxil::ResourceClass::Sampler:
+ if (!llvm::hlsl::rootsig::verifyDescriptorRangeFlag(
+ Version, llvm::dxbc::DescriptorRangeType::Sampler,
+ llvm::to_underlying(Clause->Flags)))
+ ReportFlagError(Loc);
+ break;
+ break;
+ }
}
}
diff --git a/llvm/include/llvm/BinaryFormat/DXContainer.h b/llvm/include/llvm/BinaryFormat/DXContainer.h
index f74c9775cb3f3..54747b46ccc6c 100644
--- a/llvm/include/llvm/BinaryFormat/DXContainer.h
+++ b/llvm/include/llvm/BinaryFormat/DXContainer.h
@@ -209,6 +209,16 @@ inline bool isValidParameterType(uint32_t V) {
return false;
}
+#define DESCRIPTOR_RANGE(Val, Enum) \
+ case Val: \
+ return true;
+inline bool isValidRangeType(uint32_t V) {
+ switch (V) {
+#include "DXContainerConstants.def"
+ }
+ return false;
+}
+
#define SHADER_VISIBILITY(Val, Enum) Enum = Val,
enum class ShaderVisibility : uint32_t {
#include "DXContainerConstants.def"
diff --git a/llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h b/llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h
index fde32a1fff591..acb83e54b92ed 100644
--- a/llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h
+++ b/llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h
@@ -30,7 +30,8 @@ LLVM_ABI bool verifyRegisterValue(uint32_t RegisterValue);
LLVM_ABI bool verifyRegisterSpace(uint32_t RegisterSpace);
LLVM_ABI bool verifyRootDescriptorFlag(uint32_t Version, uint32_t FlagsVal);
LLVM_ABI bool verifyRangeType(uint32_t Type);
-LLVM_ABI bool verifyDescriptorRangeFlag(uint32_t Version, uint32_t Type,
+LLVM_ABI bool verifyDescriptorRangeFlag(uint32_t Version,
+ dxbc::DescriptorRangeType Type,
uint32_t FlagsVal);
LLVM_ABI bool verifyNumDescriptors(uint32_t NumDescriptors);
LLVM_ABI bool verifySamplerFilter(uint32_t Value);
diff --git a/llvm/include/llvm/MC/DXContainerRootSignature.h b/llvm/include/llvm/MC/DXContainerRootSignature.h
index 85b45323cee08..28664434f933c 100644
--- a/llvm/include/llvm/MC/DXContainerRootSignature.h
+++ b/llvm/include/llvm/MC/DXContainerRootSignature.h
@@ -31,6 +31,15 @@ struct RootDescriptor {
uint32_t Flags;
};
+struct DescriptorRange {
+ dxbc::DescriptorRangeType RangeType;
+ uint32_t NumDescriptors;
+ uint32_t BaseShaderRegister;
+ uint32_t RegisterSpace;
+ uint32_t Flags;
+ uint32_t OffsetInDescriptorsFromTableStart;
+};
+
struct RootParameterInfo {
dxbc::RootParameterType Type;
dxbc::ShaderVisibility Visibility;
@@ -42,11 +51,11 @@ struct RootParameterInfo {
};
struct DescriptorTable {
- SmallVector<dxbc::RTS0::v2::DescriptorRange> Ranges;
- SmallVector<dxbc::RTS0::v2::DescriptorRange>::const_iterator begin() const {
+ SmallVector<DescriptorRange> Ranges;
+ SmallVector<DescriptorRange>::const_iterator begin() const {
return Ranges.begin();
}
- SmallVector<dxbc::RTS0::v2::DescriptorRange>::const_iterator end() const {
+ SmallVector<DescriptorRange>::const_iterator end() const {
return Ranges.end();
}
};
diff --git a/llvm/lib/BinaryFormat/DXContainer.cpp b/llvm/lib/BinaryFormat/DXContainer.cpp
index 36d10d0b63078..f79db506962bc 100644
--- a/llvm/lib/BinaryFormat/DXContainer.cpp
+++ b/llvm/lib/BinaryFormat/DXContainer.cpp
@@ -149,6 +149,16 @@ ArrayRef<EnumEntry<RootParameterType>> dxbc::getRootParameterTypes() {
return ArrayRef(RootParameterTypes);
}
+#define DESCRIPTOR_RANGE(Val, Enum) {#Enum, DescriptorRangeType::Enum},
+
+static const EnumEntry<DescriptorRangeType> DescriptorRangeTypes[] = {
+#include "llvm/BinaryFormat/DXContainerConstants.def"
+};
+
+ArrayRef<EnumEntry<DescriptorRangeType>> dxbc::getDescriptorRangeTypes() {
+ return ArrayRef(DescriptorRangeTypes);
+}
+
#define SEMANTIC_KIND(Val, Enum) {#Enum, PSV::SemanticKind::Enum},
static const EnumEntry<PSV::SemanticKind> SemanticKindNames[] = {
diff --git a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
index 770a6d1638e3c..a5e66e73a5dc1 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
@@ -325,7 +325,7 @@ Error MetadataParser::parseDescriptorRange(mcdxbc::DescriptorTable &Table,
if (RangeDescriptorNode->getNumOperands() != 6)
return make_error<InvalidRSMetadataFormat>("Descriptor Range");
- dxbc::RTS0::v2::DescriptorRange Range;
+ mcdxbc::DescriptorRange Range;
std::optional<StringRef> ElementText =
extractMdStringValue(RangeDescriptorNode, 0);
@@ -333,17 +333,11 @@ Error MetadataParser::parseDescriptorRange(mcdxbc::DescriptorTable &Table,
if (!ElementText.has_value())
return make_error<InvalidRSMetadataFormat>("Descriptor Range");
- Range.RangeType =
- StringSwitch<uint32_t>(*ElementText)
- .Case("CBV", to_underlying(dxbc::DescriptorRangeType::CBV))
- .Case("SRV", to_underlying(dxbc::DescriptorRangeType::SRV))
- .Case("UAV", to_underlying(dxbc::DescriptorRangeType::UAV))
- .Case("Sampler", to_underlying(dxbc::DescriptorRangeType::Sampler))
- .Default(~0U);
-
- if (Range.RangeType == ~0U)
- return make_error<GenericRSMetadataError>("Invalid Descriptor Range type.",
- RangeDescriptorNode);
+ Range.RangeType = StringSwitch<dxbc::DescriptorRangeType>(*ElementText)
+ .Case("CBV", dxbc::DescriptorRangeType::CBV)
+ .Case("SRV", dxbc::DescriptorRangeType::SRV)
+ .Case("UAV", dxbc::DescriptorRangeType::UAV)
+ .Case("Sampler", dxbc::DescriptorRangeType::Sampler);
if (std::optional<uint32_t> Val = extractMdIntValue(RangeDescriptorNode, 1))
Range.NumDescriptors = *Val;
@@ -571,12 +565,7 @@ Error MetadataParser::validateRootSignature(
case dxbc::RootParameterType::DescriptorTable: {
const mcdxbc::DescriptorTable &Table =
RSD.ParametersContainer.getDescriptorTable(Info.Location);
- for (const dxbc::RTS0::v2::DescriptorRange &Range : Table) {
- if (!hlsl::rootsig::verifyRangeType(Range.RangeType))
- DeferredErrs =
- joinErrors(std::move(DeferredErrs),
- make_error<RootSignatureValidationError<uint32_t>>(
- "RangeType", Range.RangeType));
+ for (const mcdxbc::DescriptorRange &Range : Table) {
if (!hlsl::rootsig::verifyRegisterSpace(Range.RegisterSpace))
DeferredErrs =
diff --git a/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp b/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
index 72308a3de5fd4..5dbeede778b04 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
@@ -63,13 +63,12 @@ bool verifyRangeType(uint32_t Type) {
return false;
}
-bool verifyDescriptorRangeFlag(uint32_t Version, uint32_t Type,
+bool verifyDescriptorRangeFlag(uint32_t Version, dxbc::DescriptorRangeType Type,
uint32_t FlagsVal) {
using FlagT = dxbc::DescriptorRangeFlags;
FlagT Flags = FlagT(FlagsVal);
- const bool IsSampler =
- (Type == llvm::to_underlying(dxbc::DescriptorRangeType::Sampler));
+ const bool IsSampler = (Type == dxbc::DescriptorRangeType::Sampler);
if (Version == 1) {
// Since the metadata is unversioned, we expect to explicitly see the values
diff --git a/llvm/lib/ObjectYAML/DXContainerEmitter.cpp b/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
index a51821e196cb8..a749f47792453 100644
--- a/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
+++ b/llvm/lib/ObjectYAML/DXContainerEmitter.cpp
@@ -315,9 +315,10 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
P.RootSignature->Parameters.getOrInsertTable(L);
mcdxbc::DescriptorTable Table;
for (const auto &R : TableYaml.Ranges) {
-
- dxbc::RTS0::v2::DescriptorRange Range;
- Range.RangeType = R.RangeType;
+ assert(dxbc::isValidRangeType(R.RangeType) &&
+ "Invalid Descriptor Range Type");
+ mcdxbc::DescriptorRange Range;
+ Range.RangeType = dxbc::DescriptorRangeType(R.RangeType);
Range.NumDescriptors = R.NumDescriptors;
Range.BaseShaderRegister = R.BaseShaderRegister;
Range.RegisterSpace = R.RegisterSpace;
diff --git a/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp b/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
index 5557443a3db93..c6f79621c39de 100644
--- a/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
+++ b/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
@@ -194,7 +194,7 @@ static void validateRootSignature(Module &M,
const mcdxbc::DescriptorTable &Table =
RSD.ParametersContainer.getDescriptorTable(ParamInfo.Location);
- for (const dxbc::RTS0::v2::DescriptorRange &Range : Table.Ranges) {
+ for (const mcdxbc::DescriptorRange &Range : Table.Ranges) {
uint32_t UpperBound =
Range.NumDescriptors == ~0U
? Range.BaseShaderRegister
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.cpp b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
index b05b8ee699ef6..296e11f3ad5e9 100644
--- a/llvm/lib/Target/DirectX/DXILRootSignature.cpp
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
@@ -205,8 +205,11 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
RS.ParametersContainer.getDescriptorTable(Loc);
OS << " NumRanges: " << Table.Ranges.size() << "\n";
- for (const dxbc::RTS0::v2::DescriptorRange Range : Table) {
- OS << " - Range Type: " << Range.RangeType << "\n"
+ for (const mcdxbc::DescriptorRange Range : Table) {
+ OS << " - Range Type: "
+ << enumToStringRef(Range.RangeType,
+ dxbc::getDescriptorRangeTypes())
+ << "\n"
<< " Register Space: " << Range.RegisterSpace << "\n"
<< " Base Shader Register: " << Range.BaseShaderRegister << "\n"
<< " Num Descriptors: " << Range.NumDescriptors << "\n"
diff --git a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-RangeType.ll b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-RangeType.ll
deleted file mode 100644
index 4a65a53f9f727..0000000000000
--- a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-RangeType.ll
+++ /dev/null
@@ -1,20 +0,0 @@
-; RUN: not opt -passes='print<dxil-root-signature>' %s -S -o - 2>&1 | FileCheck %s
-
-target triple = "dxil-unknown-shadermodel6.0-compute"
-
-; CHECK: error: Invalid Descriptor Range type
-; CHECK-NOT: Root Signature Definitions
-
-define void @main() #0 {
-entry:
- ret void
-}
-attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" }
-
-
-!dx.rootsignatures = !{!2} ; list of function/root signature pairs
-!2 = !{ ptr @main, !3, i32 2 } ; function, root signature
-!3 = !{ !5 } ; list of root signature elements
-!5 = !{ !"DescriptorTable", i32 0, !6, !7 }
-!6 = !{ !"Invalid", i32 1, i32 0, i32 -1, i32 -1, i32 4 }
-!7 = !{ !"UAV", i32 5, i32 1, i32 10, i32 5, i32 2 }
diff --git a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Parameters.ll b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Parameters.ll
index 742fea14f5af6..6c6739d6ed390 100644
--- a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Parameters.ll
+++ b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Parameters.ll
@@ -38,13 +38,13 @@ attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" }
;CHECK-NEXT: - Parameter Type: DescriptorTable
;CHECK-NEXT: Shader Visibility: All
;CHECK-NEXT: NumRanges: 2
-;CHECK-NEXT: - Range Type: 0
+;CHECK-NEXT: - Range Type: SRV
;CHECK-NEXT: Register Space: 0
;CHECK-NEXT: Base Shader Register: 1
;CHECK-NEXT: Num Descriptors: 1
;CHECK-NEXT: Offset In Descriptors From Table Start: 4294967295
;CHECK-NEXT: Flags: 4
-;CHECK-NEXT: - Range Type: 1
+;CHECK-NEXT: - Range Type: UAV
;CHECK-NEXT: Register Space: 10
;CHECK-NEXT: Base Shader Register: 1
;CHECK-NEXT: Num Descriptors: 5
|
clang/lib/Sema/SemaHLSL.cpp
Outdated
// value | ||
ReportError(Loc, 1, 0xfffffffe); | ||
} | ||
switch (Clause->Type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be simpler with a toDescriptorRangeType()
helper (whether as a static function or even just a lambda). Also, the flags argument to verifyDescriptorRangeFlag
looks like it's just immediately cast back to a dxbc::DescriptorRangeFlags - we should just update the signature to take the enum in the first place.
if (!llvm::hlsl::rootsig::verifyDescriptorRangeFlag(
Version, toDescriptorRangeType(Clause->Type), Clause->Flags))
ReportFlagError(Loc);
Range.RangeType = StringSwitch<dxbc::DescriptorRangeType>(*ElementText) | ||
.Case("CBV", dxbc::DescriptorRangeType::CBV) | ||
.Case("SRV", dxbc::DescriptorRangeType::SRV) | ||
.Case("UAV", dxbc::DescriptorRangeType::UAV) | ||
.Case("Sampler", dxbc::DescriptorRangeType::Sampler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can use StringSwitch here (and I don't think we should remove the test for an invalid descriptor range type). We'll just need to do an if/else chain over the valid strings and error if we hit something invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bogner - (I think that the latest version is fine) but I'm curious why StringSwitch wouldn't work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the StringSwitch return if the input is invalid? StringSwitch has to handle all cases or it asserts, and giving it a .Default()
case would require us to have a sigil in the DescriptorRangeType
enum for invalid values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that makes sense.
It could return an Expected<DescriptorRangeType>
:)
#include "llvm/BinaryFormat/DXContainerConstants.def" | ||
}; | ||
|
||
ArrayRef<EnumEntry<DescriptorRangeType>> dxbc::getDescriptorRangeTypes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replicating the pattern to use with enumToStringRef
uint32_t NumDescriptors; | ||
uint32_t BaseShaderRegister; | ||
uint32_t RegisterSpace; | ||
uint32_t Flags; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to keep the flags as uint32_t
, since the flags are not mutually exclusive.
for (const dxbc::RTS0::v2::DescriptorRange &Range : Table) { | ||
if (!hlsl::rootsig::verifyRangeType(Range.RangeType)) | ||
DeferredErrs = | ||
joinErrors(std::move(DeferredErrs), | ||
make_error<RootSignatureValidationError<uint32_t>>( | ||
"RangeType", Range.RangeType)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Range type should've been verified and converted to an enum by this point, so this check don't make sense anymore.
bool verifyDescriptorRangeFlag(uint32_t Version, dxbc::DescriptorRangeType Type, | ||
uint32_t Flags) { | ||
return verifyDescriptorRangeFlag(Version, Type, | ||
dxbc::DescriptorRangeFlags(Flags)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated before, this is being added to avoid adding a header include, which is also recommended by llvm coding styles https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible
assert(dxbc::isValidRangeType(R.RangeType) && | ||
"Invalid Descriptor Range Type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we use enums in the yaml represention this should be impossible to trigger. But I am making it clear here, by adding this assert.
joinErrors(std::move(DeferredErrs), | ||
make_error<RootSignatureValidationError<uint32_t>>( | ||
"RangeType", Range.RangeType)); | ||
for (const mcdxbc::DescriptorRange &Range : Table) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove whitespace below this line
inline bool isValidRangeType(uint32_t V) { | ||
static_assert(llvm::to_underlying(dxil::ResourceClass::Sampler) == 3, | ||
"dxil::ResourceClass numeric values must match the Root " | ||
"Signature values associated to each class."); | ||
return V <= llvm::to_underlying(dxil::ResourceClass::Sampler); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are using dxil::ResourceClass
to represent Root Signature Descriptor Range Type, the numeric numbers associated with each member of dxil::ResourceClass
, must match the expected numeric representation for range types in root signature binary representation.
// DESCRIPTOR_RANGE(value, name). | ||
#ifdef DESCRIPTOR_RANGE | ||
|
||
DESCRIPTOR_RANGE(0, SRV) | ||
DESCRIPTOR_RANGE(1, UAV) | ||
DESCRIPTOR_RANGE(2, CBV) | ||
DESCRIPTOR_RANGE(3, Sampler) | ||
#undef DESCRIPTOR_RANGE | ||
#endif // DESCRIPTOR_RANGE | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are no longer needed, they were replaced by dxil::ResourceClass
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): | ||
return true; | ||
}; | ||
|
||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check now can be done by the function isValidRangeType
, and should be called whenever reading root signature binary data.
rewriteOffsetToCurrentByte(BOS, writePlaceholder(BOS)); | ||
for (const auto &Range : Table) { | ||
support::endian::write(BOS, Range.RangeType, llvm::endianness::little); | ||
support::endian::write(BOS, static_cast<uint32_t>(Range.RangeType), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dxil::ResourceClasses
are uint8_t by default, so changing its type or casting is required. I opted to cast, since that seems to be the less disruptive option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good other than my comment about isValidRangeType
static_assert(llvm::to_underlying(dxil::ResourceClass::Sampler) == 3, | ||
"dxil::ResourceClass numeric values must match the Root " | ||
"Signature values associated to each class."); | ||
return V <= llvm::to_underlying(dxil::ResourceClass::Sampler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this assert really captures what you want it to here - if we were to add another value to ResourceClass
after Sampler
this would still be fine but the <=
check wouldn't be correct any more. Similarly, if we reordered SRV
and UAV
for some reason this wouldn't detect it.
A better way to do this would probably be to add a LastEntry
to ResourceClass
that aliases Sampler
, that is:
enum class ResourceClass : uint8_t {
SRV = 0,
UAV,
CBuffer,
Sampler,
LastEntry = Sampler,
};
Then we can just check V <= llvm::to_underlying(dxil::ResourceClass::LastEntry)
here and we don't need an assert to sanity check the enum. Note that -Wcovered-switch
will be fine with this, since the value is indeed covered by the "Sampler" case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bogner Thanks, that make sense. One question, How can I make sure the enum values are still assigned to the correct numbers? Like, SRV must be 0, UAV must be 1 and so on.
We have those tests failing if we change the order of elements in the enum. Is that good enough?
LLVM :: Analysis/DXILResource/buffer-frombinding.ll
LLVM :: CodeGen/DirectX/BufferLoad.ll
LLVM :: CodeGen/DirectX/ContainerData/PSVResources-order.ll
LLVM :: CodeGen/DirectX/ContainerData/PSVResources.ll
LLVM :: CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-AllValidFlagCombinations.ll
LLVM :: CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-AllValidFlagCombinationsV1.ll
LLVM :: CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable.ll
LLVM :: CodeGen/DirectX/CreateHandle.ll
LLVM :: CodeGen/DirectX/CreateHandleFromBinding.ll
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, other than static_assert
on each value individually there isn't much we can do here. I think the test coverage is sufficient here.
MC Descriptor Range Representation currently depend on Object structures. This PR removes that dependency and in order to facilitate removing to_underlying usage in follow-up PRs.