Skip to content

Commit fb1035c

Browse files
hekotahekota
authored
[DirectX] Fix resource binding analysis incorrectly removing duplicates (#152253)
The resource binding analysis was incorrectly reducing the size of the `Bindings` vector by one element after sorting and de-duplication. This led to an inaccurate setting of the `HasOverlappingBinding` flag in the `DXILResourceBindingInfo` analysis, as the truncated vector no longer reflected the true binding state. This update corrects the shrink logic and introduces an `assert` in the `DXILPostOptimizationValidation` pass. The assertion will trigger if `HasOverlappingBinding` is set but no corresponding error is detected, helping catch future inconsistencies. The bug surfaced when the `srv_metadata.hlsl` and `uav_metadata.hlsl` tests were updated to include unbounded resource arrays as part of llvm/llvm-project#145422. These updated test files are included in this PR, as they would cause the new assertion to fire if the original issue remained unresolved. Depends on #152250
1 parent 049953f commit fb1035c

File tree

5 files changed

+84
-38
lines changed

5 files changed

+84
-38
lines changed

‎llvm/lib/Frontend/HLSL/HLSLBinding.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ BindingInfo BindingInfoBuilder::calculateBindingInfo(
7676
// remove duplicates
7777
Binding *NewEnd = llvm::unique(Bindings);
7878
if (NewEnd != Bindings.end())
79-
Bindings.erase(NewEnd);
79+
Bindings.erase(NewEnd, Bindings.end());
8080

8181
BindingInfo Info;
8282

‎llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,7 @@ static void reportOverlappingError(Module &M, ResourceInfo R1,
6363
}
6464

6565
static void reportOverlappingBinding(Module &M, DXILResourceMap &DRM) {
66-
if (DRM.empty())
67-
return;
68-
66+
bool ErrorFound = false;
6967
for (const auto &ResList :
7068
{DRM.srvs(), DRM.uavs(), DRM.cbuffers(), DRM.samplers()}) {
7169
if (ResList.empty())
@@ -77,11 +75,15 @@ static void reportOverlappingBinding(Module &M, DXILResourceMap &DRM) {
7775
while (RI != ResList.end() &&
7876
PrevRI->getBinding().overlapsWith(RI->getBinding())) {
7977
reportOverlappingError(M, *PrevRI, *RI);
78+
ErrorFound = true;
8079
RI++;
8180
}
8281
PrevRI = CurrentRI;
8382
}
8483
}
84+
assert(ErrorFound && "this function should be called only when if "
85+
"DXILResourceBindingInfo::hasOverlapingBinding() is "
86+
"true, yet no overlapping binding was found");
8587
}
8688

8789
static void reportErrors(Module &M, DXILResourceMap &DRM,

‎llvm/test/CodeGen/DirectX/Metadata/srv_metadata.ll‎

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
; RUN: opt -S -dxil-translate-metadata < %s | FileCheck %s
22
; RUN: opt -S --passes="dxil-pretty-printer" < %s 2>&1 | FileCheck %s --check-prefix=PRINT
3-
; RUN: llc %s --filetype=asm -o - < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,PRINT
3+
; RUN: llc %s --filetype=asm -o - 2>&1 | FileCheck %s --check-prefixes=CHECK,PRINT
44

55
target datalayout = "e-m:e-p:32:32-i1:32-i8:8-i16:16-i32:32-i64:64-f16:16-f32:32-f64:64-n8:16:32:64"
66
target triple = "dxil-pc-shadermodel6.6-compute"
@@ -14,20 +14,22 @@ target triple = "dxil-pc-shadermodel6.6-compute"
1414
@Six.str = private unnamed_addr constant [4 x i8] c"Six\00", align 1
1515
@Seven.str = private unnamed_addr constant [6 x i8] c"Seven\00", align 1
1616
@Array.str = private unnamed_addr constant [6 x i8] c"Array\00", align 1
17+
@Array2.str = private unnamed_addr constant [7 x i8] c"Array2\00", align 1
1718

1819
; PRINT:; Resource Bindings:
1920
; PRINT-NEXT:;
20-
; PRINT-NEXT:; Name Type Format Dim ID HLSL Bind Count
21-
; PRINT-NEXT:; ------------------------------ ---------- ------- ----------- ------- -------------- ------
22-
; PRINT-NEXT:; Zero texture f16 buf T0 t0 1
23-
; PRINT-NEXT:; One texture f32 buf T1 t1 1
24-
; PRINT-NEXT:; Two texture f64 buf T2 t2 1
25-
; PRINT-NEXT:; Three texture i32 buf T3 t3 1
26-
; PRINT-NEXT:; Four texture byte r/o T4 t5 1
27-
; PRINT-NEXT:; Five texture struct r/o T5 t6 1
28-
; PRINT-NEXT:; Six texture u64 buf T6 t10,space2 1
29-
; PRINT-NEXT:; Array texture f32 buf T7 t4,space3 100
30-
; PRINT-NEXT:; Seven texture u64 buf T8 t20,space5 1
21+
; PRINT-NEXT:; Name Type Format Dim ID HLSL Bind Count
22+
; PRINT-NEXT:; ------------------------------ ---------- ------- ----------- ------- -------------- ---------
23+
; PRINT-NEXT:; Zero texture f16 buf T0 t0 1
24+
; PRINT-NEXT:; One texture f32 buf T1 t1 1
25+
; PRINT-NEXT:; Two texture f64 buf T2 t2 1
26+
; PRINT-NEXT:; Three texture i32 buf T3 t3 1
27+
; PRINT-NEXT:; Four texture byte r/o T4 t5 1
28+
; PRINT-NEXT:; Five texture struct r/o T5 t6 1
29+
; PRINT-NEXT:; Six texture u64 buf T6 t10,space2 1
30+
; PRINT-NEXT:; Array texture f32 buf T7 t4,space3 100
31+
; PRINT-NEXT:; Array2 texture f64 buf T8 t2,space4 unbounded
32+
; PRINT-NEXT:; Seven texture u64 buf T9 t20,space5 1
3133
;
3234

3335
define void @test() #0 {
@@ -60,19 +62,28 @@ define void @test() #0 {
6062
@llvm.dx.resource.handlefrombinding(i32 2, i32 10, i32 1, i32 0, i1 false, ptr @Six.str)
6163

6264
; Same buffer type as Six - should have the same type in metadata
63-
; Buffer<double> Seven : register(t10, space2);
65+
; Buffer<double> Seven : register(t20, space5);
6466
%Seven_h = call target("dx.TypedBuffer", i64, 0, 0, 0)
6567
@llvm.dx.resource.handlefrombinding(i32 5, i32 20, i32 1, i32 0, i1 false, ptr @Seven.str)
6668

6769
; Buffer<float4> Array[100] : register(t4, space3);
6870
; Buffer<float4> B1 = Array[30];
69-
; Buffer<float4> B1 = Array[42];
71+
; Buffer<float4> B2 = Array[42];
7072
; resource array accesses should produce one metadata entry
7173
%Array_30_h = call target("dx.TypedBuffer", <4 x float>, 0, 0, 0)
7274
@llvm.dx.resource.handlefrombinding(i32 3, i32 4, i32 100, i32 30, i1 false, ptr @Array.str)
7375
%Array_42_h = call target("dx.TypedBuffer", <4 x float>, 0, 0, 0)
7476
@llvm.dx.resource.handlefrombinding(i32 3, i32 4, i32 100, i32 42, i1 false, ptr @Array.str)
7577

78+
; test unbounded resource array
79+
; Buffer<double> Array2[] : register(t2, space4);
80+
; Buffer<double> C1 = Array[10];
81+
; Buffer<double> C2 = Array[20];
82+
%Array2_10_h = call target("dx.TypedBuffer", double, 0, 0, 0)
83+
@llvm.dx.resource.handlefrombinding(i32 4, i32 2, i32 -1, i32 10, i1 false, ptr @Array2.str)
84+
%Array2_20_h = call target("dx.TypedBuffer", double, 0, 0, 0)
85+
@llvm.dx.resource.handlefrombinding(i32 4, i32 2, i32 -1, i32 20, i1 false, ptr @Array2.str)
86+
7687
ret void
7788
}
7889

@@ -95,14 +106,15 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
95106
; CHECK: @Five = external constant %"StructuredBuffer<int16_t>"
96107
; CHECK: @Six = external constant %"Buffer<uint32_t>"
97108
; CHECK: @Array = external constant %"Buffer<float4>"
109+
; CHECK: @Array2 = external constant %"Buffer<double>"
98110
; CHECK: @Seven = external constant %"Buffer<uint32_t>"
99111

100112
; CHECK: !dx.resources = !{[[ResList:[!][0-9]+]]}
101113

102114
; CHECK: [[ResList]] = !{[[SRVList:[!][0-9]+]], null, null, null}
103115
; CHECK: [[SRVList]] = !{![[Zero:[0-9]+]], ![[One:[0-9]+]], ![[Two:[0-9]+]],
104116
; CHECK-SAME: ![[Three:[0-9]+]], ![[Four:[0-9]+]], ![[Five:[0-9]+]],
105-
; CHECK-SAME: ![[Six:[0-9]+]], ![[Array:[0-9]+]], ![[Seven:[0-9]+]]}
117+
; CHECK-SAME: ![[Six:[0-9]+]], ![[Array:[0-9]+]], ![[Array2:[0-9]+]], ![[Seven:[0-9]+]]}
106118

107119
; CHECK: ![[Zero]] = !{i32 0, ptr @Zero, !"Zero", i32 0, i32 0, i32 1, i32 10, i32 0, ![[Half:[0-9]+]]}
108120
; CHECK: ![[Half]] = !{i32 0, i32 8}
@@ -118,4 +130,5 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
118130
; CHECK: ![[Six]] = !{i32 6, ptr @Six, !"Six", i32 2, i32 10, i32 1, i32 10, i32 0, ![[U64:[0-9]+]]}
119131
; CHECK: ![[U64]] = !{i32 0, i32 7}
120132
; CHECK: ![[Array]] = !{i32 7, ptr @Array, !"Array", i32 3, i32 4, i32 100, i32 10, i32 0, ![[Float]]}
121-
; CHECK: ![[Seven]] = !{i32 8, ptr @Seven, !"Seven", i32 5, i32 20, i32 1, i32 10, i32 0, ![[U64]]}
133+
; CHECK: ![[Array2]] = !{i32 8, ptr @Array2, !"Array2", i32 4, i32 2, i32 -1, i32 10, i32 0, ![[Double]]}
134+
; CHECK: ![[Seven]] = !{i32 9, ptr @Seven, !"Seven", i32 5, i32 20, i32 1, i32 10, i32 0, ![[U64]]}

‎llvm/test/CodeGen/DirectX/Metadata/uav_metadata.ll‎

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
; RUN: opt -S -dxil-translate-metadata < %s | FileCheck %s
22
; RUN: opt -S --passes="dxil-pretty-printer" < %s 2>&1 | FileCheck %s --check-prefix=PRINT
3-
; RUN: llc %s --filetype=asm -o - < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,PRINT
3+
; RUN: llc %s --filetype=asm -o - 2>&1 | FileCheck %s --check-prefixes=CHECK,PRINT
44

55
target datalayout = "e-m:e-p:32:32-i1:32-i8:8-i16:16-i32:32-i64:64-f16:16-f32:32-f64:64-n8:16:32:64"
66
target triple = "dxil-pc-shadermodel6.6-compute"
@@ -17,23 +17,25 @@ target triple = "dxil-pc-shadermodel6.6-compute"
1717
@Nine.str = private unnamed_addr constant [5 x i8] c"Nine\00", align 1
1818
@Ten.str = private unnamed_addr constant [4 x i8] c"Ten\00", align 1
1919
@Array.str = private unnamed_addr constant [6 x i8] c"Array\00", align 1
20+
@Array2.str = private unnamed_addr constant [7 x i8] c"Array2\00", align 1
2021

2122
; PRINT:; Resource Bindings:
2223
; PRINT-NEXT:;
23-
; PRINT-NEXT:; Name Type Format Dim ID HLSL Bind Count
24-
; PRINT-NEXT:; ------------------------------ ---------- ------- ----------- ------- -------------- ------
25-
; PRINT-NEXT:; Zero UAV f16 buf U0 u0 1
26-
; PRINT-NEXT:; One UAV f32 buf U1 u1 1
27-
; PRINT-NEXT:; Two UAV f64 buf U2 u2 1
28-
; PRINT-NEXT:; Three UAV i32 buf U3 u3 1
29-
; PRINT-NEXT:; Four UAV byte r/w U4 u5 1
30-
; PRINT-NEXT:; Five UAV struct r/w U5 u6 1
31-
; PRINT-NEXT:; Six UAV i32 buf U6 u7 1
32-
; PRINT-NEXT:; Seven UAV struct r/w U7 u8 1
33-
; PRINT-NEXT:; Eight UAV byte r/w U8 u9 1
34-
; PRINT-NEXT:; Nine UAV u64 buf U9 u10,space2 1
35-
; PRINT-NEXT:; Array UAV f32 buf U10 u4,space3 100
36-
; PRINT-NEXT:; Ten UAV u64 buf U11 u22,space5 1
24+
; PRINT-NEXT:; Name Type Format Dim ID HLSL Bind Count
25+
; PRINT-NEXT:; ------------------------------ ---------- ------- ----------- ------- -------------- ---------
26+
; PRINT-NEXT:; Zero UAV f16 buf U0 u0 1
27+
; PRINT-NEXT:; One UAV f32 buf U1 u1 1
28+
; PRINT-NEXT:; Two UAV f64 buf U2 u2 1
29+
; PRINT-NEXT:; Three UAV i32 buf U3 u3 1
30+
; PRINT-NEXT:; Four UAV byte r/w U4 u5 1
31+
; PRINT-NEXT:; Five UAV struct r/w U5 u6 1
32+
; PRINT-NEXT:; Six UAV i32 buf U6 u7 1
33+
; PRINT-NEXT:; Seven UAV struct r/w U7 u8 1
34+
; PRINT-NEXT:; Eight UAV byte r/w U8 u9 1
35+
; PRINT-NEXT:; Nine UAV u64 buf U9 u10,space2 1
36+
; PRINT-NEXT:; Array UAV f32 buf U10 u4,space3 100
37+
; PRINT-NEXT:; Array2 UAV f64 buf U11 u2,space4 unbounded
38+
; PRINT-NEXT:; Ten UAV u64 buf U12 u22,space5 1
3739

3840
define void @test() #0 {
3941
; RWBuffer<half4> Zero : register(u0)
@@ -78,13 +80,22 @@ define void @test() #0 {
7880

7981
; RWBuffer<float4> Array[100] : register(u4, space3);
8082
; RWBuffer<float4> B1 = Array[30];
81-
; RWBuffer<float4> B1 = Array[42];
83+
; RWBuffer<float4> B2 = Array[42];
8284
; resource array accesses should produce one metadata entry
8385
%Array_30_h = call target("dx.TypedBuffer", <4 x float>, 1, 0, 0)
8486
@llvm.dx.resource.handlefrombinding(i32 3, i32 4, i32 100, i32 30, i1 false, ptr @Array.str)
8587
%Array_42_h = call target("dx.TypedBuffer", <4 x float>, 1, 0, 0)
8688
@llvm.dx.resource.handlefrombinding(i32 3, i32 4, i32 100, i32 42, i1 false, ptr @Array.str)
8789

90+
; test unbounded resource array
91+
; RWBuffer<double> Array2[] : register(u2, space4);
92+
; RWBuffer<double> C1 = Array[10];
93+
; RWBuffer<double> C2 = Array[20];
94+
%Array2_10_h = call target("dx.TypedBuffer", double, 1, 0, 0)
95+
@llvm.dx.resource.handlefrombinding(i32 4, i32 2, i32 -1, i32 10, i1 false, ptr @Array2.str)
96+
%Array2_20_h = call target("dx.TypedBuffer", double, 1, 0, 0)
97+
@llvm.dx.resource.handlefrombinding(i32 4, i32 2, i32 -1, i32 20, i1 false, ptr @Array2.str)
98+
8899
; Same buffer type as Nine - should have the same type in metadata
89100
; RWBuffer<double> Ten : register(u2);
90101
%Ten_h = call target("dx.TypedBuffer", i64, 1, 0, 0)
@@ -118,6 +129,7 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
118129
; CHECK: @Eight = external constant %RasterizerOrderedByteAddressBuffer
119130
; CHECK: @Nine = external constant %"RWBuffer<uint32_t>"
120131
; CHECK: @Array = external constant %"RWBuffer<float4>"
132+
; CHECK: @Array2 = external constant %"RWBuffer<double>"
121133
; CHECK: @Ten = external constant %"RWBuffer<uint32_t>"
122134

123135
; CHECK: !dx.resources = !{[[ResList:[!][0-9]+]]}
@@ -126,7 +138,7 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
126138
; CHECK: [[UAVList]] = !{![[Zero:[0-9]+]], ![[One:[0-9]+]], ![[Two:[0-9]+]],
127139
; CHECK-SAME: ![[Three:[0-9]+]], ![[Four:[0-9]+]], ![[Five:[0-9]+]],
128140
; CHECK-SAME: ![[Six:[0-9]+]], ![[Seven:[0-9]+]], ![[Eight:[0-9]+]],
129-
; CHECK-SAME: ![[Nine:[0-9]+]], ![[Array:[0-9]+]], ![[Ten:[0-9]+]]}
141+
; CHECK-SAME: ![[Nine:[0-9]+]], ![[Array:[0-9]+]], ![[Array2:[0-9]+]], ![[Ten:[0-9]+]]}
130142

