Skip to content

[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

Open
wants to merge 50 commits into
base: users/joaosaffran/152229
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
0e8828c
refactoring
Jun 26, 2025
2edd215
refactoring
Jun 26, 2025
242545e
clean up
Jul 2, 2025
3f8dec4
format
Jul 2, 2025
3b1ce3b
formating
Jul 2, 2025
f5720af
fix import issues
Jul 4, 2025
ea54904
formating
Jul 4, 2025
a49aa19
refactoring
Jun 26, 2025
d90676f
init refactoring
Jun 26, 2025
a04eb9f
adding validation
Jul 2, 2025
5994b8f
clean
Jul 2, 2025
e8b14bf
implementing
Jul 4, 2025
8f40e83
finish implementing && fix tests
Jul 4, 2025
28350b2
fix issue
Jul 5, 2025
4fd2e0b
sync parent
Jul 5, 2025
e25ee87
sync parent
Jul 5, 2025
881dd36
address comments
Jul 7, 2025
8779ee9
Merge branch 'refactoring/updating-return-root-sig-analysis' into val…
Jul 7, 2025
c16f15b
fix test
Jul 8, 2025
c7d5be7
format
Jul 8, 2025
cc5afae
address changes
Jul 8, 2025
571a0ef
fix tests
Jul 8, 2025
974d4bc
Merge branch 'refactoring/updating-return-root-sig-analysis' into val…
Jul 8, 2025
e0bc862
add preserved
Jul 8, 2025
b5a0b32
addressing comments
Jul 10, 2025
00a74af
Merge branch 'main' into validation/check-descriptors-are-bound
Jul 10, 2025
5ccb842
updating
Jul 11, 2025
5423aba
format
Jul 14, 2025
a7637a7
adding tests
Jul 14, 2025
da42c0c
clean up
Jul 14, 2025
edb015d
address comments
Jul 15, 2025
9f3888e
adding root constants
Jul 15, 2025
578a03b
clean
Jul 15, 2025
b4a0e16
moving code arround
Jul 17, 2025
ef14638
clean
Jul 17, 2025
662c3a8
addressing comments
Jul 21, 2025
260633c
address comments
Jul 25, 2025
d42f156
Merge branch 'main' into validation/check-descriptors-are-bound
Jul 31, 2025
9ee3a4b
Merge branch 'main' into validation/check-descriptors-are-bound
Jul 31, 2025
6db6224
update code
Aug 1, 2025
04658b8
cleanup
Aug 1, 2025
adf3feb
address comments from inbelic
Aug 5, 2025
fc338b5
Merge branch 'validation/overlapping-ranges' into validation/check-de…
Aug 6, 2025
f5b5b3e
clean
Aug 6, 2025
03d571a
clean?
Aug 6, 2025
ef51048
format
Aug 6, 2025
21675e6
formating
Aug 6, 2025
47662f0
Merge branch 'validation/overlapping-ranges' into validation/check-de…
Aug 8, 2025
6da5fb0
format
Aug 8, 2025
0c72dcf
addressing inbelic comments
Aug 12, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions llvm/include/llvm/Frontend/HLSL/HLSLBinding.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/DXILABI.h"
#include "llvm/Support/ErrorHandling.h"
#include <cstdint>

namespace llvm {
namespace hlsl {
Expand Down Expand Up @@ -57,6 +58,7 @@ class BindingInfo {
}
// Size == -1 means unbounded array
LLVM_ABI std::optional<uint32_t> findAvailableBinding(int32_t Size);
LLVM_ABI bool isBound(BindingRange B);
};

struct BindingSpaces {
Expand Down Expand Up @@ -95,6 +97,8 @@ class BindingInfo {
LLVM_ABI std::optional<uint32_t>
findAvailableBinding(dxil::ResourceClass RC, uint32_t Space, int32_t Size);

LLVM_ABI bool isBound(dxil::ResourceClass RC, uint32_t Space, BindingRange B);

friend class BindingInfoBuilder;
};

Expand Down
45 changes: 0 additions & 45 deletions llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h
Original file line number Diff line number Diff line change
Expand Up @@ -148,51 +148,6 @@ struct OverlappingRanges {
/// B: Check for overlap with any overlapping Visibility ResourceRange
LLVM_ABI llvm::SmallVector<OverlappingRanges>
findOverlappingRanges(ArrayRef<RangeInfo> Infos);

class RootSignatureBindingValidation {
private:
llvm::SmallVector<RangeInfo, 16> Bindings;
struct TypeRange {
uint32_t Start;
uint32_t End;
};
std::unordered_map<dxil::ResourceClass, TypeRange> Ranges;

public:
void addBinding(dxil::ResourceClass Type, const RangeInfo &Binding) {
auto It = Ranges.find(Type);

if (It == Ranges.end()) {
uint32_t InsertPos = Bindings.size();
Bindings.push_back(Binding);
Ranges[Type] = {InsertPos, InsertPos + 1};
return;
}
uint32_t InsertPos = It->second.End;
Bindings.insert(Bindings.begin() + InsertPos, Binding);

It->second.End++;

for (auto &[Type, Range] : Ranges) {
if (Range.Start > InsertPos) {
Range.Start++;
Range.End++;
}
}
}

llvm::ArrayRef<RangeInfo>
getBindingsOfType(const dxil::ResourceClass &Type) const {
auto It = Ranges.find(Type);
if (It == Ranges.end())
return {};
return llvm::ArrayRef<RangeInfo>(Bindings.data() + It->second.Start,
It->second.End - It->second.Start);
}
};
llvm::SmallVector<RangeInfo>
findUnboundRanges(const llvm::ArrayRef<RangeInfo> &Ranges,
const llvm::ArrayRef<RangeInfo> &Bindings);
} // namespace rootsig
} // namespace hlsl
} // namespace llvm
Expand Down
17 changes: 17 additions & 0 deletions llvm/lib/Frontend/HLSL/HLSLBinding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "llvm/Frontend/HLSL/HLSLBinding.h"
#include "llvm/ADT/STLExtras.h"
#include <cstdint>

using namespace llvm;
using namespace hlsl;
Expand Down Expand Up @@ -66,6 +67,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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can take advantage of FreeRanges being sorted and use llvm::lower_bound for this check

return true;
}

