[DirectX] Fix resource binding analysis incorrectly removing duplicates #152253
+84
−38
Conversation
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
|
@llvm/pr-subscribers-hlsl @llvm/pr-subscribers-backend-directx Author: Helena Kotas (hekota) ChangesThe resource binding analysis was incorrectly reducing the size of the This update corrects the shrink logic and introduces an The bug surfaced when the Depends on #152250 Full diff: https://github.com/llvm/llvm-project/pull/152253.diff 4 Files Affected:
diff --git a/llvm/lib/Frontend/HLSL/HLSLBinding.cpp b/llvm/lib/Frontend/HLSL/HLSLBinding.cpp
index d581311f22028..45391460354d0 100644
--- a/llvm/lib/Frontend/HLSL/HLSLBinding.cpp
+++ b/llvm/lib/Frontend/HLSL/HLSLBinding.cpp
@@ -76,7 +76,7 @@ BindingInfo BindingInfoBuilder::calculateBindingInfo(
// remove duplicates
Binding *NewEnd = llvm::unique(Bindings);
if (NewEnd != Bindings.end())
- Bindings.erase(NewEnd);
+ Bindings.erase(NewEnd, Bindings.end());
BindingInfo Info;
diff --git a/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp b/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
index 398dcbb8d1737..9d995bc70a997 100644
--- a/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
+++ b/llvm/lib/Target/DirectX/DXILPostOptimizationValidation.cpp
@@ -63,9 +63,7 @@ static void reportOverlappingError(Module &M, ResourceInfo R1,
}
static void reportOverlappingBinding(Module &M, DXILResourceMap &DRM) {
- if (DRM.empty())
- return;
-
+ bool ErrorFound = false;
for (const auto &ResList :
{DRM.srvs(), DRM.uavs(), DRM.cbuffers(), DRM.samplers()}) {
if (ResList.empty())
@@ -77,11 +75,15 @@ static void reportOverlappingBinding(Module &M, DXILResourceMap &DRM) {
while (RI != ResList.end() &&
PrevRI->getBinding().overlapsWith(RI->getBinding())) {
reportOverlappingError(M, *PrevRI, *RI);
+ ErrorFound = true;
RI++;
}
PrevRI = CurrentRI;
}
}
+ assert(ErrorFound && "this function should be called only when if "
+ "DXILResourceBindingInfo::hasOverlapingBinding() is "
+ "true, yet no overlapping binding was found");
}
static void reportErrors(Module &M, DXILResourceMap &DRM,
diff --git a/llvm/test/CodeGen/DirectX/Metadata/srv_metadata.ll b/llvm/test/CodeGen/DirectX/Metadata/srv_metadata.ll
index abab5c9fb1166..5c9fb93d7f570 100644
--- a/llvm/test/CodeGen/DirectX/Metadata/srv_metadata.ll
+++ b/llvm/test/CodeGen/DirectX/Metadata/srv_metadata.ll
@@ -1,6 +1,6 @@
; RUN: opt -S -dxil-translate-metadata < %s | FileCheck %s
; RUN: opt -S --passes="dxil-pretty-printer" < %s 2>&1 | FileCheck %s --check-prefix=PRINT
-; RUN: llc %s --filetype=asm -o - < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,PRINT
+; RUN: llc %s --filetype=asm -o - 2>&1 | FileCheck %s --check-prefixes=CHECK,PRINT
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"
target triple = "dxil-pc-shadermodel6.6-compute"
@@ -14,20 +14,22 @@ target triple = "dxil-pc-shadermodel6.6-compute"
@Six.str = private unnamed_addr constant [4 x i8] c"Six\00", align 1
@Seven.str = private unnamed_addr constant [6 x i8] c"Seven\00", align 1
@Array.str = private unnamed_addr constant [6 x i8] c"Array\00", align 1
+@Array2.str = private unnamed_addr constant [7 x i8] c"Array2\00", align 1
; PRINT:; Resource Bindings:
; PRINT-NEXT:;
-; PRINT-NEXT:; Name Type Format Dim ID HLSL Bind Count
-; PRINT-NEXT:; ------------------------------ ---------- ------- ----------- ------- -------------- ------
-; PRINT-NEXT:; Zero texture f16 buf T0 t0 1
-; PRINT-NEXT:; One texture f32 buf T1 t1 1
-; PRINT-NEXT:; Two texture f64 buf T2 t2 1
-; PRINT-NEXT:; Three texture i32 buf T3 t3 1
-; PRINT-NEXT:; Four texture byte r/o T4 t5 1
-; PRINT-NEXT:; Five texture struct r/o T5 t6 1
-; PRINT-NEXT:; Six texture u64 buf T6 t10,space2 1
-; PRINT-NEXT:; Array texture f32 buf T7 t4,space3 100
-; PRINT-NEXT:; Seven texture u64 buf T8 t20,space5 1
+; PRINT-NEXT:; Name Type Format Dim ID HLSL Bind Count
+; PRINT-NEXT:; ------------------------------ ---------- ------- ----------- ------- -------------- --------
+; PRINT-NEXT:; Zero texture f16 buf T0 t0 1
+; PRINT-NEXT:; One texture f32 buf T1 t1 1
+; PRINT-NEXT:; Two texture f64 buf T2 t2 1
+; PRINT-NEXT:; Three texture i32 buf T3 t3 1
+; PRINT-NEXT:; Four texture byte r/o T4 t5 1
+; PRINT-NEXT:; Five texture struct r/o T5 t6 1
+; PRINT-NEXT:; Six texture u64 buf T6 t10,space2 1
+; PRINT-NEXT:; Array texture f32 buf T7 t4,space3 100
+; PRINT-NEXT:; Array2 texture f64 buf T8 t2,space4 unbounded
+; PRINT-NEXT:; Seven texture u64 buf T9 t20,space5 1
;
define void @test() #0 {
@@ -60,19 +62,28 @@ define void @test() #0 {
@llvm.dx.resource.handlefrombinding(i32 2, i32 10, i32 1, i32 0, i1 false, ptr @Six.str)
; Same buffer type as Six - should have the same type in metadata
- ; Buffer<double> Seven : register(t10, space2);
+ ; Buffer<double> Seven : register(t20, space5);
%Seven_h = call target("dx.TypedBuffer", i64, 0, 0, 0)
@llvm.dx.resource.handlefrombinding(i32 5, i32 20, i32 1, i32 0, i1 false, ptr @Seven.str)
; Buffer<float4> Array[100] : register(t4, space3);
; Buffer<float4> B1 = Array[30];
- ; Buffer<float4> B1 = Array[42];
+ ; Buffer<float4> B2 = Array[42];
; resource array accesses should produce one metadata entry
%Array_30_h = call target("dx.TypedBuffer", <4 x float>, 0, 0, 0)
@llvm.dx.resource.handlefrombinding(i32 3, i32 4, i32 100, i32 30, i1 false, ptr @Array.str)
%Array_42_h = call target("dx.TypedBuffer", <4 x float>, 0, 0, 0)
@llvm.dx.resource.handlefrombinding(i32 3, i32 4, i32 100, i32 42, i1 false, ptr @Array.str)
+ ; test unbounded resource array
+ ; Buffer<double> Array2[] : register(t2, space4);
+ ; Buffer<double> C1 = Array[10];
+ ; Buffer<double> C2 = Array[20];
+ %Array2_10_h = call target("dx.TypedBuffer", double, 0, 0, 0)
+ @llvm.dx.resource.handlefrombinding(i32 4, i32 2, i32 -1, i32 10, i1 false, ptr @Array2.str)
+ %Array2_20_h = call target("dx.TypedBuffer", double, 0, 0, 0)
+ @llvm.dx.resource.handlefrombinding(i32 4, i32 2, i32 -1, i32 20, i1 false, ptr @Array2.str)
+
ret void
}
@@ -95,6 +106,7 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
; CHECK: @Five = external constant %"StructuredBuffer<int16_t>"
; CHECK: @Six = external constant %"Buffer<uint32_t>"
; CHECK: @Array = external constant %"Buffer<float4>"
+; CHECK: @Array2 = external constant %"Buffer<double>"
; CHECK: @Seven = external constant %"Buffer<uint32_t>"
; CHECK: !dx.resources = !{[[ResList:[!][0-9]+]]}
@@ -102,7 +114,7 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
; CHECK: [[ResList]] = !{[[SRVList:[!][0-9]+]], null, null, null}
; CHECK: [[SRVList]] = !{![[Zero:[0-9]+]], ![[One:[0-9]+]], ![[Two:[0-9]+]],
; CHECK-SAME: ![[Three:[0-9]+]], ![[Four:[0-9]+]], ![[Five:[0-9]+]],
-; CHECK-SAME: ![[Six:[0-9]+]], ![[Array:[0-9]+]], ![[Seven:[0-9]+]]}
+; CHECK-SAME: ![[Six:[0-9]+]], ![[Array:[0-9]+]], ![[Array2:[0-9]+]], ![[Seven:[0-9]+]]}
; CHECK: ![[Zero]] = !{i32 0, ptr @Zero, !"Zero", i32 0, i32 0, i32 1, i32 10, i32 0, ![[Half:[0-9]+]]}
; CHECK: ![[Half]] = !{i32 0, i32 8}
@@ -118,4 +130,5 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
; CHECK: ![[Six]] = !{i32 6, ptr @Six, !"Six", i32 2, i32 10, i32 1, i32 10, i32 0, ![[U64:[0-9]+]]}
; CHECK: ![[U64]] = !{i32 0, i32 7}
; CHECK: ![[Array]] = !{i32 7, ptr @Array, !"Array", i32 3, i32 4, i32 100, i32 10, i32 0, ![[Float]]}
-; CHECK: ![[Seven]] = !{i32 8, ptr @Seven, !"Seven", i32 5, i32 20, i32 1, i32 10, i32 0, ![[U64]]}
+; CHECK: ![[Array2]] = !{i32 8, ptr @Array2, !"Array2", i32 4, i32 2, i32 -1, i32 10, i32 0, ![[Double]]}
+; CHECK: ![[Seven]] = !{i32 9, ptr @Seven, !"Seven", i32 5, i32 20, i32 1, i32 10, i32 0, ![[U64]]}
diff --git a/llvm/test/CodeGen/DirectX/Metadata/uav_metadata.ll b/llvm/test/CodeGen/DirectX/Metadata/uav_metadata.ll
index 9893f8b9ea4d7..77f36dd40fd7c 100644
--- a/llvm/test/CodeGen/DirectX/Metadata/uav_metadata.ll
+++ b/llvm/test/CodeGen/DirectX/Metadata/uav_metadata.ll
@@ -1,6 +1,6 @@
; RUN: opt -S -dxil-translate-metadata < %s | FileCheck %s
; RUN: opt -S --passes="dxil-pretty-printer" < %s 2>&1 | FileCheck %s --check-prefix=PRINT
-; RUN: llc %s --filetype=asm -o - < %s 2>&1 | FileCheck %s --check-prefixes=CHECK,PRINT
+; RUN: llc %s --filetype=asm -o - 2>&1 | FileCheck %s --check-prefixes=CHECK,PRINT
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"
target triple = "dxil-pc-shadermodel6.6-compute"
@@ -17,23 +17,25 @@ target triple = "dxil-pc-shadermodel6.6-compute"
@Nine.str = private unnamed_addr constant [5 x i8] c"Nine\00", align 1
@Ten.str = private unnamed_addr constant [4 x i8] c"Ten\00", align 1
@Array.str = private unnamed_addr constant [6 x i8] c"Array\00", align 1
+@Array2.str = private unnamed_addr constant [7 x i8] c"Array2\00", align 1
; PRINT:; Resource Bindings:
; PRINT-NEXT:;
-; PRINT-NEXT:; Name Type Format Dim ID HLSL Bind Count
-; PRINT-NEXT:; ------------------------------ ---------- ------- ----------- ------- -------------- ------
-; PRINT-NEXT:; Zero UAV f16 buf U0 u0 1
-; PRINT-NEXT:; One UAV f32 buf U1 u1 1
-; PRINT-NEXT:; Two UAV f64 buf U2 u2 1
-; PRINT-NEXT:; Three UAV i32 buf U3 u3 1
-; PRINT-NEXT:; Four UAV byte r/w U4 u5 1
-; PRINT-NEXT:; Five UAV struct r/w U5 u6 1
-; PRINT-NEXT:; Six UAV i32 buf U6 u7 1
-; PRINT-NEXT:; Seven UAV struct r/w U7 u8 1
-; PRINT-NEXT:; Eight UAV byte r/w U8 u9 1
-; PRINT-NEXT:; Nine UAV u64 buf U9 u10,space2 1
-; PRINT-NEXT:; Array UAV f32 buf U10 u4,space3 100
-; PRINT-NEXT:; Ten UAV u64 buf U11 u22,space5 1
+; PRINT-NEXT:; Name Type Format Dim ID HLSL Bind Count
+; PRINT-NEXT:; ------------------------------ ---------- ------- ----------- ------- -------------- ---------
+; PRINT-NEXT:; Zero UAV f16 buf U0 u0 1
+; PRINT-NEXT:; One UAV f32 buf U1 u1 1
+; PRINT-NEXT:; Two UAV f64 buf U2 u2 1
+; PRINT-NEXT:; Three UAV i32 buf U3 u3 1
+; PRINT-NEXT:; Four UAV byte r/w U4 u5 1
+; PRINT-NEXT:; Five UAV struct r/w U5 u6 1
+; PRINT-NEXT:; Six UAV i32 buf U6 u7 1
+; PRINT-NEXT:; Seven UAV struct r/w U7 u8 1
+; PRINT-NEXT:; Eight UAV byte r/w U8 u9 1
+; PRINT-NEXT:; Nine UAV u64 buf U9 u10,space2 1
+; PRINT-NEXT:; Array UAV f32 buf U10 u4,space3 100
+; PRINT-NEXT:; Array2 UAV f64 buf U11 u2,space4 unbounded
+; PRINT-NEXT:; Ten UAV u64 buf U12 u22,space5 1
define void @test() #0 {
; RWBuffer<half4> Zero : register(u0)
@@ -78,13 +80,22 @@ define void @test() #0 {
; RWBuffer<float4> Array[100] : register(u4, space3);
; RWBuffer<float4> B1 = Array[30];
- ; RWBuffer<float4> B1 = Array[42];
+ ; RWBuffer<float4> B2 = Array[42];
; resource array accesses should produce one metadata entry
%Array_30_h = call target("dx.TypedBuffer", <4 x float>, 1, 0, 0)
@llvm.dx.resource.handlefrombinding(i32 3, i32 4, i32 100, i32 30, i1 false, ptr @Array.str)
%Array_42_h = call target("dx.TypedBuffer", <4 x float>, 1, 0, 0)
@llvm.dx.resource.handlefrombinding(i32 3, i32 4, i32 100, i32 42, i1 false, ptr @Array.str)
+ ; test unbounded resource array
+ ; RWBuffer<double> Array2[] : register(u2, space4);
+ ; RWBuffer<double> C1 = Array[10];
+ ; RWBuffer<double> C2 = Array[20];
+ %Array2_10_h = call target("dx.TypedBuffer", double, 1, 0, 0)
+ @llvm.dx.resource.handlefrombinding(i32 4, i32 2, i32 -1, i32 10, i1 false, ptr @Array2.str)
+ %Array2_20_h = call target("dx.TypedBuffer", double, 1, 0, 0)
+ @llvm.dx.resource.handlefrombinding(i32 4, i32 2, i32 -1, i32 20, i1 false, ptr @Array2.str)
+
; Same buffer type as Nine - should have the same type in metadata
; RWBuffer<double> Ten : register(u2);
%Ten_h = call target("dx.TypedBuffer", i64, 1, 0, 0)
@@ -118,6 +129,7 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
; CHECK: @Eight = external constant %RasterizerOrderedByteAddressBuffer
; CHECK: @Nine = external constant %"RWBuffer<uint32_t>"
; CHECK: @Array = external constant %"RWBuffer<float4>"
+; CHECK: @Array2 = external constant %"RWBuffer<double>"
; CHECK: @Ten = external constant %"RWBuffer<uint32_t>"
; CHECK: !dx.resources = !{[[ResList:[!][0-9]+]]}
@@ -126,7 +138,7 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
; CHECK: [[UAVList]] = !{![[Zero:[0-9]+]], ![[One:[0-9]+]], ![[Two:[0-9]+]],
; CHECK-SAME: ![[Three:[0-9]+]], ![[Four:[0-9]+]], ![[Five:[0-9]+]],
; CHECK-SAME: ![[Six:[0-9]+]], ![[Seven:[0-9]+]], ![[Eight:[0-9]+]],
-; CHECK-SAME: ![[Nine:[0-9]+]], ![[Array:[0-9]+]], ![[Ten:[0-9]+]]}
+; CHECK-SAME: ![[Nine:[0-9]+]], ![[Array:[0-9]+]], ![[Array2:[0-9]+]], ![[Ten:[0-9]+]]}
; CHECK: ![[Zero]] = !{i32 0, ptr @Zero, !"Zero", i32 0, i32 0, i32 1, i32 10, i1 false, i1 false, i1 false, ![[Half:[0-9]+]]}
; CHECK: ![[Half]] = !{i32 0, i32 8}
@@ -146,4 +158,5 @@ attributes #0 = { noinline nounwind "hlsl.shader"="compute" }
; CHECK: ![[Nine]] = !{i32 9, ptr @Nine, !"Nine", i32 2, i32 10, i32 1, i32 10, i1 false, i1 false, i1 false, ![[U64:[0-9]+]]}
; CHECK: ![[U64]] = !{i32 0, i32 7}
; CHECK: ![[Array]] = !{i32 10, ptr @Array, !"Array", i32 3, i32 4, i32 100, i32 10, i1 false, i1 false, i1 false, ![[Float]]}
-; CHECK: ![[Ten]] = !{i32 11, ptr @Ten, !"Ten", i32 5, i32 22, i32 1, i32 10, i1 false, i1 false, i1 false, ![[U64:[0-9]+]]}
+; CHECK: ![[Array2]] = !{i32 11, ptr @Array2, !"Array2", i32 4, i32 2, i32 -1, i32 10, i1 false, i1 false, i1 false, ![[Double]]}
+; CHECK: ![[Ten]] = !{i32 12, ptr @Ten, !"Ten", i32 5, i32 22, i32 1, i32 10, i1 false, i1 false, i1 false, ![[U64:[0-9]+]]}
|
|
This should be testable with a unit test in llvm/unittests/Fronted/HLSLBindingTest.cpp rather than via an assert in DXILPostOptimizationValidation |
Something like this should catch it I think: TEST(HLSLBindingTest, TestNoOverlapWithDuplicates) {
hlsl::BindingInfoBuilder Builder;
// We add the same binding three times, and just use `nullptr` for the cookie
// so that they should all be uniqued away.
Builder.trackBinding(ResourceClass::SRV, /*Space=*/0, /*LowerBound=*/5,
/*UpperBound=*/5, /*Cookie=*/nullptr);
Builder.trackBinding(ResourceClass::SRV, /*Space=*/0, /*LowerBound=*/5,
/*UpperBound=*/5, /*Cookie=*/nullptr);
Builder.trackBinding(ResourceClass::SRV, /*Space=*/0, /*LowerBound=*/5,
/*UpperBound=*/5, /*Cookie=*/nullptr);
bool HasOverlap;
hlsl::BindingInfo Info = Builder.calculateBindingInfo(HasOverlap);
EXPECT_FALSE(HasOverlap);
} |
Good catch! I have added a test. |
…resource-binding-unique-error
hekota
changed the base branch from
users/hekota/pr152250-fix-wrong-overlap-detection
to
main
V-FEXrt
approved these changes
Aug 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The resource binding analysis was incorrectly reducing the size of the
Bindingsvector by one element after sorting and de-duplication. This led to an inaccurate setting of theHasOverlappingBindingflag in theDXILResourceBindingInfoanalysis, as the truncated vector no longer reflected the true binding state.This update corrects the shrink logic and introduces an
assertin theDXILPostOptimizationValidationpass. The assertion will trigger ifHasOverlappingBindingis set but no corresponding error is detected, helping catch future inconsistencies.The bug surfaced when the
srv_metadata.hlslanduav_metadata.hlsltests were updated to include unbounded resource arrays as part of #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