-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[DirectX] Validate if Textures/TypedBuffers are being bound in Root Signatures #147573
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
Open
joaosaffran
wants to merge
18
commits into
llvm:users/joaosaffran/146785
Choose a base branch
from
joaosaffran:validation/textures-not-bind-root-signatures
base: users/joaosaffran/146785
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
01a558b
add validation
43eb04e
adding validation
f67bec1
Merge branch 'validation/check-descriptors-are-bound' into validation…
d7b4cf4
format
403b546
Merge branch 'validation/check-descriptors-are-bound' into validation…
aea75da
fix test
5af9199
update
46294cb
Merge branch 'validation/check-descriptors-are-bound' into validation…
91ff1bf
clean up
5f8bbeb
Merge branch 'validation/check-descriptors-are-bound' into validation…
8459d14
Merge branch 'validation/check-descriptors-are-bound' into validation…
34deb3a
Merge branch 'validation/check-descriptors-are-bound' into validation…
49f3bf2
Merge branch 'validation/check-descriptors-are-bound' into validation…
848501d
address comments
8d97116
format test
9c34f3f
fix test
24040a0
Merge branch 'validation/check-descriptors-are-bound' into validation…
2c30cd9
addressing comments
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,8 @@ | |
#include "DXILRootSignature.h" | ||
#include "DXILShaderFlags.h" | ||
#include "DirectX.h" | ||
#include "llvm/ADT/ArrayRef.h" | ||
#include "llvm/ADT/STLForwardCompat.h" | ||
#include "llvm/ADT/SmallString.h" | ||
#include "llvm/Analysis/DXILMetadataAnalysis.h" | ||
#include "llvm/Analysis/DXILResource.h" | ||
|
@@ -128,6 +130,17 @@ static void reportOverlappingBinding(Module &M, DXILResourceMap &DRM) { | |
} | ||
} | ||
|
||
static void | ||
reportInvalidHandleTyBoundInRs(Module &M, Twine Type, | ||
ResourceInfo::ResourceBinding Binding) { | ||
SmallString<128> Message; | ||
raw_svector_ostream OS(Message); | ||
OS << "resource " << Type << " at register (space=" << Binding.Space | ||
<< ", register=" << Binding.LowerBound << ")" | ||
<< " is bound to a texture or typed buffer."; | ||
M.getContext().diagnose(DiagnosticInfoGeneric(Message)); | ||
} | ||
|
||
static void reportRegNotBound(Module &M, | ||
llvm::hlsl::rootsig::RangeInfo Unbound) { | ||
SmallString<128> Message; | ||
|
@@ -232,6 +245,44 @@ initRSBindingValidation(const mcdxbc::RootSignatureDesc &RSD, | |
return Validation; | ||
} | ||
|
||
static SmallVector<ResourceInfo::ResourceBinding> | ||
getRootDescriptorsBindingInfo(const mcdxbc::RootSignatureDesc &RSD, | ||
dxbc::ShaderVisibility Visibility) { | ||
|
||
SmallVector<ResourceInfo::ResourceBinding> RDs; | ||
|
||
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::SRV): | ||
case llvm::to_underlying(dxbc::RootParameterType::UAV): | ||
case llvm::to_underlying(dxbc::RootParameterType::CBV): { | ||
dxbc::RTS0::v2::RootDescriptor Desc = | ||
RSD.ParametersContainer.getRootDescriptor(Loc); | ||
|
||
ResourceInfo::ResourceBinding Binding; | ||
Binding.LowerBound = Desc.ShaderRegister; | ||
Binding.Space = Desc.RegisterSpace; | ||
Binding.Size = 1; | ||
|
||
RDs.push_back(Binding); | ||
break; | ||
} | ||
} | ||
} | ||
|
||
return RDs; | ||
} | ||
|
||
std::optional<mcdxbc::RootSignatureDesc> | ||
getRootSignature(RootSignatureBindingInfo &RSBI, | ||
dxil::ModuleMetadataInfo &MMI) { | ||
|
@@ -244,6 +295,25 @@ getRootSignature(RootSignatureBindingInfo &RSBI, | |
return RootSigDesc; | ||
} | ||
|
||
static void reportInvalidHandleTy( | ||
Module &M, const llvm::ArrayRef<dxil::ResourceInfo::ResourceBinding> &RDs, | ||
const iterator_range<SmallVectorImpl<dxil::ResourceInfo>::iterator> | ||
inbelic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
&Resources) { | ||
for (auto Res = Resources.begin(), End = Resources.end(); Res != End; Res++) { | ||
llvm::dxil::ResourceInfo::ResourceBinding Binding = Res->getBinding(); | ||
for (const auto &RD : RDs) { | ||
if (Binding.overlapsWith(RD)) { | ||
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: you could probably move the error reporting to be inside this if statement and then you don't need the |
||
TargetExtType *Handle = Res->getHandleTy(); | ||
auto *TypedBuffer = dyn_cast_or_null<TypedBufferExtType>(Handle); | ||
auto *Texture = dyn_cast_or_null<TextureExtType>(Handle); | ||
|
||
if (TypedBuffer != nullptr || Texture != nullptr) | ||
reportInvalidHandleTyBoundInRs(M, Res->getName(), Res->getBinding()); | ||
} | ||
} | ||
} | ||
} | ||
|
||
static void reportUnboundRegisters( | ||
Module &M, | ||
const llvm::hlsl::rootsig::RootSignatureBindingValidation &Validation, | ||
|
@@ -282,16 +352,24 @@ static void reportErrors(Module &M, DXILResourceMap &DRM, | |
"DXILResourceImplicitBinding pass"); | ||
|
||
if (auto RSD = getRootSignature(RSBI, MMI)) { | ||
|
||
dxbc::ShaderVisibility Visibility = tripleToVisibility(MMI.ShaderProfile); | ||
llvm::hlsl::rootsig::RootSignatureBindingValidation Validation = | ||
initRSBindingValidation(*RSD, tripleToVisibility(MMI.ShaderProfile)); | ||
initRSBindingValidation(*RSD, Visibility); | ||
|
||
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()); | ||
|
||
SmallVector<ResourceInfo::ResourceBinding> RDs = | ||
getRootDescriptorsBindingInfo(*RSD, Visibility); | ||
|
||
reportInvalidHandleTy(M, RDs, DRM.cbuffers()); | ||
reportInvalidHandleTy(M, RDs, DRM.srvs()); | ||
reportInvalidHandleTy(M, RDs, DRM.uavs()); | ||
reportInvalidHandleTy(M, RDs, DRM.samplers()); | ||
} | ||
} | ||
} // namespace | ||
|
22 changes: 22 additions & 0 deletions
22
llvm/test/CodeGen/DirectX/rootsignature-validation-textures-fail.ll
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
; RUN: not opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | FileCheck %s | ||
; CHECK: error: resource TB at register (space=0, register=0) is bound to a texture or typed buffer. | ||
|
||
|
||
; Root Signature( | ||
; UAV(b0, space=0, visibility=SHADER_VISIBILITY_ALL) | ||
|
||
@TB.str = private unnamed_addr constant [3 x i8] c"TB\00", align 1 | ||
|
||
define void @CSMain() "hlsl.shader"="compute" { | ||
entry: | ||
|
||
%TB = tail call target("dx.Texture", float, 1, 0, 0) @llvm.dx.resource.handlefrombinding.tdx.TypedBuffer_f32_1_0_0t(i32 0, i32 0, i32 1, i32 0, i1 false, ptr nonnull @TB.str) | ||
|
||
ret void | ||
} | ||
|
||
!dx.rootsignatures = !{!0} | ||
|
||
!0 = !{ptr @CSMain, !1, i32 2} | ||
!1 = !{!2} | ||
!2 = !{!"RootUAV", i32 0, i32 0, i32 0, i32 4} |
21 changes: 21 additions & 0 deletions
21
llvm/test/CodeGen/DirectX/rootsignature-validation-textures.ll
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
; RUN: opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | ||
; expected-no-diagnostics | ||
; Root Signature( | ||
; DescriptorTable(UAV(b0, space=0, visibility=SHADER_VISIBILITY_ALL)) | ||
|
||
@TB.str = private unnamed_addr constant [3 x i8] c"TB\00", align 1 | ||
|
||
define void @CSMain() "hlsl.shader"="compute" { | ||
entry: | ||
|
||
%TB = tail call target("dx.Texture", float, 1, 0, 0) @llvm.dx.resource.handlefrombinding.tdx.TypedBuffer_f32_1_0_0t(i32 0, i32 0, i32 1, i32 0, i1 false, ptr nonnull @TB.str) | ||
|
||
ret void | ||
} | ||
|
||
!dx.rootsignatures = !{!0} | ||
|
||
!0 = !{ptr @CSMain, !1, i32 2} | ||
!1 = !{!3} | ||
!3 = !{!"DescriptorTable", i32 0, !4} | ||
!4 = !{!"UAV", i32 1, i32 0, i32 0, i32 -1, i32 0} |
25 changes: 25 additions & 0 deletions
25
llvm/test/CodeGen/DirectX/rootsignature-validation-typedbuffer-fail.ll
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
; RUN: not opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | FileCheck %s | ||
; CHECK: error: resource TB at register (space=0, register=0) is bound to a texture or typed buffer. | ||
|
||
|
||
; Root Signature( | ||
; CBV(b3, space=1, visibility=SHADER_VISIBILITY_ALL) | ||
; DescriptorTable(SRV(t0, space=0, numDescriptors=1), visibility=SHADER_VISIBILITY_ALL) | ||
; DescriptorTable(Sampler(s0, numDescriptors=2), visibility=SHADER_VISIBILITY_VERTEX) | ||
; DescriptorTable(UAV(u0, numDescriptors=unbounded), visibility=SHADER_VISIBILITY_ALL) | ||
|
||
@TB.str = private unnamed_addr constant [3 x i8] c"TB\00", align 1 | ||
|
||
define void @CSMain() "hlsl.shader"="compute" { | ||
entry: | ||
|
||
%TB = tail call target("dx.TypedBuffer", float, 1, 0, 0) @llvm.dx.resource.handlefrombinding.tdx.TypedBuffer_f32_1_0_0t(i32 0, i32 0, i32 1, i32 0, i1 false, ptr nonnull @TB.str) | ||
|
||
ret void | ||
} | ||
|
||
!dx.rootsignatures = !{!0} | ||
|
||
!0 = !{ptr @CSMain, !1, i32 2} | ||
!1 = !{!2} | ||
!2 = !{!"RootUAV", i32 0, i32 0, i32 0, i32 4} |
21 changes: 21 additions & 0 deletions
21
llvm/test/CodeGen/DirectX/rootsignature-validation-typedbuffer.ll
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
; RUN: opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | ||
; expected-no-diagnostics | ||
; Root Signature( | ||
; DescriptorTable(UAV(b0, space=0, visibility=SHADER_VISIBILITY_ALL)) | ||
|
||
@TB.str = private unnamed_addr constant [3 x i8] c"TB\00", align 1 | ||
|
||
define void @CSMain() "hlsl.shader"="compute" { | ||
entry: | ||
|
||
%TB = tail call target("dx.TypedBuffer", float, 1, 0, 0) @llvm.dx.resource.handlefrombinding.tdx.TypedBuffer_f32_1_0_0t(i32 0, i32 0, i32 1, i32 0, i1 false, ptr nonnull @TB.str) | ||
|
||
ret void | ||
} | ||
|
||
!dx.rootsignatures = !{!0} | ||
|
||
!0 = !{ptr @CSMain, !1, i32 2} | ||
!1 = !{!3} | ||
!3 = !{!"DescriptorTable", i32 0, !4} | ||
!4 = !{!"UAV", i32 1, i32 0, i32 0, i32 -1, i32 0} |
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
Oops, something went wrong.
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.
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 would have thought an if condition would be more expected control flow here. Do you think this switch statement will need to possibly be extended later?
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.
Not really, unless DirectX includes a new Root Element Type in Root Signatures, this shouldn't change