bool BindingInfo::isBound(dxil::ResourceClass RC, uint32_t Space,
BindingRange B) {
BindingSpaces &BS = getBindingSpaces(RC);
RegisterSpace &RS = BS.getOrInsertSpace(Space);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth it to add a getSpace that returns an optional if it doesn't exist? It would mean we don't create a bunch of extra RegisteSpaces that need to be traversed on each subsequent getBindingSpaces

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bogner designed this class.
@bogner: thoughts?

return RS.isBound(B);
}

BindingInfo BindingInfoBuilder::calculateBindingInfo(
llvm::function_ref<void(const BindingInfoBuilder &Builder,
const Binding &Overlapping)>
Expand Down
108 changes: 44 additions & 64 deletions llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Analysis/DXILMetadataAnalysis.h"
#include "llvm/Analysis/DXILResource.h"
#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/Frontend/HLSL/HLSLBinding.h"
#include "llvm/Frontend/HLSL/RootSignatureValidations.h"
#include "llvm/IR/DiagnosticInfo.h"
#include "llvm/IR/Instructions.h"
#include "llvm/IR/IntrinsicsDirectX.h"
#include "llvm/IR/Module.h"
#include "llvm/InitializePasses.h"
#include "llvm/Support/DXILABI.h"
#include <utility>

#define DEBUG_TYPE "dxil-post-optimization-validation"

Expand Down Expand Up @@ -116,11 +120,12 @@ static void reportOverlappingBinding(Module &M, DXILResourceMap &DRM) {
}
}

static void reportRegNotBound(Module &M,
llvm::hlsl::rootsig::RangeInfo Unbound) {
static void
reportRegNotBound(Module &M, ResourceClass Class,
llvm::dxil::ResourceInfo::ResourceBinding Unbound) {
SmallString<128> Message;
raw_svector_ostream OS(Message);
OS << "register " << getResourceClassName(Unbound.Class)
OS << "register " << getResourceClassName(Class)
<< " (space=" << Unbound.Space << ", register=" << Unbound.LowerBound
<< ")"
<< " does not have a binding in the Root Signature";
Expand Down Expand Up @@ -155,12 +160,9 @@ tripleToVisibility(llvm::Triple::EnvironmentType ET) {
}
}

static hlsl::rootsig::RootSignatureBindingValidation
initRSBindingValidation(const mcdxbc::RootSignatureDesc &RSD,
dxbc::ShaderVisibility Visibility) {

hlsl::rootsig::RootSignatureBindingValidation Validation;

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);
Expand All @@ -175,15 +177,10 @@ initRSBindingValidation(const mcdxbc::RootSignatureDesc &RSD,
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using llvm::to_underlying on all the cases, why not convert the int to enum and handle the potential error outside the switch? That will make the code easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);

hlsl::rootsig::RangeInfo Binding;
Binding.LowerBound = Const.ShaderRegister;
Binding.Space = Const.RegisterSpace;
Binding.UpperBound = Binding.LowerBound;

// Root Constants Bind to CBuffers
Validation.addBinding(ResourceClass::CBuffer, Binding);

Builder.trackBinding(dxil::ResourceClass::CBuffer, Const.RegisterSpace,
Const.ShaderRegister,
Const.ShaderRegister + Const.Num32BitValues,
nullptr);
break;
}

Expand All @@ -192,32 +189,25 @@ initRSBindingValidation(const mcdxbc::RootSignatureDesc &RSD,
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);

hlsl::rootsig::RangeInfo Binding;
Binding.LowerBound = Desc.ShaderRegister;
Binding.Space = Desc.RegisterSpace;
Binding.UpperBound = Binding.LowerBound;