131143
; CHECK: ![[Zero]] = !{i32 0, ptr @Zero, !"Zero", i32 0, i32 0, i32 1, i32 10, i1 false, i1 false, i1 false, ![[Half:[0-9]+]]}
132144
; CHECK: ![[Half]] = !{i32 0, i32 8}
@@ -146,4 +158,5 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
146158
; CHECK: ![[Nine]] = !{i32 9, ptr @Nine, !"Nine", i32 2, i32 10, i32 1, i32 10, i1 false, i1 false, i1 false, ![[U64:[0-9]+]]}
147159
; CHECK: ![[U64]] = !{i32 0, i32 7}
148160
; CHECK: ![[Array]] = !{i32 10, ptr @Array, !"Array", i32 3, i32 4, i32 100, i32 10, i1 false, i1 false, i1 false, ![[Float]]}
149-
; CHECK: ![[Ten]] = !{i32 11, ptr @Ten, !"Ten", i32 5, i32 22, i32 1, i32 10, i1 false, i1 false, i1 false, ![[U64:[0-9]+]]}
161+
; CHECK: ![[Array2]] = !{i32 11, ptr @Array2, !"Array2", i32 4, i32 2, i32 -1, i32 10, i1 false, i1 false, i1 false, ![[Double]]}
162+
; CHECK: ![[Ten]] = !{i32 12, ptr @Ten, !"Ten", i32 5, i32 22, i32 1, i32 10, i1 false, i1 false, i1 false, ![[U64:[0-9]+]]}

