-
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 17 commits
c95ce68
af6aa53
29eb893
ed4c553
28fb609
403972d
0f0435d
e841a98
ae6d67a
41f32bd
6f3d019
971ad57
db73d71
3b04c2d
1ddffc3
db0008e
b4e5fb4
4a655a5
cc94561
98f48d2
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,14 @@ | ||
|
||
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 needs a file header. |
||
#include "llvm/Support/DXILABI.h" | ||
#include "llvm/BinaryFormat/DXContainer.h" | ||
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.
|
||
#include "llvm/Support/ErrorHandling.h" | ||
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 this is used |
||
#include "llvm/Support/ScopedPrinter.h" | ||
|
||
using namespace llvm; | ||
namespace llvm { | ||
namespace dxil { | ||
StringRef getResourceClassName(dxil::ResourceClass RC) { | ||
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. Please avoid namespace blocks like this in the cpp file and just qualify the names: StringRef dxil::getResourceClassName(dxil::ResourceClass RC) { Some rationale here: https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions |
||
return enumToStringRef(RC, dxbc::getResourceClasses()); | ||
} | ||
} // namespace dxil | ||
} // namespace llvm | ||
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. Missing newline at the end of the file. |
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; | ||||||||
} | ||||||||
} | ||||||||
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 |
||||||||
|
||||||||
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; | ||||||||
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.
|
||||||||
} | ||||||||
llvm_unreachable("Unconvertible RootParameterType"); | ||||||||
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. Would it be better to have a different |
||||||||
} | ||||||||
|
||||||||
static void reportInvalidDirection(Module &M, DXILResourceMap &DRM) { | ||||||||
for (const auto &UAV : DRM.uavs()) { | ||||||||
|
@@ -84,8 +115,126 @@ static void reportOverlappingBinding(Module &M, DXILResourceMap &DRM) { | |||||||
} | ||||||||
} | ||||||||
|
||||||||
static void | ||||||||
reportOverlappingRegisters(Module &M, | ||||||||
const llvm::hlsl::BindingInfoBuilder::Binding &R1, | ||||||||
const llvm::hlsl::BindingInfoBuilder::Binding &R2) { | ||||||||
SmallString<128> Message; | ||||||||
|
||||||||
raw_svector_ostream OS(Message); | ||||||||
OS << "resource " << getResourceClassName(R1.RC) << " (space=" << R1.Space | ||||||||
<< ", registers=[" << R1.LowerBound << ", " << R1.UpperBound | ||||||||
<< "]) overlaps with resource " << getResourceClassName(R2.RC) | ||||||||
<< " (space=" << R2.Space << ", registers=[" << R2.LowerBound << ", " | ||||||||
<< R2.UpperBound << "])"; | ||||||||
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); | ||||||||
SmallVector<char> IDs; | ||||||||
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. Are duplicates possible? Do we need to unique them at all? If not, then we probably don't need 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. Duplicates are possible, if we have a root signature like: 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 using the 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. Not sure if there is a standard, since this isn't used in a lot of places. But I tried to follow a similar pattern as this test:
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. Using However, there are two reasons just using
|
||||||||
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, | ||||||||
&IDs.emplace_back()); | ||||||||
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, &IDs.emplace_back()); | ||||||||
|
||||||||
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, | ||||||||
&IDs.emplace_back()); | ||||||||
} | ||||||||
break; | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
for (const dxbc::RTS0::v1::StaticSampler &S : RSD.StaticSamplers) | ||||||||
Builder.trackBinding(dxil::ResourceClass::Sampler, S.RegisterSpace, | ||||||||
S.ShaderRegister, S.ShaderRegister, | ||||||||
&IDs.emplace_back()); | ||||||||
|
||||||||
Builder.calculateBindingInfo( | ||||||||
[&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. 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 really need to clean up the places that make this assumption and grab the first entry of the entry point vector explicitly, either by wrapping it with an API with an explicit name or by more explicitly handling library profiles. That said, this is a pretty ubiquitous problem already I don't think we need to fix it in this PR. |
||||||||
} | ||||||||
|
||||||||
static void reportErrors(Module &M, DXILResourceMap &DRM, | ||||||||
DXILResourceBindingInfo &DRBI) { | ||||||||
DXILResourceBindingInfo &DRBI, | ||||||||
RootSignatureBindingInfo &RSBI, | ||||||||
dxil::ModuleMetadataInfo &MMI) { | ||||||||
if (DRM.hasInvalidCounterDirection()) | ||||||||
reportInvalidDirection(M, DRM); | ||||||||
|
||||||||
|
@@ -94,14 +243,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 +267,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 +284,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 +301,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) | ||||||||
|
||||||||
|
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 this really adds much value by itself, maybe we can move it to the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
; 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)) | ||
|
||
define void @CSMain() "hlsl.shader"="compute" { | ||
entry: | ||
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
.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 see, in that case I think we need to move the
getResourceClasses
from here toDXILABI
. Otherwise, we have a circular dependency betweenDXILABI
andBinaryFormat/DXContainer.h
.Either that or inline this function where it is used.
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 circular dependency shouldn't be an issue, since we use the
#ifndef
in all files.I will fix this, but in a follow-up PR, since this change is already out of scope for this patch.
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.
Here is the PR fixing it: #153490