Skip to content

Commit 1c7cb4f

Browse files
amaioranoamaiorano
authored
Fix instcombine overflow check inserting inst at wrong place (microsoft#6679)
When optimizing an overflow check of an add followed by a compare, the new instruction was being inserted at the compare, and the add removed. This produced invalid IR in cases where there were other uses of the former add between it and the compare. This fix makes sure to insert the new instruction at the old add location, rather than at the compare. Note that this was also fixed in LLVM: llvm/llvm-project@6f5dca7
1 parent 3dc6742 commit 1c7cb4f

File tree

2 files changed

+193
-0
lines changed

2 files changed

+193
-0
lines changed

‎lib/Transforms/InstCombine/InstCombineCompares.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2145,6 +2145,12 @@ bool InstCombiner::OptimizeOverflowCheck(OverflowCheckFlavor OCF, Value *LHS,
21452145
return true;
21462146
};
21472147

2148+
// If the overflow check was an add followed by a compare, the insertion point
2149+
// may be pointing to the compare. We want to insert the new instructions
2150+
// before the add in case there are uses of the add between the add and the
2151+
// compare.
2152+
Builder->SetInsertPoint(&OrigI);
2153+
21482154
switch (OCF) {
21492155
case OCF_INVALID:
21502156
llvm_unreachable("bad overflow check kind!");

‎tools/clang/test/DXC/Passes/InstructionCombining/instcombine-opt-overflow-check-inserts-at-add.ll

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
; RUN: %dxopt %s -hlsl-passes-resume -instcombine,NoSink=0 -S | FileCheck %s
2+
3+
; Generated from the following HLSL:
4+
; cbuffer cbuffer_g : register(b0) {
5+
; uint4 g[1];
6+
; };
7+
;
8+
; [numthreads(1, 1, 1)]
9+
; void main() {
10+
; uint a = 2147483648u;
11+
; uint b = (g[0].x | 2651317025u);
12+
; uint c = (b + 2651317025u);
13+
; while (true) {
14+
; bool d = (a > c);
15+
; if (d) {
16+
; break;
17+
; } else {
18+
; while (true) {
19+
; if (!d) {
20+
; return;
21+
; }
22+
; a = b;
23+
; bool e = (d ? d : d);
24+
; if (e) {
25+
; break;
26+
; }
27+
; }
28+
; }
29+
; }
30+
; }
31+
;
32+
; Compiling this was resulting in invalid IR being produced from instcombine.
33+
; Specifically, when optimizing an overflow check of an add followed by a compare,
34+
; the new instruction was being inserted at the compare, and the add removed. This
35+
; broke in cases like this one, where there were other uses of the former add between
36+
; it and the compare. The fix was to make sure to insert the new instruction
37+
; (another add in this case) at the old add rather than at the compare.
38+
39+
; Make sure the new %add is still in the entry block before its uses.
40+
; CHECK-LABEL: entry
41+
; CHECK: %add = add i32 %or, -1643650271
42+
; CHECK-NEXT: %cmp.2 = icmp sgt i32 %add, -1,
43+
;
44+
; Make sure the new %add is NOT in the loopexit where %cmp was optimized.
45+
; CHECK-LABEL: while.body.loopexit
46+
; CHECK-NEXT: br i1 false, label %if.end.preheader, label %while.end.14
47+
48+
;
49+
; Buffer Definitions:
50+
;
51+
; cbuffer cbuffer_g
52+
; {
53+
;
54+
; struct cbuffer_g
55+
; {
56+
;
57+
; uint4 g[1]; ; Offset: 0
58+
;
59+
; } cbuffer_g; ; Offset: 0 Size: 16
60+
;
61+
; }
62+
;
63+
;
64+
; Resource Bindings:
65+
;
66+
; Name Type Format Dim ID HLSL Bind Count
67+
; ------------------------------ ---------- ------- ----------- ------- -------------- ------
68+
; cbuffer_g cbuffer NA NA CB0 cb0 1
69+
;
70+
target datalayout = "e-m:e-p:32:32-i1:32-i8:32-i16:32-i32:32-i64:64-f16:32-f32:32-f64:64-n8:16:32:64"
71+
target triple = "dxil-ms-dx"
72+
73+
%cbuffer_g = type { [1 x <4 x i32>] }
74+
%dx.types.Handle = type { i8* }
75+
%dx.types.ResourceProperties = type { i32, i32 }
76+
%dx.types.CBufRet.i32 = type { i32, i32, i32, i32 }
77+
78+
@cbuffer_g = external constant %cbuffer_g
79+
@llvm.used = appending global [1 x i8*] [i8* bitcast (%cbuffer_g* @cbuffer_g to i8*)], section "llvm.metadata"
80+
81+
; Function Attrs: nounwind
82+
define void @main() #0 {
83+
entry:
84+
%0 = load %cbuffer_g, %cbuffer_g* @cbuffer_g
85+
%cbuffer_g = call %dx.types.Handle @dx.op.createHandleForLib.cbuffer_g(i32 160, %cbuffer_g %0) ; CreateHandleForLib(Resource)
86+
%1 = call %dx.types.Handle @dx.op.annotateHandle(i32 216, %dx.types.Handle %cbuffer_g, %dx.types.ResourceProperties { i32 13, i32 16 }) ; AnnotateHandle(res,props) resource: CBuffer
87+
%2 = call %dx.types.CBufRet.i32 @dx.op.cbufferLoadLegacy.i32(i32 59, %dx.types.Handle %1, i32 0), !dbg !19 ; line:8 col:13 ; CBufferLoadLegacy(handle,regIndex)
88+
%3 = extractvalue %dx.types.CBufRet.i32 %2, 0, !dbg !19 ; line:8 col:13
89+
%or = or i32 %3, -1643650271, !dbg !23 ; line:8 col:20
90+
%add = add i32 %or, -1643650271, !dbg !24 ; line:9 col:15
91+
%cmp.2 = icmp ugt i32 -2147483648, %add, !dbg !25 ; line:11 col:17
92+
%frombool.3 = zext i1 %cmp.2 to i32, !dbg !26 ; line:11 col:10
93+
%tobool1.4 = icmp eq i32 %frombool.3, 0, !dbg !27 ; line:12 col:9
94+
%or.cond.6 = and i1 %tobool1.4, %cmp.2, !dbg !27 ; line:12 col:9
95+
br i1 %or.cond.6, label %if.end.preheader, label %while.end.14, !dbg !27 ; line:12 col:9
96+
97+
while.body.loopexit: ; preds = %if.end
98+
%cmp = icmp ugt i32 %or, %add, !dbg !25 ; line:11 col:17
99+
%frombool = zext i1 %cmp to i32, !dbg !26 ; line:11 col:10
100+
%tobool1 = icmp eq i32 %frombool, 0, !dbg !27 ; line:12 col:9
101+
%or.cond = and i1 %tobool1, %cmp, !dbg !27 ; line:12 col:9
102+
br i1 %or.cond, label %if.end.preheader, label %while.end.14, !dbg !27 ; line:12 col:9
103+
104+
if.end.preheader: ; preds = %entry, %while.body.loopexit
105+
%d.0 = phi i32 [ %frombool, %while.body.loopexit ], [ %frombool.3, %entry ]
106+
br label %if.end, !dbg !28 ; line:19 col:13
107+
108+
while.body.3: ; preds = %if.end
109+
%tobool4.old = icmp ne i32 %d.0, 0, !dbg !29 ; line:16 col:14
110+
br i1 %tobool4.old, label %if.end, label %while.end.14, !dbg !30 ; line:16 col:13
111+
112+
if.end: ; preds = %if.end.preheader, %while.body.3
113+
%tobool6 = icmp ne i32 %d.0, 0, !dbg !31 ; line:20 col:19
114+
%tobool7 = icmp ne i32 %d.0, 0, !dbg !32 ; line:20 col:23
115+
%tobool8 = icmp ne i32 %d.0, 0, !dbg !33 ; line:20 col:27
116+
%4 = select i1 %tobool6, i1 %tobool7, i1 %tobool8, !dbg !31 ; line:20 col:19
117+
br i1 %4, label %while.body.loopexit, label %while.body.3, !dbg !34 ; line:21 col:13
118+
119+
while.end.14: ; preds = %while.body.loopexit, %while.body.3, %entry
120+
ret void, !dbg !35 ; line:27 col:1
121+
}
122+
123+
; Function Attrs: nounwind readnone
124+
declare %dx.types.Handle @"dx.hl.createhandle..%dx.types.Handle (i32, %cbuffer_g*, i32)"(i32, %cbuffer_g*, i32) #1
125+
126+
; Function Attrs: nounwind readnone
127+
declare %dx.types.Handle @"dx.hl.annotatehandle..%dx.types.Handle (i32, %dx.types.Handle, %dx.types.ResourceProperties, %cbuffer_g)"(i32, %dx.types.Handle, %dx.types.ResourceProperties, %cbuffer_g) #1
128+
129+
; Function Attrs: nounwind readonly
130+
declare %dx.types.CBufRet.i32 @dx.op.cbufferLoadLegacy.i32(i32, %dx.types.Handle, i32) #2
131+
132+
; Function Attrs: nounwind readonly
133+
declare %dx.types.Handle @dx.op.createHandleForLib.cbuffer_g(i32, %cbuffer_g) #2
134+
135+
; Function Attrs: nounwind readnone
136+
declare %dx.types.Handle @dx.op.annotateHandle(i32, %dx.types.Handle, %dx.types.ResourceProperties) #1
137+
138+
attributes #0 = { nounwind }
139+
attributes #1 = { nounwind readnone }
140+
attributes #2 = { nounwind readonly }
141+
142+
!llvm.module.flags = !{!0}
143+
!pauseresume = !{!1}
144+
!llvm.ident = !{!2}
145+
!dx.version = !{!3}
146+
!dx.valver = !{!4}
147+
!dx.shaderModel = !{!5}
148+
!dx.resources = !{!6}
149+
!dx.typeAnnotations = !{!9, !12}
150+
!dx.entryPoints = !{!16}
151+
152+
!0 = !{i32 2, !"Debug Info Version", i32 3}
153+
!1 = !{!"hlsl-dxilemit", !"hlsl-dxilload"}
154+
!2 = !{!"dxc(private) 1.8.0.4514 (d9bd2a706-dirty)"}
155+
!3 = !{i32 1, i32 0}
156+
!4 = !{i32 1, i32 8}
157+
!5 = !{!"cs", i32 6, i32 0}
158+
!6 = !{null, null, !7, null}
159+
!7 = !{!8}
160+
!8 = !{i32 0, %cbuffer_g* @cbuffer_g, !"cbuffer_g", i32 0, i32 0, i32 1, i32 16, null}
161+
!9 = !{i32 0, %cbuffer_g undef, !10}
162+
!10 = !{i32 16, !11}
163+
!11 = !{i32 6, !"g", i32 3, i32 0, i32 7, i32 5}
164+
!12 = !{i32 1, void ()* @main, !13}
165+
!13 = !{!14}
166+
!14 = !{i32 1, !15, !15}
167+
!15 = !{}
168+
!16 = !{void ()* @main, !"main", null, !6, !17}
169+
!17 = !{i32 4, !18}
170+
!18 = !{i32 1, i32 1, i32 1}
171+
!19 = !DILocation(line: 8, column: 13, scope: !20)
172+
!20 = !DISubprogram(name: "main", scope: !21, file: !21, line: 6, type: !22, isLocal: false, isDefinition: true, scopeLine: 6, flags: DIFlagPrototyped, isOptimized: false, function: void ()* @main)
173+
!21 = !DIFile(filename: "/mnt/c/Users/amaiorano/Downloads/342545100/standalone_reduced.hlsl", directory: "")
174+
!22 = !DISubroutineType(types: !15)
175+
!23 = !DILocation(line: 8, column: 20, scope: !20)
176+
!24 = !DILocation(line: 9, column: 15, scope: !20)
177+
!25 = !DILocation(line: 11, column: 17, scope: !20)
178+
!26 = !DILocation(line: 11, column: 10, scope: !20)
179+
!27 = !DILocation(line: 12, column: 9, scope: !20)
180+
!28 = !DILocation(line: 19, column: 13, scope: !20)
181+
!29 = !DILocation(line: 16, column: 14, scope: !20)
182+
!30 = !DILocation(line: 16, column: 13, scope: !20)
183+
!31 = !DILocation(line: 20, column: 19, scope: !20)
184+
!32 = !DILocation(line: 20, column: 23, scope: !20)
185+
!33 = !DILocation(line: 20, column: 27, scope: !20)
186+
!34 = !DILocation(line: 21, column: 13, scope: !20)
187+
!35 = !DILocation(line: 27, column: 1, scope: !20)

0 commit comments

Comments
 (0)