-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[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
Changes from 5 commits
c95ce68
af6aa53
29eb893
ed4c553
28fb609
403972d
0f0435d
e841a98
ae6d67a
41f32bd
6f3d019
971ad57
db73d71
3b04c2d
1ddffc3
db0008e
b4e5fb4
4a655a5
cc94561
98f48d2
eb334b8
06c0da4
d58606f
74980c8
d376abf
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 |
---|---|---|
|
@@ -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. 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" | ||||||||||||||||
|
@@ -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)) { | ||||||||||||||||
|
||||||||||||||||
case DescriptorRangeType::SRV: | ||||||||||||||||
return ResourceClass::SRV; | ||||||||||||||||
case DescriptorRangeType::UAV: | ||||||||||||||||
return ResourceClass::UAV; | ||||||||||||||||
case DescriptorRangeType::CBV: | ||||||||||||||||
return ResourceClass::CBuffer; | ||||||||||||||||
case DescriptorRangeType::Sampler: | ||||||||||||||||
return ResourceClass::Sampler; | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
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. You'll need an |
||||||||||||||||
|
||||||||||||||||
ResourceClass ParameterToResourceClass(uint32_t Type) { | ||||||||||||||||
|
||||||||||||||||
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,122 @@ 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) { | ||||||||||||||||
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): { | ||||||||||||||||
|
||||||||||||||||
dxbc::RTS0::v1::RootConstants Const = | ||||||||||||||||
RSD.ParametersContainer.getConstant(Loc); | ||||||||||||||||
Builder.trackBinding(dxil::ResourceClass::CBuffer, Const.RegisterSpace, | ||||||||||||||||
Const.ShaderRegister, | ||||||||||||||||
Const.ShaderRegister + Const.Num32BitValues, &Const); | ||||||||||||||||
|
Const.ShaderRegister + Const.Num32BitValues, &Const); | |
Const.ShaderRegister, &Const); |
This is what we do in the frontend, and iirc, I did check the behaviour with DXC. In either case, can you double-check what the correct behaviour is. But I believe it will always have a size of 1.
Outdated
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.
&Const
is a reference to a local here - this is undefined behaviour. If we need a cookie to deduplicate these bindings we'll need something that has a sufficient lifetime. Same with the following calls to trackBinding
Outdated
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.BaseShaderRegister + | |
Range.NumDescriptors, | |
: Range.BaseShaderRegister + | |
Range.NumDescriptors - 1, |
I believe this is an inclusive range. We should add a test case for this 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.
Probably more readable to introduce a UpperBound
variable rather than bury the ternary operator in the function call as well.
Outdated
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.
It's best to only use auto when the type is blatantly obvious like a cast, or sometimes in cases like iterators where the type name is long enough to obscure what's happening. Please spell out the type here. Also, should this be const
?
Outdated
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:
for (auto &S : RSD.StaticSamplers) { | |
Builder.trackBinding(dxil::ResourceClass::Sampler, S.RegisterSpace, | |
S.ShaderRegister, S.ShaderRegister, &S); | |
} | |
for (auto &S : RSD.StaticSamplers) | |
Builder.trackBinding(dxil::ResourceClass::Sampler, S.RegisterSpace, | |
S.ShaderRegister, S.ShaderRegister, &S); |
Outdated
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'm a bit surprised / worried that this API is returning a std::optional<mcdxbc::RootSignatureDesc>
and not a std::optional<mcdxbc::RootSignatureDesc &>
- the root signature description object doesn't look cheap to copy (it has multiple vectors in it!) and here we are copying it twice...
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.
std::optional
don't seem to support reference, used a pointer instead.
inbelic marked this conversation as resolved.
Show resolved
Hide resolved
|
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) | ||
inbelic marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
ret void | ||
} | ||
|
||
!dx.rootsignatures = !{!0} | ||
|
||
!0 = !{ptr @CSMain, !1, i32 2} | ||
!1 = !{!2} | ||
!2 = !{!"RootConstants", i32 0, i32 2, i32 0, i32 4} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
; RUN: not opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | FileCheck %s | ||
; CHECK: error: register CBuffer (space=0, register=2) is overlapping with register CBuffer (space=0, register=0), verify your root signature definition. | ||
|
||
define void @CSMain() "hlsl.shader"="compute" { | ||
entry: | ||
ret void | ||
} | ||
|
||
; RootConstants(num32BitConstants=4, b2), DescriptorTable(CBV(b0, numDescriptors=3)) | ||
!dx.rootsignatures = !{!0} | ||
!0 = !{ptr @CSMain, !1, i32 2} | ||
!1 = !{!2, !3} | ||
!2 = !{!"RootConstants", i32 0, i32 2, i32 0, i32 4} | ||
!3 = !{!"DescriptorTable", i32 0, !4} | ||
!4 = !{!"CBV", i32 3, i32 0, i32 0, i32 -1, i32 4} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
; RUN: not opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | FileCheck %s | ||
; CHECK: error: register UAV (space=10, register=4294967295) is overlapping with register UAV (space=10, register=4294967295), verify your root signature definition. | ||
define void @CSMain() "hlsl.shader"="compute" { | ||
entry: | ||
ret void | ||
} | ||
|
||
; DescriptorTable(UAV(u0, numDescriptors=unbounded), visibility = SHADER_VISIBILITY_HULL), DescriptorTable(UAV(u2, numDescriptors=4)) | ||
!dx.rootsignatures = !{!0} | ||
!0 = !{ptr @CSMain, !1, i32 2} | ||
!1 = !{!2, !4} | ||
!2 = !{!"DescriptorTable", i32 0, !3} | ||
!3 = !{!"UAV", i32 -1, i32 -1, i32 10, i32 -1, i32 2} | ||
!4 = !{!"DescriptorTable", i32 0, !5} | ||
!5 = !{ !"UAV", i32 -1, i32 -1, i32 10, i32 5, i32 2 } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
; RUN: not opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | FileCheck %s | ||
; CHECK: error: register UAV (space=1, register=3) is overlapping with register UAV (space=1, register=0), verify your root signature definition. | ||
|
||
|
||
define void @CSMain() "hlsl.shader"="compute" { | ||
entry: | ||
ret void | ||
} | ||
|
||
; DescriptorTable(UAV(u0, numDescriptors=unbounded), visibility = SHADER_VISIBILITY_HULL), UAV(u3, space=1) | ||
!dx.rootsignatures = !{!0} | ||
!0 = !{ptr @CSMain, !1, i32 2} | ||
!1 = !{!2, !4} | ||
!2 = !{!"RootUAV", i32 0, i32 3, i32 1, i32 4} | ||
!4 = !{!"DescriptorTable", i32 0, !5} | ||
!5 = !{!"UAV", i32 3, i32 0, i32 1, i32 -1, i32 2} |
inbelic marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
; RUN: not opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | FileCheck %s | ||
; CHECK: error: register Sampler (space=0, register=42) is overlapping with register Sampler (space=0, register=42), verify your root signature definition. | ||
|
||
define void @CSMain() "hlsl.shader"="compute" { | ||
entry: | ||
ret void | ||
} | ||
|
||
!dx.rootsignatures = !{!0} | ||
!0 = !{ptr @CSMain, !1, i32 2} | ||
!1 = !{!2, !3} | ||
!2 = !{ !"StaticSampler", i32 5, i32 4, i32 5, i32 3, float 0x3FF7CCCCC0000000, i32 10, i32 2, i32 1, float -1.270000e+02, float 1.220000e+02, i32 42, i32 0, i32 0 } | ||
!3 = !{ !"StaticSampler", i32 4, i32 2, i32 3, i32 5, float 0x3FF6CCCCC0000000, i32 9, i32 3, i32 2, float -1.280000e+02, float 1.280000e+02, i32 42, i32 0, i32 0 } |
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 isn't an appropriate place for this function - the enum is defined in
Support/DXILABI.h
and this is just an arbitrary user of that that happens to also be a dependency of where you're using this functionality. Also, it's not a "great" practice to have a bunch of string literals in an inline function in a header - these will end up as duplicate data in every translation unit and we have to rely on the linker to clean it all up later.I guess it would probably be best to add a
DXILABI.cpp
in Support and out-of-line the resource class enum stringify-ing function there.