‎llvm/unittests/Frontend/HLSLBindingTest.cpp‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,3 +273,21 @@ TEST(HLSLBindingTest, TestFindAvailable) {
273273
// In an empty space we find the slot at the beginning.
274274
EXPECT_THAT(V, HasSpecificValue(0u));
275275
}
276+
277+
// Test that add duplicate bindings are correctly de-duplicated
278+
TEST(HLSLBindingTest, TestNoOverlapWithDuplicates) {
279+
hlsl::BindingInfoBuilder Builder;
280+
281+
// We add the same binding three times, and just use `nullptr` for the cookie
282+
// so that they should all be uniqued away.
283+
Builder.trackBinding(ResourceClass::SRV, /*Space=*/0, /*LowerBound=*/5,
284+
/*UpperBound=*/5, /*Cookie=*/nullptr);
285+
Builder.trackBinding(ResourceClass::SRV, /*Space=*/0, /*LowerBound=*/5,
286+
/*UpperBound=*/5, /*Cookie=*/nullptr);
287+
Builder.trackBinding(ResourceClass::SRV, /*Space=*/0, /*LowerBound=*/5,
288+
/*UpperBound=*/5, /*Cookie=*/nullptr);
289+
bool HasOverlap;
290+
hlsl::BindingInfo Info = Builder.calculateBindingInfo(HasOverlap);
291+
292+
EXPECT_FALSE(HasOverlap);
293+
}

0 commit comments

Comments
 (0)