-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[DirectX] Add Range Overlap validation #152229
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: main
Are you sure you want to change the base?
Changes from 9 commits
c95ce68
af6aa53
29eb893
ed4c553
28fb609
403972d
0f0435d
e841a98
ae6d67a
41f32bd
6f3d019
971ad57
db73d71
3b04c2d
1ddffc3
db0008e
b4e5fb4
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 |
---|---|---|
@@ -0,0 +1,22 @@ | ||
|
||
#include "llvm/Support/DXILABI.h" | ||
#include "llvm/Support/ErrorHandling.h" | ||
|
||
using namespace llvm; | ||
namespace llvm { | ||
namespace dxil { | ||
StringRef getResourceClassName(dxil::ResourceClass RC) { | ||
switch (RC) { | ||
case dxil::ResourceClass::SRV: | ||
return "SRV"; | ||
case dxil::ResourceClass::UAV: | ||
return "UAV"; | ||
case dxil::ResourceClass::CBuffer: | ||
return "CBuffer"; | ||
case dxil::ResourceClass::Sampler: | ||
return "Sampler"; | ||
} | ||
llvm_unreachable("Unhandled ResourceClass"); | ||
} | ||
} // namespace dxil | ||
} // 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" | ||
|
@@ -918,6 +919,7 @@ PreservedAnalyses DXILOpLowering::run(Module &M, ModuleAnalysisManager &MAM) { | |
PA.preserve<DXILResourceAnalysis>(); | ||
PA.preserve<DXILMetadataAnalysis>(); | ||
PA.preserve<ShaderFlagsAnalysis>(); | ||
PA.preserve<RootSignatureAnalysis>(); | ||
return PA; | ||
} | ||
|
||
|
@@ -945,6 +947,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. We'll need to preserve the root signature analysis in the new-PM version of the pass as well (that is, in |
||
} | ||
}; | ||
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" | ||||||||
|
@@ -17,13 +18,43 @@ | |||||||
#include "llvm/IR/IntrinsicsDirectX.h" | ||||||||
#include "llvm/IR/Module.h" | ||||||||
#include "llvm/InitializePasses.h" | ||||||||
#include "llvm/Support/DXILABI.h" | ||||||||
|
||||||||
#define DEBUG_TYPE "dxil-post-optimization-validation" | ||||||||
|
||||||||
using namespace llvm; | ||||||||
using namespace llvm::dxil; | ||||||||
|
||||||||
namespace { | ||||||||
static ResourceClass toResourceClass(dxbc::DescriptorRangeType RangeType) { | ||||||||
using namespace dxbc; | ||||||||
switch (RangeType) { | ||||||||
case DescriptorRangeType::SRV: | ||||||||
return ResourceClass::SRV; | ||||||||
case DescriptorRangeType::UAV: | ||||||||
return ResourceClass::UAV; | ||||||||
case DescriptorRangeType::CBV: | ||||||||
return ResourceClass::CBuffer; | ||||||||
case DescriptorRangeType::Sampler: | ||||||||
return ResourceClass::Sampler; | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
static ResourceClass toResourceClass(dxbc::RootParameterType Type) { | ||||||||
using namespace dxbc; | ||||||||
switch (Type) { | ||||||||
case RootParameterType::Constants32Bit: | ||||||||
return ResourceClass::CBuffer; | ||||||||
case RootParameterType::SRV: | ||||||||
return ResourceClass::SRV; | ||||||||
case RootParameterType::UAV: | ||||||||
return ResourceClass::UAV; | ||||||||
case RootParameterType::CBV: | ||||||||
return ResourceClass::CBuffer; | ||||||||
case dxbc::RootParameterType::DescriptorTable: | ||||||||
inbelic marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
break; | ||||||||
} | ||||||||
llvm_unreachable("Unconvertible RootParameterType"); | ||||||||
} | ||||||||
|
||||||||
static void reportInvalidDirection(Module &M, DXILResourceMap &DRM) { | ||||||||
for (const auto &UAV : DRM.uavs()) { | ||||||||
|
@@ -84,8 +115,123 @@ static void reportOverlappingBinding(Module &M, DXILResourceMap &DRM) { | |||||||
} | ||||||||
} | ||||||||
|
||||||||
static void reportOverlappingRegisters( | ||||||||
Module &M, const llvm::hlsl::BindingInfoBuilder::Binding &Reported, | ||||||||
const llvm::hlsl::BindingInfoBuilder::Binding &Overlaping) { | ||||||||
SmallString<128> Message; | ||||||||
raw_svector_ostream OS(Message); | ||||||||
OS << "register " << getResourceClassName(Reported.RC) | ||||||||
<< " (space=" << Reported.Space << ", register=" << Reported.LowerBound | ||||||||
<< ")" << " is overlapping with" << " register " | ||||||||
<< getResourceClassName(Overlaping.RC) << " (space=" << Overlaping.Space | ||||||||
<< ", register=" << Overlaping.LowerBound << ")" | ||||||||
<< ", verify your root signature definition."; | ||||||||
|
||||||||
M.getContext().diagnose(DiagnosticInfoGeneric(Message)); | ||||||||
} | ||||||||
|
||||||||
static dxbc::ShaderVisibility | ||||||||
tripleToVisibility(llvm::Triple::EnvironmentType ET) { | ||||||||
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 validateRootSignature(Module &M, | ||||||||
const mcdxbc::RootSignatureDesc &RSD, | ||||||||
dxil::ModuleMetadataInfo &MMI) { | ||||||||
|
||||||||
hlsl::BindingInfoBuilder Builder; | ||||||||
dxbc::ShaderVisibility Visibility = tripleToVisibility(MMI.ShaderProfile); | ||||||||
|
||||||||
for (const mcdxbc::RootParameterInfo &ParamInfo : RSD.ParametersContainer) { | ||||||||
dxbc::ShaderVisibility ParamVisibility = | ||||||||
static_cast<dxbc::ShaderVisibility>(ParamInfo.Header.ShaderVisibility); | ||||||||
if (ParamVisibility != dxbc::ShaderVisibility::All && | ||||||||
ParamVisibility != Visibility) | ||||||||
continue; | ||||||||
dxbc::RootParameterType ParamType = | ||||||||
static_cast<dxbc::RootParameterType>(ParamInfo.Header.ParameterType); | ||||||||
switch (ParamType) { | ||||||||
case dxbc::RootParameterType::Constants32Bit: { | ||||||||
dxbc::RTS0::v1::RootConstants Const = | ||||||||
RSD.ParametersContainer.getConstant(ParamInfo.Location); | ||||||||
Builder.trackBinding(dxil::ResourceClass::CBuffer, Const.RegisterSpace, | ||||||||
Const.ShaderRegister, Const.ShaderRegister, nullptr); | ||||||||
break; | ||||||||
} | ||||||||
|
||||||||
case dxbc::RootParameterType::SRV: | ||||||||
case dxbc::RootParameterType::UAV: | ||||||||
case dxbc::RootParameterType::CBV: { | ||||||||
dxbc::RTS0::v2::RootDescriptor Desc = | ||||||||
RSD.ParametersContainer.getRootDescriptor(ParamInfo.Location); | ||||||||
Builder.trackBinding(toResourceClass(static_cast<dxbc::RootParameterType>( | ||||||||
ParamInfo.Header.ParameterType)), | ||||||||
Desc.RegisterSpace, Desc.ShaderRegister, | ||||||||
Desc.ShaderRegister, nullptr); | ||||||||
|
||||||||
break; | ||||||||
} | ||||||||
case dxbc::RootParameterType::DescriptorTable: { | ||||||||
const mcdxbc::DescriptorTable &Table = | ||||||||
RSD.ParametersContainer.getDescriptorTable(ParamInfo.Location); | ||||||||
|
||||||||
for (const dxbc::RTS0::v2::DescriptorRange &Range : Table.Ranges) { | ||||||||
uint32_t UpperBound = | ||||||||
Range.NumDescriptors == ~0U | ||||||||
? Range.BaseShaderRegister | ||||||||
: Range.BaseShaderRegister + Range.NumDescriptors - 1; | ||||||||
Builder.trackBinding( | ||||||||
toResourceClass( | ||||||||
static_cast<dxbc::DescriptorRangeType>(Range.RangeType)), | ||||||||
Range.RegisterSpace, Range.BaseShaderRegister, UpperBound, nullptr); | ||||||||
} | ||||||||
break; | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
for (const dxbc::RTS0::v1::StaticSampler &S : RSD.StaticSamplers) | ||||||||
Builder.trackBinding(dxil::ResourceClass::Sampler, S.RegisterSpace, | ||||||||
S.ShaderRegister, S.ShaderRegister, nullptr); | ||||||||
inbelic marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
hlsl::BindingInfo Info = Builder.calculateBindingInfo( | ||||||||
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.
Suggested change
|
||||||||
[&M](const llvm::hlsl::BindingInfoBuilder &Builder, | ||||||||
const llvm::hlsl::BindingInfoBuilder::Binding &ReportedBinding) { | ||||||||
const llvm::hlsl::BindingInfoBuilder::Binding &Overlaping = | ||||||||
Builder.findOverlapping(ReportedBinding); | ||||||||
reportOverlappingRegisters(M, ReportedBinding, Overlaping); | ||||||||
}); | ||||||||
} | ||||||||
|
||||||||
static mcdxbc::RootSignatureDesc * | ||||||||
getRootSignature(RootSignatureBindingInfo &RSBI, | ||||||||
dxil::ModuleMetadataInfo &MMI) { | ||||||||
if (MMI.EntryPropertyVec.size() == 0) | ||||||||
return nullptr; | ||||||||
return RSBI.getDescForFunction(MMI.EntryPropertyVec[0].Entry); | ||||||||
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.
Suggested change
Is that true? 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, there are a lot of tests that compile without an entry point, mainly scenarios for libraries. 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. But if it isn't 0, is it then always 1? 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 don't think so, HLSL can have multiple entry point, for multiple shader stages. 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. In that case, what makes it always be the first one? 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. For non library shader compilation, only one entry point is expected. If we have more than one entry point, we are compiling a library shader. Library don't compile root signature to dxcontainer. |
||||||||
} | ||||||||
|
||||||||
static void reportErrors(Module &M, DXILResourceMap &DRM, | ||||||||
DXILResourceBindingInfo &DRBI) { | ||||||||
DXILResourceBindingInfo &DRBI, | ||||||||
RootSignatureBindingInfo &RSBI, | ||||||||
dxil::ModuleMetadataInfo &MMI) { | ||||||||
if (DRM.hasInvalidCounterDirection()) | ||||||||
reportInvalidDirection(M, DRM); | ||||||||
|
||||||||
|
@@ -94,14 +240,19 @@ static void reportErrors(Module &M, DXILResourceMap &DRM, | |||||||
|
||||||||
assert(!DRBI.hasImplicitBinding() && "implicit bindings should be handled in " | ||||||||
"DXILResourceImplicitBinding pass"); | ||||||||
|
||||||||
if (mcdxbc::RootSignatureDesc *RSD = getRootSignature(RSBI, MMI)) | ||||||||
validateRootSignature(M, *RSD, MMI); | ||||||||
} | ||||||||
} // 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 +264,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 +281,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>(); | ||||||||
inbelic marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
} | ||||||||
}; | ||||||||
char DXILPostOptimizationValidationLegacy::ID = 0; | ||||||||
|
@@ -139,6 +298,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) | ||||||||
INITIALIZE_PASS_END(DXILPostOptimizationValidationLegacy, DEBUG_TYPE, | ||||||||
"DXIL Post Optimization Validation", false, false) | ||||||||
|
||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,22 @@ | ||||||||||
; RUN: opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | ||||||||||
; expected-no-diagnostics | ||||||||||
; Root Signature(RootConstants(num32BitConstants=4, b2)) | ||||||||||
|
||||||||||
%__cblayout_CB = type <{ float }> | ||||||||||
|
||||||||||
@CB.str = private unnamed_addr constant [3 x i8] c"CB\00", align 1 | ||||||||||
|
||||||||||
define void @CSMain() "hlsl.shader"="compute" { | ||||||||||
entry: | ||||||||||
; cbuffer CB : register(b2, space0) { | ||||||||||
; float a; | ||||||||||
; } | ||||||||||
%CB = tail call target("dx.CBuffer", target("dx.Layout", %__cblayout_CB, 4, 0)) @llvm.dx.resource.handlefrombinding(i32 0, i32 2, i32 1, i32 0, i1 false, ptr nonnull @CB.str) | ||||||||||
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.
Suggested change
The other ones don't have any resources defined |
||||||||||
ret void | ||||||||||
} | ||||||||||
|
||||||||||
!dx.rootsignatures = !{!0} | ||||||||||
|
||||||||||
!0 = !{ptr @CSMain, !1, i32 2} | ||||||||||
!1 = !{!2} | ||||||||||
!2 = !{!"RootConstants", i32 0, i32 2, i32 0, i32 4} |
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.
You should be able to use the recently landed function defined here in tangent with
enumToStringRef
: #152213There 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.
Bump
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.
Check
llvm/lib/Support/DXILABI.cpp
,getResourceClassName
callsenumToStringRef
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.
Ah okay, I think we could probably just do that inline? Then we don't need to define
DXILABI.cpp
at all, since this file is defined to just contain "definitions of constants and enums" to be stable in the DXILThere 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.
Please check #152229 (comment), bogner explains why we created
DXILABI.cpp
.