Skip to content

Commit 8206fbd

Browse files
dneto0dneto0
authored
Fix RemoveUnstructuredLoopExits when an exiting edge jumps out multiple levels of loops. (microsoft#6668)
Before doing any major surgery on an exit from loop L, ensure that if an exit edge from L goes to block X, then X is in L's parent loop or no loop at all. Add test cases: - a reduced test case where the exiting block does not dominate its own loop latch. - a reduced test case where the exiting block is the latch for its own loop. This reproduces the assert triggered by the original HLSL. - the original HLSL that triggered this bug fix. - the intermediate module from the original HLSL, taken just before the attempt to remove unstructured loop exits.
1 parent 8408ae8 commit 8206fbd

File tree

6 files changed

+663
-0
lines changed

6 files changed

+663
-0
lines changed

‎lib/Transforms/Scalar/DxilRemoveUnstructuredLoopExits.cpp

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,10 +130,12 @@
130130
#include "llvm/IR/Constant.h"
131131
#include "llvm/IR/Instructions.h"
132132
#include "llvm/IR/IntrinsicInst.h"
133+
#include "llvm/IR/Module.h"
133134
#include "llvm/IR/Verifier.h"
134135
#include "llvm/Support/Debug.h"
135136
#include "llvm/Support/raw_ostream.h"
136137
#include "llvm/Transforms/Scalar.h"
138+
#include "llvm/Transforms/Utils/BasicBlockUtils.h"
137139
#include "llvm/Transforms/Utils/Local.h"
138140
#include "llvm/Transforms/Utils/LoopUtils.h"
139141

@@ -456,6 +458,79 @@ static SmallVector<Value_Info, 8> CollectExitValues(Value *new_exit_cond,
456458
return exit_values;
457459
}
458460

461+
// Ensures the branch from exiting_block to outside L escapes exactly one
462+
// level of loop nesting, and does not immediately jump into an otherwise
463+
// unrelated loop. Creates a downstream block as needed. If the exiting edge is
464+
// critical, it will be split. Updates dominator tree and loop info. Returns
465+
// true if any changes were made.
466+
static bool EnsureSingleLevelExit(Loop *L, LoopInfo *LI, DominatorTree *DT,
467+
BasicBlock *exiting_block) {
468+
BasicBlock *exit_block = GetExitBlockForExitingBlock(L, exiting_block);
469+
470+
Loop *exit_loop = LI->getLoopFor(exit_block);
471+
assert(L != exit_loop);
472+
473+
Loop *parent_loop = L->getParentLoop();
474+
if (parent_loop != exit_loop) {
475+
// Split the edge between the blocks, returning the newly created block.
476+
BasicBlock *new_bb = SplitEdge(exiting_block, exit_block, DT, LI);
477+
// The new block might be in the middle or at the end.
478+
BasicBlock *middle_bb;
479+
if (new_bb->getSingleSuccessor() == exit_block) {
480+
middle_bb = new_bb;
481+
} else {
482+
middle_bb = exit_block;
483+
exit_block = new_bb;
484+
}
485+
486+
// What loop does middle_bb end up in? SplitEdge has these cases:
487+
// If the edge was critical:
488+
// if source block is not in a loop: ruled out already
489+
// if dest block is not in a loop --> not in any loop.
490+
// if going from outer loop to inner loop: ruled out already
491+
// if going from inner loop to outer loop --> outer loop
492+
// if loops unrelated by containment -> the parent loop of the
493+
// destination block (which must be a loop header because we
494+
// assume irreducible loops).
495+
// If the edge was non-critcial:
496+
// If the exit block only had one incominge edge --> same loop as
497+
// destination block.
498+
// otherwise the exiting block had a single successor.
499+
// This is ruled out because the the exiting block ends with a
500+
// conditional branch, and so has two successors.
501+
502+
// Move the middle_block to the parent loop, if it exists.
503+
// If all goes well, the latch exit block will branch to it.
504+
// If the algorithm bails early, then there is no harm in putting
505+
// it in L's parent loop. At worst it will be an exiting block for
506+
// the parent loop.
507+
LI->removeBlock(middle_bb);
508+
if (parent_loop) {
509+
parent_loop->addBasicBlockToLoop(middle_bb, *LI);
510+
511+
// middle_bb block is now an exiting block, going from parent_loop to
512+
// exit_loop, which we know are different. Make sure it ends in a
513+
// in a conditional branch, as expected by the rest of the algorithm.
514+
auto *br = cast<BranchInst>(middle_bb->getTerminator());
515+
assert(!br->isConditional());
516+
auto *true_val = ConstantInt::getTrue(br->getContext());
517+
br->eraseFromParent();
518+
BasicBlock *parent_latch = parent_loop->getLoopLatch();
519+
BranchInst::Create(exit_block, parent_latch, true_val, middle_bb);
520+
// Fix phis in parent_latch
521+
for (Instruction &inst : *parent_latch) {
522+
PHINode *phi = dyn_cast<PHINode>(&inst);
523+
if (!phi)
524+
break;
525+
// We don't care about the values. The path is never taken.
526+
phi->addIncoming(GetDefaultValue(phi->getType()), middle_bb);
527+
}
528+
}
529+
return true;
530+
}
531+
return false;
532+
}
533+
459534
// Restructures exiting_block so its work, including its exit branch, is moved
460535
// to a block B that dominates the latch block. Let's call B the
461536
// newly-exiting-block.
@@ -465,6 +540,15 @@ static bool RemoveUnstructuredLoopExitsIteration(BasicBlock *exiting_block,
465540
Loop *L, LoopInfo *LI,
466541
DominatorTree *DT) {
467542
BasicBlock *latch = L->getLoopLatch();
543+
544+
if (EnsureSingleLevelExit(L, LI, DT, latch)) {
545+
// Exit early so we're forced to recompute exit blocks.
546+
return true;
547+
}
548+
if (EnsureSingleLevelExit(L, LI, DT, exiting_block)) {
549+
return true;
550+
}
551+
468552
BasicBlock *latch_exit = GetExitBlockForExitingBlock(L, latch);
469553
BasicBlock *exit_block = GetExitBlockForExitingBlock(L, exiting_block);
470554

‎test/HLSL/passes/dxil_remove_unstructured_loop_exits/exit-to-unrelated-loop.ll

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
; RUN: opt %s -analyze -loops | FileCheck -check-prefix=LOOPBEFORE %s
2+
; RUN: opt %s -dxil-remove-unstructured-loop-exits -o %t.bc
3+
; RUN: opt %t.bc -S | FileCheck %s
4+
; RUN: opt %t.bc -analyze -loops | FileCheck -check-prefix=LOOPAFTER %s
5+
6+
; The exiting edge goes to the header of a completely unrelated loop.
7+
; That edge is 'critical' in CFG terms, and will be split before attempting
8+
; to restructure the exit.
9+
10+
11+
; entry
12+
; | +---------+
13+
; v v |
14+
; header.1 --> if.1 -----> header.u2 -+
15+
; ^ | |
16+
; | | |
17+
; | +-------- endif.1 end.u2
18+
; | |
19+
; | v
20+
; latch.1
21+
; |
22+
; v
23+
; end
24+
;
25+
26+
; LOOPBEFORE-DAG: Loop at depth 1 containing: %header.u2<header><latch><exiting>
27+
; LOOPBEFORE-DAG: Loop at depth 1 containing: %header.1<header>,%if.1<exiting>,%endif.1,%latch.1<latch><exiting>
28+
; LOOPBEFORE-NOT: Loop at depth
29+
30+
; LOOPAFTER-DAG: Loop at depth 1 containing: %header.u2<header><latch><exiting>
31+
; LOOPAFTER-DAG: Loop at depth 1 containing: %header.1<header>,%if.1<exiting>,%endif.1,%latch.1<latch><exiting>
32+
; LOOPAFTER-NOT: Loop at depth
33+
34+
35+
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"
36+
target triple = "dxil-ms-dx"
37+
38+
39+
define void @main(i1 %cond) {
40+
entry:
41+
br label %header.1
42+
43+
header.1:
44+
br label %if.1
45+
46+
if.1:
47+
br i1 %cond, label %header.u2, label %endif.1
48+
49+
endif.1:
50+
br label %latch.1
51+
52+
latch.1:
53+
br i1 %cond, label %end, label %header.1
54+
55+
end:
56+
ret void
57+
58+
header.u2:
59+
br i1 %cond, label %end.u2, label %header.u2
60+
61+
end.u2:
62+
ret void
63+
}
64+
65+
66+
; CHECK: define void @main
67+
; CHECK: entry:
68+
; CHECK: br label %header.1
69+
70+
; CHECK: header.1:
71+
; CHECK: br label %if.1
72+
73+
; CHECK: if.1:
74+
; CHECK: br i1 %cond, label %if.1.header.u2_crit_edge, label %endif.1
75+
76+
; CHECK: if.1.header.u2_crit_edge:
77+
; CHECK: br label %header.u2
78+
79+
; CHECK: endif.1:
80+
; CHECK: br label %latch.1
81+
82+
; CHECK: latch.1:
83+
; CHECK: br i1 %cond, label %end, label %header.1
84+
85+
; CHECK: end:
86+
; CHECK: ret void
87+
88+
; CHECK: header.u2:
89+
; CHECK: br i1 %cond, label %end.u2, label %header.u2
90+
91+
; CHECK: end.u2:
92+
; CHECK: ret void
93+
; CHECK: }

‎test/HLSL/passes/dxil_remove_unstructured_loop_exits/multi-level-exit-from-latch.ll

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
; RUN: opt %s -analyze -loops | FileCheck -check-prefix=LOOPBEFORE %s
2+
; RUN: opt %s -dxil-remove-unstructured-loop-exits -o %t.bc
3+
; RUN: opt %t.bc -S | FileCheck %s
4+
; RUN: opt %t.bc -analyze -loops | FileCheck -check-prefix=LOOPAFTER %s
5+
6+
; The exiting edge from the latch block of the loop at depth 3 exits to the loop at depth 1.
7+
; This reproduces the original bug.
8+
;
9+
; Loop exits are 'dedicated', one of the LoopSimplifyForm criteria.
10+
11+
;
12+
; entry
13+
; |
14+
; v
15+
; header.1 --> header.2 --> header.3 --> if.3 -----> exiting.3
16+
; ^ ^ ^ | | |
17+
; | | | v | |
18+
; | | latch.3 <--- endif.3 <----+ |
19+
; | | | |
20+
; | | | v
21+
; | latch.2 <----------------------------- exit.3.to.2
22+
; | | |
23+
; | +-------- latch.2.exit |
24+
; | | |
25+
; | | v
26+
; | | latch.3.exit
27+
; | | |
28+
; | v |
29+
; latch.1 <-----------------+
30+
; |
31+
; v
32+
; end
33+
;
34+
35+
36+
; LOOPBEFORE: Loop at depth 1 containing: %header.1<header>,%header.2,%header.3,%if.3,%exiting.3,%endif.3,%latch.3,%latch.3.exit,%exit.3.to.2,%latch.2,%latch.2.exit,%latch.1<latch><exiting>
37+
; LOOPBEFORE-NEXT: Loop at depth 2 containing: %header.2<header>,%header.3,%if.3,%exiting.3,%endif.3,%latch.3<exiting>,%exit.3.to.2,%latch.2<latch><exiting>
38+
; LOOPBEFORE-NEXT: Loop at depth 3 containing: %header.3<header>,%if.3,%exiting.3<exiting>,%endif.3,%latch.3<latch><exiting>
39+
; no more loops expected
40+
; LOOPBEFORE-NOT: Loop at depth
41+
42+
; LOOPAFTER: Loop at depth 1 containing: %header.1<header>,%header.2,%header.3,%if.3,%exiting.3,%dx.struct_exit.new_exiting,%endif.3,%latch.3,%latch.3.exit,%0,%exit.3.to.2,%dx.struct_exit.new_exiting4,%latch.2,%latch.2.exit,%1,%latch.3.exit.split,%latch.1<latch><exiting>
43+
; LOOPAFTER-NEXT: Loop at depth 2 containing: %header.2<header>,%header.3,%if.3,%exiting.3,%dx.struct_exit.new_exiting,%endif.3,%latch.3,%latch.3.exit,%0,%exit.3.to.2,%dx.struct_exit.new_exiting4<exiting>,%latch.2<latch><exiting>
44+
; LOOPAFTER-NEXT: Loop at depth 3 containing: %header.3<header>,%if.3,%exiting.3,%dx.struct_exit.new_exiting<exiting>,%endif.3,%latch.3<latch><exiting>
45+
; no more loops expected
46+
; LOOPAFTER-NOT: Loop at depth
47+
48+
49+
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"
50+
target triple = "dxil-ms-dx"
51+
52+
53+
define void @main(i1 %cond) {
54+
entry:
55+
br label %header.1
56+
57+
header.1:
58+
br label %header.2
59+
60+
header.2:
61+
br label %header.3
62+
63+
header.3:
64+
br label %if.3
65+
66+
if.3:
67+
br i1 %cond, label %exiting.3, label %endif.3
68+
69+
exiting.3:
70+
%x3val = add i32 0, 0
71+
br i1 %cond, label %exit.3.to.2, label %endif.3
72+
73+
endif.3:
74+
br label %latch.3
75+
76+
latch.3:
77+
br i1 %cond, label %latch.3.exit, label %header.3
78+
79+
latch.3.exit:
80+
br label %latch.1
81+
82+
latch.2:
83+
%l2val = phi i32 [ %x3val, %exit.3.to.2 ]
84+
br i1 %cond, label %latch.2.exit, label %header.2
85+
86+
latch.2.exit:
87+
br label %latch.1
88+
89+
exit.3.to.2:
90+
br label %latch.2
91+
92+
latch.1:
93+
br i1 %cond, label %end, label %header.1
94+
95+
end:
96+
ret void
97+
}
98+
99+
100+
; CHECK: define void @main(i1 %cond) {
101+
; CHECK: entry:
102+
; CHECK: br label %header.1
103+
104+
; CHECK: header.1:
105+
; CHECK: br label %header.2
106+
107+
; CHECK: header.2:
108+
; CHECK: br label %header.3
109+
110+
; CHECK: header.3:
111+
; CHECK: br label %if.3
112+
113+
; CHECK: if.3:
114+
; CHECK: br i1 %cond, label %exiting.3, label %dx.struct_exit.new_exiting
115+
116+
; CHECK: exiting.3:
117+
; CHECK: %x3val = add i32 0, 0
118+
; CHECK: br label %dx.struct_exit.new_exiting
119+
120+
; CHECK: dx.struct_exit.new_exiting:
121+
; CHECK: %dx.struct_exit.prop1 = phi i1 [ %cond, %exiting.3 ], [ false, %if.3 ]
122+
; CHECK: %dx.struct_exit.prop = phi i32 [ %x3val, %exiting.3 ], [ 0, %if.3 ]
123+
; CHECK: br i1 %dx.struct_exit.prop1, label %latch.3.exit, label %endif.3
124+
125+
; CHECK: endif.3:
126+
; CHECK: br label %latch.3
127+
128+
; CHECK: latch.3:
129+
; CHECK: br i1 %cond, label %latch.3.exit, label %header.3
130+
131+
; CHECK: latch.3.exit:
132+
; CHECK: %dx.struct_exit.exit_cond_lcssa = phi i1 [ %dx.struct_exit.prop1, %dx.struct_exit.new_exiting ], [ false, %latch.3 ]
133+
; CHECK: %dx.struct_exit.val_lcssa = phi i32 [ %dx.struct_exit.prop, %dx.struct_exit.new_exiting ], [ 0, %latch.3 ]
134+
; CHECK: br i1 %dx.struct_exit.exit_cond_lcssa, label %exit.3.to.2, label %0
135+
136+
; CHECK: <label>:0
137+
; CHECK: br label %dx.struct_exit.new_exiting4
138+
139+
; CHECK: latch.3.exit.split:
140+
; CHECK: br label %latch.1
141+
142+
; CHECK: dx.struct_exit.new_exiting4:
143+
; CHECK: %dx.struct_exit.prop3 = phi i1 [ true, %0 ], [ false, %exit.3.to.2 ]
144+
; CHECK: %l2val = phi i32 [ %x3val.lcssa, %exit.3.to.2 ], [ 0, %0 ]
145+
; CHECK: br i1 %dx.struct_exit.prop3, label %latch.2.exit, label %latch.2
146+
147+
; CHECK: latch.2:
148+
; CHECK: br i1 %cond, label %latch.2.exit, label %header.2
149+
150+
; CHECK: latch.2.exit:
151+
; CHECK: %dx.struct_exit.exit_cond_lcssa6 = phi i1 [ %dx.struct_exit.prop3, %dx.struct_exit.new_exiting4 ], [ false, %latch.2 ]
152+
; CHECK: br i1 %dx.struct_exit.exit_cond_lcssa6, label %latch.3.exit.split, label %1
153+
154+
; CHECK: <label>:1
155+
; CHECK: br label %latch.1
156+
157+
; CHECK: exit.3.to.2:
158+
; CHECK: %x3val.lcssa = phi i32 [ %dx.struct_exit.val_lcssa, %latch.3.exit ]
159+
; CHECK: br label %dx.struct_exit.new_exiting4
160+
161+
; CHECK: latch.1:
162+
; CHECK: br i1 %cond, label %end, label %header.1
163+
164+
; CHECK: end:
165+
; CHECK: ret void
166+
; CHECK: }

0 commit comments

Comments
 (0)