-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[DirectX] Validate registers are bound to root signature #146785
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
base: users/joaosaffran/152229
Are you sure you want to change the base?
Changes from 35 commits
0e8828c
2edd215
242545e
3f8dec4
3b1ce3b
f5720af
ea54904
a49aa19
d90676f
a04eb9f
5994b8f
e8b14bf
8f40e83
28350b2
4fd2e0b
e25ee87
881dd36
8779ee9
c16f15b
c7d5be7
cc5afae
571a0ef
974d4bc
e0bc862
b5a0b32
00a74af
5ccb842
5423aba
a7637a7
da42c0c
edb015d
9f3888e
578a03b
b4a0e16
ef14638
662c3a8
260633c
d42f156
9ee3a4b
6db6224
04658b8
adf3feb
fc338b5
f5b5b3e
03d571a
ef51048
21675e6
47662f0
6da5fb0
0c72dcf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -316,6 +316,32 @@ findOverlappingRanges(llvm::SmallVector<RangeInfo> &Infos) { | |
return Overlaps; | ||
} | ||
|
||
llvm::SmallVector<RangeInfo> | ||
findUnboundRanges(const llvm::SmallVectorImpl<RangeInfo> &Ranges, | ||
joaosaffran marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const llvm::ArrayRef<RangeInfo> &Bindings) { | ||
llvm::SmallVector<RangeInfo> Unbounds; | ||
for (const auto &Range : Ranges) { | ||
bool Bound = false; | ||
// hlsl::rootsig::RangeInfo Range; | ||
// Range.Space = ResBinding.Space; | ||
// Range.LowerBound = ResBinding.LowerBound; | ||
// Range.UpperBound = Range.LowerBound + ResBinding.Size - 1; | ||
|
||
for (const auto &Binding : Bindings) { | ||
if (Range.Space == Binding.Space && | ||
Range.LowerBound >= Binding.LowerBound && | ||
Range.UpperBound <= Binding.UpperBound) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will forward this comment, just so it doesn't get lost:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DXC doesn't seem to consider this a valid binding scenario: https://hlsl.godbolt.org/z/vYo4cY7vv. So we might not want to make it valid in clang. |
||
Bound = true; | ||
break; | ||
} | ||
} | ||
if (!Bound) { | ||
Unbounds.push_back(Range); | ||
} | ||
joaosaffran marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return Unbounds; | ||
} | ||
|
||
} // namespace rootsig | ||
} // namespace hlsl | ||
} // namespace llvm |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
#include "DXILOpLowering.h" | ||
#include "DXILConstants.h" | ||
#include "DXILOpBuilder.h" | ||
#include "DXILRootSignature.h" | ||
#include "DXILShaderFlags.h" | ||
#include "DirectX.h" | ||
#include "llvm/ADT/SmallVector.h" | ||
|
@@ -945,6 +946,7 @@ class DXILOpLoweringLegacy : public ModulePass { | |
AU.addPreserved<DXILResourceWrapperPass>(); | ||
AU.addPreserved<DXILMetadataAnalysisWrapperPass>(); | ||
AU.addPreserved<ShaderFlagsAnalysisWrapper>(); | ||
AU.addPreserved<RootSignatureAnalysisWrapper>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pass, changes the IR, therefore, in order to preserve the data from RootSignatureAnalysisPass, I need to mark it as preserved. |
||
} | ||
}; | ||
char DXILOpLoweringLegacy::ID = 0; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,11 +7,13 @@ | |
//===----------------------------------------------------------------------===// | ||
|
||
#include "DXILPostOptimizationValidation.h" | ||
#include "DXILRootSignature.h" | ||
#include "DXILShaderFlags.h" | ||
#include "DirectX.h" | ||
#include "llvm/ADT/SmallString.h" | ||
#include "llvm/Analysis/DXILMetadataAnalysis.h" | ||
#include "llvm/Analysis/DXILResource.h" | ||
#include "llvm/Frontend/HLSL/RootSignatureValidations.h" | ||
#include "llvm/IR/DiagnosticInfo.h" | ||
#include "llvm/IR/Instructions.h" | ||
#include "llvm/IR/IntrinsicsDirectX.h" | ||
|
@@ -24,6 +26,48 @@ using namespace llvm; | |
using namespace llvm::dxil; | ||
|
||
namespace { | ||
static const char *ResourceClassToString(llvm::dxil::ResourceClass Class) { | ||
joaosaffran marked this conversation as resolved.
Show resolved
Hide resolved
|
||
switch (Class) { | ||
case ResourceClass::SRV: | ||
return "SRV"; | ||
case ResourceClass::UAV: | ||
return "UAV"; | ||
case ResourceClass::CBuffer: | ||
return "CBuffer"; | ||
case ResourceClass::Sampler: | ||
return "Sampler"; | ||
} | ||
joaosaffran marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
static ResourceClass RangeToResourceClass(uint32_t RangeType) { | ||
using namespace dxbc; | ||
switch (static_cast<DescriptorRangeType>(RangeType)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this even be a separate enum? It looks like DescriptorRangeType is identical to ResourceClass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
case DescriptorRangeType::SRV: | ||
return ResourceClass::SRV; | ||
case DescriptorRangeType::UAV: | ||
return ResourceClass::UAV; | ||
case DescriptorRangeType::CBV: | ||
return ResourceClass::CBuffer; | ||
case DescriptorRangeType::Sampler: | ||
return ResourceClass::Sampler; | ||
} | ||
} | ||
|
||
ResourceClass ParameterToResourceClass(uint32_t Type) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type has been validated before this point right? So we know it is a valid type. Can we just do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No because |
||
using namespace dxbc; | ||
switch (Type) { | ||
case llvm::to_underlying(RootParameterType::Constants32Bit): | ||
return ResourceClass::CBuffer; | ||
case llvm::to_underlying(RootParameterType::SRV): | ||
return ResourceClass::SRV; | ||
case llvm::to_underlying(RootParameterType::UAV): | ||
return ResourceClass::UAV; | ||
case llvm::to_underlying(RootParameterType::CBV): | ||
return ResourceClass::CBuffer; | ||
default: | ||
llvm_unreachable("Unknown RootParameterType"); | ||
} | ||
} | ||
|
||
static void reportInvalidDirection(Module &M, DXILResourceMap &DRM) { | ||
for (const auto &UAV : DRM.uavs()) { | ||
|
@@ -84,8 +128,150 @@ static void reportOverlappingBinding(Module &M, DXILResourceMap &DRM) { | |
} | ||
} | ||
|
||
static void reportRegNotBound(Module &M, | ||
llvm::hlsl::rootsig::RangeInfo Unbound) { | ||
SmallString<128> Message; | ||
raw_svector_ostream OS(Message); | ||
OS << "register " << ResourceClassToString(Unbound.Class) | ||
<< " (space=" << Unbound.Space << ", register=" << Unbound.LowerBound | ||
<< ")" | ||
<< " is not defined in Root Signature"; | ||
joaosaffran marked this conversation as resolved.
Show resolved
Hide resolved
|
||
M.getContext().diagnose(DiagnosticInfoGeneric(Message)); | ||
} | ||
|
||
static dxbc::ShaderVisibility | ||
tripleToVisibility(llvm::Triple::EnvironmentType ET) { | ||
assert((ET == Triple::Pixel || ET == Triple::Vertex || | ||
ET == Triple::Geometry || ET == Triple::Hull || | ||
ET == Triple::Domain || ET == Triple::Mesh || | ||
ET == Triple::Compute) && | ||
"Invalid Triple to shader stage conversion"); | ||
|
||
switch (ET) { | ||
case Triple::Pixel: | ||
return dxbc::ShaderVisibility::Pixel; | ||
case Triple::Vertex: | ||
return dxbc::ShaderVisibility::Vertex; | ||
case Triple::Geometry: | ||
return dxbc::ShaderVisibility::Geometry; | ||
case Triple::Hull: | ||
return dxbc::ShaderVisibility::Hull; | ||
case Triple::Domain: | ||
return dxbc::ShaderVisibility::Domain; | ||
case Triple::Mesh: | ||
return dxbc::ShaderVisibility::Mesh; | ||
case Triple::Compute: | ||
return dxbc::ShaderVisibility::All; | ||
default: | ||
llvm_unreachable("Invalid triple to shader stage conversion"); | ||
} | ||
} | ||
|
||
static hlsl::rootsig::RootSignatureBindingValidation | ||
initRSBindingValidation(const mcdxbc::RootSignatureDesc &RSD, | ||
dxbc::ShaderVisibility Visibility) { | ||
|
||
hlsl::rootsig::RootSignatureBindingValidation Validation; | ||
|
||
for (size_t I = 0; I < RSD.ParametersContainer.size(); I++) { | ||
const auto &[Type, Loc] = | ||
RSD.ParametersContainer.getTypeAndLocForParameter(I); | ||
|
||
const auto &Header = RSD.ParametersContainer.getHeader(I); | ||
if (Header.ShaderVisibility != | ||
llvm::to_underlying(dxbc::ShaderVisibility::All) && | ||
Header.ShaderVisibility != llvm::to_underlying(Visibility)) | ||
continue; | ||
|
||
switch (Type) { | ||
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was originally a design decision. The decision was made to avoid converting uint_32t to enum in obj2yaml/yaml2obj and when reading and writing root signature to/from the binary format. This simplified the code at the time. |
||
dxbc::RTS0::v1::RootConstants Const = | ||
RSD.ParametersContainer.getConstant(Loc); | ||
|
||
hlsl::rootsig::RangeInfo Binding; | ||
Binding.LowerBound = Const.ShaderRegister; | ||
Binding.Space = Const.RegisterSpace; | ||
Binding.UpperBound = Binding.LowerBound; | ||
|
||
// Root Constants Bind to CBuffers | ||
Validation.addBinding(ResourceClass::CBuffer, Binding); | ||
|
||
break; | ||
} | ||
|
||
case llvm::to_underlying(dxbc::RootParameterType::SRV): | ||
case llvm::to_underlying(dxbc::RootParameterType::UAV): | ||
case llvm::to_underlying(dxbc::RootParameterType::CBV): { | ||
dxbc::RTS0::v2::RootDescriptor Desc = | ||
RSD.ParametersContainer.getRootDescriptor(Loc); | ||
|
||
hlsl::rootsig::RangeInfo Binding; | ||
Binding.LowerBound = Desc.ShaderRegister; | ||
Binding.Space = Desc.RegisterSpace; | ||
Binding.UpperBound = Binding.LowerBound; | ||
|
||
Validation.addBinding(ParameterToResourceClass(Type), Binding); | ||
break; | ||
} | ||
case llvm::to_underlying(dxbc::RootParameterType::DescriptorTable): { | ||
const mcdxbc::DescriptorTable &Table = | ||
RSD.ParametersContainer.getDescriptorTable(Loc); | ||
|
||
for (const dxbc::RTS0::v2::DescriptorRange &Range : Table.Ranges) { | ||
hlsl::rootsig::RangeInfo Binding; | ||
Binding.LowerBound = Range.BaseShaderRegister; | ||
Binding.Space = Range.RegisterSpace; | ||
Binding.UpperBound = Binding.LowerBound + Range.NumDescriptors - 1; | ||
Validation.addBinding(RangeToResourceClass(Range.RangeType), Binding); | ||
} | ||
break; | ||
} | ||
} | ||
} | ||
|
||
return Validation; | ||
} | ||
|
||
std::optional<mcdxbc::RootSignatureDesc> | ||
getRootSignature(RootSignatureBindingInfo &RSBI, | ||
dxil::ModuleMetadataInfo &MMI) { | ||
if (MMI.EntryPropertyVec.size() == 0) | ||
return std::nullopt; | ||
std::optional<mcdxbc::RootSignatureDesc> RootSigDesc = | ||
RSBI.getDescForFunction(MMI.EntryPropertyVec[0].Entry); | ||
if (!RootSigDesc) | ||
return std::nullopt; | ||
return RootSigDesc; | ||
} | ||
|
||
static void reportUnboundRegisters( | ||
Module &M, | ||
const llvm::hlsl::rootsig::RootSignatureBindingValidation &Validation, | ||
ResourceClass Class, | ||
const iterator_range<SmallVectorImpl<dxil::ResourceInfo>::iterator> | ||
&Resources) { | ||
SmallVector<hlsl::rootsig::RangeInfo> Ranges; | ||
for (auto Res = Resources.begin(), End = Resources.end(); Res != End; Res++) { | ||
ResourceInfo::ResourceBinding ResBinding = Res->getBinding(); | ||
hlsl::rootsig::RangeInfo Range; | ||
Range.Space = ResBinding.Space; | ||
Range.LowerBound = ResBinding.LowerBound; | ||
Range.UpperBound = Range.LowerBound + ResBinding.Size - 1; | ||
Range.Class = Class; | ||
Ranges.push_back(Range); | ||
} | ||
|
||
SmallVector<hlsl::rootsig::RangeInfo> Unbounds = | ||
hlsl::rootsig::findUnboundRanges(Ranges, | ||
Validation.getBindingsOfType(Class)); | ||
for (const auto &Unbound : Unbounds) | ||
reportRegNotBound(M, Unbound); | ||
} | ||
|
||
static void reportErrors(Module &M, DXILResourceMap &DRM, | ||
DXILResourceBindingInfo &DRBI) { | ||
DXILResourceBindingInfo &DRBI, | ||
RootSignatureBindingInfo &RSBI, | ||
dxil::ModuleMetadataInfo &MMI) { | ||
if (DRM.hasInvalidCounterDirection()) | ||
reportInvalidDirection(M, DRM); | ||
|
||
|
@@ -94,14 +280,30 @@ static void reportErrors(Module &M, DXILResourceMap &DRM, | |
|
||
assert(!DRBI.hasImplicitBinding() && "implicit bindings should be handled in " | ||
"DXILResourceImplicitBinding pass"); | ||
|
||
if (auto RSD = getRootSignature(RSBI, MMI)) { | ||
|
||
llvm::hlsl::rootsig::RootSignatureBindingValidation Validation = | ||
initRSBindingValidation(*RSD, tripleToVisibility(MMI.ShaderProfile)); | ||
|
||
reportUnboundRegisters(M, Validation, ResourceClass::CBuffer, | ||
DRM.cbuffers()); | ||
reportUnboundRegisters(M, Validation, ResourceClass::UAV, DRM.uavs()); | ||
reportUnboundRegisters(M, Validation, ResourceClass::Sampler, | ||
DRM.samplers()); | ||
reportUnboundRegisters(M, Validation, ResourceClass::SRV, DRM.srvs()); | ||
} | ||
} | ||
} // namespace | ||
|
||
PreservedAnalyses | ||
DXILPostOptimizationValidation::run(Module &M, ModuleAnalysisManager &MAM) { | ||
DXILResourceMap &DRM = MAM.getResult<DXILResourceAnalysis>(M); | ||
DXILResourceBindingInfo &DRBI = MAM.getResult<DXILResourceBindingAnalysis>(M); | ||
reportErrors(M, DRM, DRBI); | ||
RootSignatureBindingInfo &RSBI = MAM.getResult<RootSignatureAnalysis>(M); | ||
ModuleMetadataInfo &MMI = MAM.getResult<DXILMetadataAnalysis>(M); | ||
|
||
reportErrors(M, DRM, DRBI, RSBI, MMI); | ||
return PreservedAnalyses::all(); | ||
} | ||
|
||
|
@@ -113,7 +315,12 @@ class DXILPostOptimizationValidationLegacy : public ModulePass { | |
getAnalysis<DXILResourceWrapperPass>().getResourceMap(); | ||
DXILResourceBindingInfo &DRBI = | ||
getAnalysis<DXILResourceBindingWrapperPass>().getBindingInfo(); | ||
reportErrors(M, DRM, DRBI); | ||
RootSignatureBindingInfo &RSBI = | ||
getAnalysis<RootSignatureAnalysisWrapper>().getRSInfo(); | ||
dxil::ModuleMetadataInfo &MMI = | ||
getAnalysis<DXILMetadataAnalysisWrapperPass>().getModuleMetadata(); | ||
|
||
reportErrors(M, DRM, DRBI, RSBI, MMI); | ||
return false; | ||
} | ||
StringRef getPassName() const override { | ||
|
@@ -125,10 +332,13 @@ class DXILPostOptimizationValidationLegacy : public ModulePass { | |
void getAnalysisUsage(llvm::AnalysisUsage &AU) const override { | ||
AU.addRequired<DXILResourceWrapperPass>(); | ||
AU.addRequired<DXILResourceBindingWrapperPass>(); | ||
AU.addRequired<DXILMetadataAnalysisWrapperPass>(); | ||
AU.addRequired<RootSignatureAnalysisWrapper>(); | ||
AU.addPreserved<DXILResourceWrapperPass>(); | ||
AU.addPreserved<DXILResourceBindingWrapperPass>(); | ||
AU.addPreserved<DXILMetadataAnalysisWrapperPass>(); | ||
AU.addPreserved<ShaderFlagsAnalysisWrapper>(); | ||
AU.addPreserved<RootSignatureAnalysisWrapper>(); | ||
} | ||
}; | ||
char DXILPostOptimizationValidationLegacy::ID = 0; | ||
|
@@ -139,6 +349,8 @@ INITIALIZE_PASS_BEGIN(DXILPostOptimizationValidationLegacy, DEBUG_TYPE, | |
INITIALIZE_PASS_DEPENDENCY(DXILResourceBindingWrapperPass) | ||
INITIALIZE_PASS_DEPENDENCY(DXILResourceTypeWrapperPass) | ||
INITIALIZE_PASS_DEPENDENCY(DXILResourceWrapperPass) | ||
INITIALIZE_PASS_DEPENDENCY(DXILMetadataAnalysisWrapperPass) | ||
INITIALIZE_PASS_DEPENDENCY(RootSignatureAnalysisWrapper) | ||
joaosaffran marked this conversation as resolved.
Show resolved
Hide resolved
|
||
INITIALIZE_PASS_END(DXILPostOptimizationValidationLegacy, DEBUG_TYPE, | ||
"DXIL Post Optimization Validation", false, 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.
I still think we can simplify this logic as denote 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.
I am not sure, that would make it easier, since we would need to make sure the array is sorted as well as using
lower_bound
to keep track of where each element starts. I am not opposed to this strategy thought, just want to hear @bogner opinion on this first.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.
The LLVM Programmers manual explicitly states:
(Source: https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options)
llvm::DenseMap
is generally preferred. That said, sincedxil::ResourceClass
has a valid value range [0,3], maybe we should just declare a 4-element array...Uh oh!
There was an error while loading. Please reload this page.
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 can be done with an insert-then-query approach, so the sorted vector is a reasonable suggestion. That said, this is the third implementation of pretty much the same algorithm for calculating binding ranges at this point. See my top level comment.