[HLSL] Add descriptor table metadata parsing #142492
Open
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
added 30 commits
April 24, 2025 22:06
|
@llvm/pr-subscribers-backend-directx @llvm/pr-subscribers-llvm-binary-utilities Author: None (joaosaffran) Changes
Patch is 20.71 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/142492.diff 8 Files Affected:
diff --git a/llvm/include/llvm/BinaryFormat/DXContainerConstants.def b/llvm/include/llvm/BinaryFormat/DXContainerConstants.def
index 501ef0c31cdd0..fa66450c563c4 100644
--- a/llvm/include/llvm/BinaryFormat/DXContainerConstants.def
+++ b/llvm/include/llvm/BinaryFormat/DXContainerConstants.def
@@ -98,6 +98,17 @@ DESCRIPTOR_RANGE_FLAG(16, DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS)
#undef DESCRIPTOR_RANGE_FLAG
#endif // DESCRIPTOR_RANGE_FLAG
+// DESCRIPTOR_RANGE(value, name).
+#ifdef DESCRIPTOR_RANGE
+ DESCRIPTOR_RANGE(4, ERROR)
+ DESCRIPTOR_RANGE(0, SRV)
+ DESCRIPTOR_RANGE(1, UAV)
+ DESCRIPTOR_RANGE(2, CBV)
+ DESCRIPTOR_RANGE(3, Sampler)
+DESCRIPTOR_RANGE(0, NONE)
+#undef DESCRIPTOR_RANGE
+#endif // DESCRIPTOR_RANGE
+
#ifdef ROOT_PARAMETER
ROOT_PARAMETER(0, DescriptorTable)
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.cpp b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
index 878272537d366..33a74d71027aa 100644
--- a/llvm/lib/Target/DirectX/DXILRootSignature.cpp
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
@@ -12,6 +12,7 @@
//===----------------------------------------------------------------------===//
#include "DXILRootSignature.h"
#include "DirectX.h"
+#include "llvm/ADT/STLForwardCompat.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Analysis/DXILMetadataAnalysis.h"
@@ -27,6 +28,7 @@
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/raw_ostream.h"
+#include <cstddef>
#include <cstdint>
#include <optional>
#include <utility>
@@ -166,6 +168,90 @@ static bool parseRootDescriptors(LLVMContext *Ctx,
return false;
}
+static bool parseDescriptorRange(LLVMContext *Ctx,
+ mcdxbc::RootSignatureDesc &RSD,
+ mcdxbc::DescriptorTable &Table,
+ MDNode *RangeDescriptorNode) {
+
+ if (RangeDescriptorNode->getNumOperands() != 6)
+ return reportError(Ctx, "Invalid format for Descriptor Range");
+
+ dxbc::RTS0::v2::DescriptorRange Range;
+
+ std::optional<StringRef> ElementText =
+ extractMdStringValue(RangeDescriptorNode, 0);
+
+ if (!ElementText.has_value())
+ return reportError(Ctx, "Descriptor Range, first element is not a string.");
+
+ Range.RangeType =
+ StringSwitch<uint32_t>(*ElementText)
+ .Case("CBV", llvm::to_underlying(dxbc::DescriptorRangeType::CBV))
+ .Case("SRV", llvm::to_underlying(dxbc::DescriptorRangeType::SRV))
+ .Case("UAV", llvm::to_underlying(dxbc::DescriptorRangeType::UAV))
+ .Case("Sampler",
+ llvm::to_underlying(dxbc::DescriptorRangeType::Sampler))
+ .Default(llvm::to_underlying(dxbc::DescriptorRangeType::ERROR));
+
+ if (std::optional<uint32_t> Val = extractMdIntValue(RangeDescriptorNode, 1))
+ Range.NumDescriptors = *Val;
+ else
+ return reportError(Ctx, "Invalid value for Number of Descriptor in Range");
+
+ if (std::optional<uint32_t> Val = extractMdIntValue(RangeDescriptorNode, 2))
+ Range.BaseShaderRegister = *Val;
+ else
+ return reportError(Ctx, "Invalid value for BaseShaderRegister");
+
+ if (std::optional<uint32_t> Val = extractMdIntValue(RangeDescriptorNode, 3))
+ Range.RegisterSpace = *Val;
+ else
+ return reportError(Ctx, "Invalid value for RegisterSpace");
+
+ if (std::optional<uint32_t> Val = extractMdIntValue(RangeDescriptorNode, 4))
+ Range.OffsetInDescriptorsFromTableStart = *Val;
+ else
+ return reportError(Ctx,
+ "Invalid value for OffsetInDescriptorsFromTableStart");
+
+ if (std::optional<uint32_t> Val = extractMdIntValue(RangeDescriptorNode, 5))
+ Range.Flags = *Val;
+ else
+ return reportError(Ctx, "Invalid value for Descriptor Range Flags");
+
+ Table.Ranges.push_back(Range);
+ return false;
+}
+
+static bool parseDescriptorTable(LLVMContext *Ctx,
+ mcdxbc::RootSignatureDesc &RSD,
+ MDNode *DescriptorTableNode) {
+ if (DescriptorTableNode->getNumOperands() < 2)
+ return reportError(Ctx, "Invalid format for Descriptor Table");
+
+ dxbc::RTS0::v1::RootParameterHeader Header;
+ if (std::optional<uint32_t> Val = extractMdIntValue(DescriptorTableNode, 1))
+ Header.ShaderVisibility = *Val;
+ else
+ return reportError(Ctx, "Invalid value for ShaderVisibility");
+
+ mcdxbc::DescriptorTable Table;
+ Header.ParameterType =
+ llvm::to_underlying(dxbc::RootParameterType::DescriptorTable);
+
+ for (unsigned int I = 2; I < DescriptorTableNode->getNumOperands(); I++) {
+ MDNode *Element = dyn_cast<MDNode>(DescriptorTableNode->getOperand(I));
+ if (Element == nullptr)
+ return reportError(Ctx, "Missing Root Element Metadata Node.");
+
+ if (parseDescriptorRange(Ctx, RSD, Table, Element))
+ return true;
+ }
+
+ RSD.ParametersContainer.addParameter(Header, Table);
+ return false;
+}
+
static bool parseRootSignatureElement(LLVMContext *Ctx,
mcdxbc::RootSignatureDesc &RSD,
MDNode *Element) {
@@ -180,6 +266,7 @@ static bool parseRootSignatureElement(LLVMContext *Ctx,
.Case("RootCBV", RootSignatureElementKind::RootDescriptors)
.Case("RootSRV", RootSignatureElementKind::RootDescriptors)
.Case("RootUAV", RootSignatureElementKind::RootDescriptors)
+ .Case("DescriptorTable", RootSignatureElementKind::DescriptorTable)
.Default(RootSignatureElementKind::Error);
switch (ElementKind) {
@@ -190,6 +277,8 @@ static bool parseRootSignatureElement(LLVMContext *Ctx,
return parseRootConstants(Ctx, RSD, Element);
case RootSignatureElementKind::RootDescriptors:
return parseRootDescriptors(Ctx, RSD, Element);
+ case RootSignatureElementKind::DescriptorTable:
+ return parseDescriptorTable(Ctx, RSD, Element);
case RootSignatureElementKind::Error:
return reportError(Ctx, "Invalid Root Signature Element: " +
ElementText->getString());
@@ -225,11 +314,84 @@ static bool verifyRegisterValue(uint32_t RegisterValue) {
}
static bool verifyRegisterSpace(uint32_t RegisterSpace) {
- return !(RegisterSpace >= 0xFFFFFFF0 && RegisterSpace <= 0xFFFFFFFF);
+ return !(RegisterSpace >= 0xFFFFFFF0 && RegisterSpace < 0xFFFFFFFF);
}
static bool verifyDescriptorFlag(uint32_t Flags) { return (Flags & ~0xE) == 0; }
+static 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;
+}
+
+static bool verifyDescriptorRangeFlag(uint32_t Version, uint32_t Type,
+ uint32_t Flags) {
+ if (Version == 1 &&
+ Type == llvm::to_underlying(dxbc::DescriptorRangeType::Sampler))
+ return Flags == 0;
+
+ if (Version == 2 &&
+ Type == llvm::to_underlying(dxbc::DescriptorRangeType::Sampler)) {
+ switch (Flags) {
+ case 0:
+ case llvm::to_underlying(dxbc::DescriptorRangeFlag::DATA_VOLATILE):
+ case llvm::to_underlying(
+ dxbc::DescriptorRangeFlag::
+ DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS):
+ return true;
+ }
+ return false;
+ }
+
+ if (Version == 1 &&
+ Type != llvm::to_underlying(dxbc::DescriptorRangeType::Sampler))
+ return Flags ==
+ llvm::to_underlying(dxbc::DescriptorRangeFlag::DESCRIPTORS_VOLATILE);
+
+ if (Version == 2 &&
+ Type != llvm::to_underlying(dxbc::DescriptorRangeType::Sampler)) {
+ switch (Flags) {
+ case 0:
+ case llvm::to_underlying(dxbc::DescriptorRangeFlag::DESCRIPTORS_VOLATILE):
+ case llvm::to_underlying(dxbc::DescriptorRangeFlag::DATA_VOLATILE):
+ case llvm::to_underlying(dxbc::DescriptorRangeFlag::DATA_STATIC):
+ case llvm::to_underlying(
+ dxbc::DescriptorRangeFlag::DATA_STATIC_WHILE_SET_AT_EXECUTE):
+ case llvm::to_underlying(
+ dxbc::DescriptorRangeFlag::
+ DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS):
+ case llvm::to_underlying(dxbc::DescriptorRangeFlag::DESCRIPTORS_VOLATILE) |
+ llvm::to_underlying(dxbc::DescriptorRangeFlag::DATA_VOLATILE):
+ case llvm::to_underlying(dxbc::DescriptorRangeFlag::DESCRIPTORS_VOLATILE) |
+ llvm::to_underlying(
+ dxbc::DescriptorRangeFlag::DATA_STATIC_WHILE_SET_AT_EXECUTE):
+ case llvm::to_underlying(
+ dxbc::DescriptorRangeFlag::
+ DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS) |
+ llvm::to_underlying(dxbc::DescriptorRangeFlag::DATA_VOLATILE):
+ case llvm::to_underlying(
+ dxbc::DescriptorRangeFlag::
+ DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS) |
+ llvm::to_underlying(dxbc::DescriptorRangeFlag::DATA_STATIC):
+ case llvm::to_underlying(
+ dxbc::DescriptorRangeFlag::
+ DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS) |
+ llvm::to_underlying(
+ dxbc::DescriptorRangeFlag::DATA_STATIC_WHILE_SET_AT_EXECUTE):
+ return true;
+ }
+ return false;
+ }
+ return false;
+}
+
static bool validate(LLVMContext *Ctx, const mcdxbc::RootSignatureDesc &RSD) {
if (!verifyVersion(RSD.Version)) {
@@ -268,6 +430,22 @@ static bool validate(LLVMContext *Ctx, const mcdxbc::RootSignatureDesc &RSD) {
}
break;
}
+ case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable): {
+ const mcdxbc::DescriptorTable &Table =
+ RSD.ParametersContainer.getDescriptorTable(Info.Location);
+ for (const dxbc::RTS0::v2::DescriptorRange &Range : Table) {
+ if (!verifyRangeType(Range.RangeType))
+ return reportValueError(Ctx, "RangeType", Range.RangeType);
+
+ if (!verifyRegisterSpace(Range.RegisterSpace))
+ return reportValueError(Ctx, "RegisterSpace", Range.RegisterSpace);
+
+ if (!verifyDescriptorRangeFlag(RSD.Version, Range.RangeType,
+ Range.Flags))
+ return reportValueError(Ctx, "DescriptorFlag", Range.Flags);
+ }
+ break;
+ }
}
}
@@ -380,14 +558,12 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
OS << "Definition for '" << F.getName() << "':\n";
// start root signature header
- Space++;
OS << indent(Space) << "Flags: " << format_hex(RS.Flags, 8) << "\n";
OS << indent(Space) << "Version: " << RS.Version << "\n";
OS << indent(Space) << "RootParametersOffset: " << RS.RootParameterOffset
<< "\n";
OS << indent(Space) << "NumParameters: " << RS.ParametersContainer.size()
<< "\n";
- Space++;
for (size_t I = 0; I < RS.ParametersContainer.size(); I++) {
const auto &[Type, Loc] =
RS.ParametersContainer.getTypeAndLocForParameter(I);
@@ -410,7 +586,7 @@ 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::CBV):
case llvm::to_underlying(dxbc::RootParameterType::UAV):
case llvm::to_underlying(dxbc::RootParameterType::SRV): {
const dxbc::RTS0::v2::RootDescriptor &Descriptor =
@@ -423,8 +599,28 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
OS << indent(Space + 2) << "Flags: " << Descriptor.Flags << "\n";
break;
}
+ case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable): {
+ const mcdxbc::DescriptorTable &Table =
+ RS.ParametersContainer.getDescriptorTable(Loc);
+ OS << indent(Space + 2) << "NumRanges: " << Table.Ranges.size() << "\n";
+
+ for (const dxbc::RTS0::v2::DescriptorRange Range : Table) {
+ OS << indent(Space + 2) << "- Range Type: " << Range.RangeType
+ << "\n";
+ OS << indent(Space + 4) << "Register Space: " << Range.RegisterSpace
+ << "\n";
+ OS << indent(Space + 4)
+ << "Base Shader Register: " << Range.BaseShaderRegister << "\n";
+ OS << indent(Space + 4) << "Num Descriptors: " << Range.NumDescriptors
+ << "\n";
+ OS << indent(Space + 4) << "Offset In Descriptors From Table Start: "
+ << Range.OffsetInDescriptorsFromTableStart << "\n";
+ if (RS.Version > 1)
+ OS << indent(Space + 4) << "Flags: " << Range.Flags << "\n";
+ }
+ break;
+ }
}
- Space--;
}
OS << indent(Space) << "NumStaticSamplers: " << 0 << "\n";
OS << indent(Space) << "StaticSamplersOffset: " << RS.StaticSamplersOffset
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.h b/llvm/lib/Target/DirectX/DXILRootSignature.h
index b8742d1b1fdfd..d1ffc8cacd6f0 100644
--- a/llvm/lib/Target/DirectX/DXILRootSignature.h
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.h
@@ -28,7 +28,8 @@ enum class RootSignatureElementKind {
Error = 0,
RootFlags = 1,
RootConstants = 2,
- RootDescriptors = 3
+ RootDescriptors = 3,
+ DescriptorTable = 4,
};
class RootSignatureAnalysis : public AnalysisInfoMixin<RootSignatureAnalysis> {
friend AnalysisInfoMixin<RootSignatureAnalysis>;
diff --git a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-Flag.ll b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-Flag.ll
new file mode 100644
index 0000000000000..37d87986aa25f
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-Flag.ll
@@ -0,0 +1,20 @@
+; 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 value for DescriptorFlag: 22
+; 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 } ; function, root signature
+!3 = !{ !5 } ; list of root signature elements
+!5 = !{ !"DescriptorTable", i32 0, !6, !7 }
+!6 = !{ !"SRV", i32 0, i32 0, i32 -1, i32 -1, i32 22 }
+!7 = !{ !"UAV", i32 5, i32 1, i32 10, i32 5, i32 2 }
diff --git a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-RangeType.ll b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-RangeType.ll
new file mode 100644
index 0000000000000..1d18c0c7ff882
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-RangeType.ll
@@ -0,0 +1,20 @@
+; 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 value for RangeType: 4
+; 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 } ; function, root signature
+!3 = !{ !5 } ; list of root signature elements
+!5 = !{ !"DescriptorTable", i32 0, !6, !7 }
+!6 = !{ !"Invalid", i32 0, 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-DescriptorTable-Invalid-RegisterSpace.ll b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-RegisterSpace.ll
new file mode 100644
index 0000000000000..7f046a5748d5b
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-Invalid-RegisterSpace.ll
@@ -0,0 +1,20 @@
+; 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 value for RegisterSpace: 4294967280
+; 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 } ; function, root signature
+!3 = !{ !5 } ; list of root signature elements
+!5 = !{ !"DescriptorTable", i32 0, !6, !7 }
+!6 = !{ !"SRV", i32 0, i32 0, i32 10, i32 -1, i32 4 }
+!7 = !{ !"UAV", i32 5, i32 1, i32 4294967280, i32 5, i32 2 }
diff --git a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable.ll b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable.ll
new file mode 100644
index 0000000000000..af4acfd2736d9
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable.ll
@@ -0,0 +1,48 @@
+; RUN: opt %s -dxil-embed -dxil-globals -S -o - | FileCheck %s
+; RUN: llc %s --filetype=obj -o - | obj2yaml | FileCheck %s --check-prefix=DXC
+
+target triple = "dxil-unknown-shadermodel6.0-compute"
+
+; CHECK: @dx.rts0 = private constant [92 x i8] c"{{.*}}", section "RTS0", align 4
+
+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 } ; function, root signature
+!3 = !{ !5 } ; list of root signature elements
+!5 = !{ !"DescriptorTable", i32 0, !6, !7 }
+!6 = !{ !"SRV", i32 0, i32 0, i32 -1, i32 -1, i32 4 }
+!7 = !{ !"UAV", i32 5, i32 1, i32 10, i32 5, i32 2 }
+
+; DXC: - Name: RTS0
+; DXC-NEXT: Size: 92
+; DXC-NEXT: RootSignature:
+; DXC-NEXT: Version: 2
+; DXC-NEXT: NumRootParameters: 1
+; DXC-NEXT: RootParametersOffset: 24
+; DXC-NEXT: NumStaticSamplers: 0
+; DXC-NEXT: StaticSamplersOffset: 0
+; DXC-NEXT: Parameters:
+; DXC-NEXT: - ParameterType: 0
+; DXC-NEXT: ShaderVisibility: 0
+; DXC-NEXT: Table:
+; DXC-NEXT: NumRanges: 2
+; DXC-NEXT: RangesOffset: 44
+; DXC-NEXT: Ranges:
+; DXC-NEXT: - RangeType: 0
+; DXC-NEXT: NumDescriptors: 0
+; DXC-NEXT: BaseShaderRegister: 0
+; DXC-NEXT: RegisterSpace: 4294967295
+; DXC-NEXT: OffsetInDescriptorsFromTableStart: 4294967295
+; DXC-NEXT: DATA_VOLATILE: true
+; DXC-NEXT: - RangeType: 1
+; DXC-NEXT: NumDescriptors: 5
+; DXC-NEXT: BaseShaderRegister: 1
+; DXC-NEXT: RegisterSpace: 10
+; DXC-NEXT: OffsetInDescriptorsFromTableStart: 5
+; DXC-NEXT: DESCRIPTORS_VOLATILE: true
diff --git a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Parameters.ll b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Parameters.ll
index 714c76213e1b5..28768e252d85a 100644
--- a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Parameters.ll
+++ b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Parameters.ll
@@ -12,16 +12,19 @@ attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" }
!dx.rootsignatures = !{!2} ; list of function/root signature pairs
!2 = !{ ptr @main, !3 } ; function, root signature
-!3 = !{ !4, !5, !6 } ; list of root signature elements
+!3 = !{ !4, !5, !6, !7 } ; list of root signature elements
!4 = !{ !"RootFlags", i32 1 } ; 1 = allow_input_assembler_input_layout
!5 = !{ !"RootConstants", i32 0, i32 1, i32 2, i32 3 }
!6 = !{ !"RootSRV", i32 1, i32 4, i32 5, i32 6 }
+!7 = !{ !"DescriptorTable", i32 0, !8, !9 }
+!8 = !{ !"SRV", i32 0, i32 0, i32 -1, i32 -1, i32 4 }
+!9 = !{ !"UAV", i32 5, i32 1, i32 10, i32 5, i32 2 }
;CHECK-LABEL: Definition for 'main':
;CHECK-NEXT: Flags: 0x000001
;CHECK-NEXT: Version: 2
;CHECK-NEXT: RootParametersOffset: 24
-;CHECK-NEXT: NumParameters: 2
+;CHECK-NEXT: NumParameters: 3
;CHECK-NEXT: - Parameter Type: 1
;CHECK-NEXT: Shader Visibility: 0
;CHECK-NEXT: Register Space: 2
@@ -32,5 +35,20 @@ attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" }
;CHECK-NEXT: Register Space: 5
;CHECK-NEXT: Shader Reg...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
joaosaffran
commented
Jun 4, 2025
| @@ -236,11 +323,84 @@ static bool verifyRegisterValue(uint32_t RegisterValue) { | |||
| // This Range is reserverved, therefore invalid, according to the spec | |||
| // https://github.com/llvm/wg-hlsl/blob/main/proposals/0002-root-signature-in-clang.md#all-the-values-should-be-legal | |||
| static bool verifyRegisterSpace(uint32_t RegisterSpace) { | |||
| return !(RegisterSpace >= 0xFFFFFFF0 && RegisterSpace <= 0xFFFFFFFF); | |||
| return !(RegisterSpace >= 0xFFFFFFF0 && RegisterSpace < 0xFFFFFFFF); | |||
There was a problem hiding this comment.
This change happen because in Descriptor Ranges can contain a Register Space of -1. But, it might be better to just have a separate method to deal with Ranges validation, thoughts ?
There was a problem hiding this comment.
I am not aware of when space can be -1? Can you elaborate.
I thought it was only the offset parameter (append) or the numDescriptors parameter (unbounded)
There was a problem hiding this comment.
You are correct, sorry, copied the example from here: https://github.com/llvm/wg-hlsl/blob/main/proposals/0002-root-signature-in-clang.md#descriptor-ranges, that make me confused. Will fix this code, and the spec
inbelic
reviewed
Jun 5, 2025
Comment on lines
+103
to
+107
| DESCRIPTOR_RANGE(4, ERROR) | ||
| DESCRIPTOR_RANGE(0, SRV) | ||
| DESCRIPTOR_RANGE(1, UAV) | ||
| DESCRIPTOR_RANGE(2, CBV) | ||
| DESCRIPTOR_RANGE(3, Sampler) |
There was a problem hiding this comment.
Suggested change
| DESCRIPTOR_RANGE(4, ERROR) | |
| DESCRIPTOR_RANGE(0, SRV) | |
| DESCRIPTOR_RANGE(1, UAV) | |
| DESCRIPTOR_RANGE(2, CBV) | |
| DESCRIPTOR_RANGE(3, Sampler) | |
| DESCRIPTOR_RANGE(4, ERROR) | |
| DESCRIPTOR_RANGE(0, SRV) | |
| DESCRIPTOR_RANGE(1, UAV) | |
| DESCRIPTOR_RANGE(2, CBV) | |
| DESCRIPTOR_RANGE(3, Sampler) |
nit
| if (std::optional<uint32_t> Val = extractMdIntValue(RangeDescriptorNode, 1)) | ||
| Range.NumDescriptors = *Val; | ||
| else | ||
| return reportError(Ctx, "Invalid value for Number of Descriptor in Range"); |
There was a problem hiding this comment.
We could probably have a function like reportInvalidTypeError for all of these. Similar to reportValueError but with a message that will differentiate between the two.
Something like "Parameter: ... expected metadata node of type ... but got ..." I am not sure if you can retrieve the type of an MDNode?
| @@ -236,11 +323,84 @@ static bool verifyRegisterValue(uint32_t RegisterValue) { | |||
| // This Range is reserverved, therefore invalid, according to the spec | |||
| // https://github.com/llvm/wg-hlsl/blob/main/proposals/0002-root-signature-in-clang.md#all-the-values-should-be-legal | |||
| static bool verifyRegisterSpace(uint32_t RegisterSpace) { | |||
| return !(RegisterSpace >= 0xFFFFFFF0 && RegisterSpace <= 0xFFFFFFFF); | |||
| return !(RegisterSpace >= 0xFFFFFFF0 && RegisterSpace < 0xFFFFFFFF); | |||
There was a problem hiding this comment.
I am not aware of when space can be -1? Can you elaborate.
I thought it was only the offset parameter (append) or the numDescriptors parameter (unbounded)
| @@ -391,14 +567,12 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M, | |||
| OS << "Definition for '" << F.getName() << "':\n"; | |||
|
|
|||
| // start root signature header | |||
| Space++; | |||
There was a problem hiding this comment.
These don't seem like a related change? I guess it is to conform to use explicit idents everywhere instead of incremeting/decrementing?
There was a problem hiding this comment.
Yeah, it is just to add consistency in the idents
|
|
||
| if (Version == 2 && | ||
| Type != llvm::to_underlying(dxbc::DescriptorRangeType::Sampler)) { | ||
| switch (Flags) { |
There was a problem hiding this comment.
Personally, I find this switch statement really difficult to follow along and verify it is doing what we want.
I am pretty sure it is implementing the logic from here.
Maybe a templated isFlagSet function and an if-else chain might help?
Or a comment describing/linking to the docs to help.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes: #126640