Validation.addBinding(ParameterToResourceClass(Type), Binding);
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) {
hlsl::rootsig::RangeInfo Binding;
Binding.LowerBound = Range.BaseShaderRegister;
Binding.Space = Range.RegisterSpace;
Binding.UpperBound = Binding.LowerBound + Range.NumDescriptors - 1;
Validation.addBinding(RangeToResourceClass(Range.RangeType), Binding);
Builder.trackBinding(RangeToResourceClass(Range.RangeType),
Range.RegisterSpace, Range.BaseShaderRegister,
Range.BaseShaderRegister + Range.NumDescriptors,
nullptr);
}
break;
}
}
}

return Validation;
}

std::optional<mcdxbc::RootSignatureDesc>
Expand All @@ -232,30 +222,6 @@ getRootSignature(RootSignatureBindingInfo &RSBI,
return RootSigDesc;
}

static void reportUnboundRegisters(
Module &M,
const llvm::hlsl::rootsig::RootSignatureBindingValidation &Validation,
ResourceClass Class,
const iterator_range<SmallVectorImpl<dxil::ResourceInfo>::iterator>
&Resources) {
SmallVector<hlsl::rootsig::RangeInfo> Ranges;
for (auto Res = Resources.begin(), End = Resources.end(); Res != End; Res++) {
ResourceInfo::ResourceBinding ResBinding = Res->getBinding();
hlsl::rootsig::RangeInfo Range;
Range.Space = ResBinding.Space;
Range.LowerBound = ResBinding.LowerBound;
Range.UpperBound = Range.LowerBound + ResBinding.Size - 1;
Range.Class = Class;
Ranges.push_back(Range);
}

SmallVector<hlsl::rootsig::RangeInfo> Unbounds =
hlsl::rootsig::findUnboundRanges(Ranges,
Validation.getBindingsOfType(Class));
for (const auto &Unbound : Unbounds)
reportRegNotBound(M, Unbound);
}

static void reportErrors(Module &M, DXILResourceMap &DRM,
DXILResourceBindingInfo &DRBI,
RootSignatureBindingInfo &RSBI,
Expand All @@ -271,15 +237,29 @@ static void reportErrors(Module &M, DXILResourceMap &DRM,

if (auto RSD = getRootSignature(RSBI, MMI)) {

llvm::hlsl::rootsig::RootSignatureBindingValidation Validation =
initRSBindingValidation(*RSD, tripleToVisibility(MMI.ShaderProfile));

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());
hlsl::BindingInfoBuilder Builder;
dxbc::ShaderVisibility Visibility = tripleToVisibility(MMI.ShaderProfile);
trackRootSigDescBinding(Builder, *RSD, Visibility);

bool HasOverlap;
hlsl::BindingInfo Info = Builder.calculateBindingInfo(HasOverlap);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't seem to use HasOverlap after this. Is that expected? Should we be returning an error if it is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
auto IsBound = Info.isBound(ResList.first, ResBinding.Space, ResRange);
if (!IsBound) {
reportRegNotBound(M, ResList.first, ResBinding);
}
if (!Info.isBound(ResList.first, ResBinding.Space, ResRange)
reportRegNotBound(M, ResList.first, ResBinding);

}
}
}
}
} // namespace
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
; 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=0, register=4294967295) does not have a binding in the Root Signature
; CHECK: error: register UAV (space=0, register=4294967294) does not have a binding in the Root Signature

; Root Signature(
; CBV(b3, space=666, visibility=SHADER_VISIBILITY_ALL)
Expand All @@ -12,8 +12,8 @@

define void @CSMain() "hlsl.shader"="compute" {
entry:
; RWBuffer<float> UAV : register(u4294967295);
%RWB = tail call target("dx.TypedBuffer", float, 1, 0, 0) @llvm.dx.resource.handlefrombinding.tdx.TypedBuffer_f32_1_0_0t(i32 0, i32 -1, i32 1, i32 0, i1 false, ptr nonnull @RWB.str)
; RWBuffer<float> UAV : register(4294967294);
%RWB = tail call target("dx.TypedBuffer", float, 1, 0, 0) @llvm.dx.resource.handlefrombinding.tdx.TypedBuffer_f32_1_0_0t(i32 0, i32 4294967294, i32 1, i32 0, i1 false, ptr nonnull @RWB.str)
ret void
}

Expand All @@ -27,4 +27,4 @@ entry:
!5 = !{!"DescriptorTable", i32 0, !6}
!6 = !{!"Sampler", i32 2, i32 0, i32 0, i32 -1, i32 0}
!7 = !{!"DescriptorTable", i32 0, !8}
!8 = !{!"UAV", i32 -1, i32 0, i32 0, i32 -1, i32 2}
!8 = !{!"UAV", i32 10, i32 0, i32 0, i32 -1, i32 2}
Loading