-
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 41 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 |
---|---|---|
|
@@ -66,6 +66,22 @@ BindingInfo::RegisterSpace::findAvailableBinding(int32_t Size) { | |
return std::nullopt; | ||
} | ||
|
||
bool BindingInfo::RegisterSpace::isBound(BindingRange B) { | ||
for (BindingRange &R : FreeRanges) { | ||
if (B.LowerBound >= R.LowerBound && B.LowerBound < R.UpperBound && | ||
B.UpperBound > R.LowerBound && B.UpperBound <= R.UpperBound) | ||
return false; | ||
} | ||
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 think we can take advantage of |
||
return true; | ||
} | ||
|
||
bool BindingInfo::isBound(dxil::ResourceClass RC, uint32_t Space, | ||
BindingRange B) { | ||
BindingSpaces &BS = getBindingSpaces(RC); | ||
RegisterSpace &RS = BS.getOrInsertSpace(Space); | ||
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. Is it worth it to add a 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. |
||
return RS.isBound(B); | ||
} | ||
|
||
BindingInfo BindingInfoBuilder::calculateBindingInfo( | ||
llvm::function_ref<void(const BindingInfoBuilder &Builder, | ||
const Binding &Overlapping)> | ||
|
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,6 +7,7 @@ | |||||||||||||
//===----------------------------------------------------------------------===// | ||||||||||||||
|
||||||||||||||
#include "DXILPostOptimizationValidation.h" | ||||||||||||||
#include "DXILRootSignature.h" | ||||||||||||||
#include "DXILShaderFlags.h" | ||||||||||||||
#include "DirectX.h" | ||||||||||||||
#include "llvm/ADT/SmallString.h" | ||||||||||||||
|
@@ -24,6 +25,35 @@ using namespace llvm; | |||||||||||||
using namespace llvm::dxil; | ||||||||||||||
|
||||||||||||||
namespace { | ||||||||||||||
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 +114,112 @@ static void reportOverlappingBinding(Module &M, DXILResourceMap &DRM) { | |||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
static void | ||||||||||||||
reportRegNotBound(Module &M, ResourceClass Class, | ||||||||||||||
llvm::dxil::ResourceInfo::ResourceBinding Unbound) { | ||||||||||||||
SmallString<128> Message; | ||||||||||||||
raw_svector_ostream OS(Message); | ||||||||||||||
OS << "register " << getResourceClassName(Class) | ||||||||||||||
<< " (space=" << Unbound.Space << ", register=" << Unbound.LowerBound | ||||||||||||||
<< ")" | ||||||||||||||
<< " does not have a binding in the Root Signature"; | ||||||||||||||
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 void trackRootSigDescBinding(hlsl::BindingInfoBuilder &Builder, | ||||||||||||||
const mcdxbc::RootSignatureDesc &RSD, | ||||||||||||||
dxbc::ShaderVisibility Visibility) { | ||||||||||||||
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); | ||||||||||||||
Builder.trackBinding(dxil::ResourceClass::CBuffer, Const.RegisterSpace, | ||||||||||||||
Const.ShaderRegister, | ||||||||||||||
Const.ShaderRegister + Const.Num32BitValues, | ||||||||||||||
nullptr); | ||||||||||||||
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); | ||||||||||||||
Builder.trackBinding(ParameterToResourceClass(Type), Desc.RegisterSpace, | ||||||||||||||
Desc.ShaderRegister, Desc.ShaderRegister, nullptr); | ||||||||||||||
|
||||||||||||||
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) { | ||||||||||||||
Builder.trackBinding(RangeToResourceClass(Range.RangeType), | ||||||||||||||
Range.RegisterSpace, Range.BaseShaderRegister, | ||||||||||||||
Range.BaseShaderRegister + Range.NumDescriptors, | ||||||||||||||
nullptr); | ||||||||||||||
} | ||||||||||||||
break; | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
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 reportErrors(Module &M, DXILResourceMap &DRM, | ||||||||||||||
DXILResourceBindingInfo &DRBI) { | ||||||||||||||
DXILResourceBindingInfo &DRBI, | ||||||||||||||
RootSignatureBindingInfo &RSBI, | ||||||||||||||
dxil::ModuleMetadataInfo &MMI) { | ||||||||||||||
if (DRM.hasInvalidCounterDirection()) | ||||||||||||||
reportInvalidDirection(M, DRM); | ||||||||||||||
|
||||||||||||||
|
@@ -94,14 +228,44 @@ static void reportErrors(Module &M, DXILResourceMap &DRM, | |||||||||||||
|
||||||||||||||
assert(!DRBI.hasImplicitBinding() && "implicit bindings should be handled in " | ||||||||||||||
"DXILResourceImplicitBinding pass"); | ||||||||||||||
|
||||||||||||||
if (auto RSD = getRootSignature(RSBI, MMI)) { | ||||||||||||||
|
||||||||||||||
hlsl::BindingInfoBuilder Builder; | ||||||||||||||
dxbc::ShaderVisibility Visibility = tripleToVisibility(MMI.ShaderProfile); | ||||||||||||||
trackRootSigDescBinding(Builder, *RSD, Visibility); | ||||||||||||||
|
||||||||||||||
bool HasOverlap; | ||||||||||||||
hlsl::BindingInfo Info = Builder.calculateBindingInfo(HasOverlap); | ||||||||||||||
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. We don't seem to use 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. There is another PR that takes care of checking if there are overlapping ranges. The original implementation that order made sense, now, I might need to change the PRs orders, will fix that today. |
||||||||||||||
|
||||||||||||||
for (const auto &ResList : | ||||||||||||||
{std::make_pair(ResourceClass::SRV, DRM.srvs()), | ||||||||||||||
std::make_pair(ResourceClass::UAV, DRM.uavs()), | ||||||||||||||
std::make_pair(ResourceClass::CBuffer, DRM.cbuffers()), | ||||||||||||||
std::make_pair(ResourceClass::Sampler, DRM.samplers())}) { | ||||||||||||||
for (auto Res : ResList.second) { | ||||||||||||||
llvm::dxil::ResourceInfo::ResourceBinding ResBinding = Res.getBinding(); | ||||||||||||||
llvm::hlsl::BindingInfo::BindingRange ResRange( | ||||||||||||||
ResBinding.LowerBound, ResBinding.LowerBound + ResBinding.Size); | ||||||||||||||
|
||||||||||||||
auto IsBound = Info.isBound(ResList.first, ResBinding.Space, ResRange); | ||||||||||||||
if (!IsBound) { | ||||||||||||||
reportRegNotBound(M, ResList.first, ResBinding); | ||||||||||||||
} | ||||||||||||||
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. nit:
Suggested change
|
||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} // 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 +277,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 +294,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 +311,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) | ||||||||||||||
|
||||||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.