Skip to content

Commit 2fda200

Browse files
nikicnikic
committed
[InstCombine] Canonicalize phi order for newly inserted nodes
When new phi nodes are inserted at the start of the block, the order of these does not get canonicalized, as we pick the first phi node to canonicalize towards (and the other phi nodes may have already been visited). This results in phi nodes not being deduplicated and thus a fix-point verification failure. Fix this by remembering the predecessors of the first phi node we encounter -- this will usually result in the same order we picked previously. I also considered using predecessors() order instead, but that causes substantial test fallout. Additionally the predecessors() order tends to be the reverse of the "natural" order. Fixes #46688.
1 parent f703774 commit 2fda200

File tree

5 files changed

+77
-20
lines changed

5 files changed

+77
-20
lines changed

‎llvm/include/llvm/Transforms/InstCombine/InstCombiner.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,9 @@ class LLVM_LIBRARY_VISIBILITY InstCombiner {
8686
/// Edges that are known to never be taken.
8787
SmallDenseSet<std::pair<BasicBlock *, BasicBlock *>, 8> DeadEdges;
8888

89+
/// Order of predecessors to canonicalize phi nodes towards.
90+
SmallDenseMap<BasicBlock *, SmallVector<BasicBlock *>, 8> PredOrder;
91+
8992
public:
9093
InstCombiner(InstructionWorklist &Worklist, BuilderTy &Builder,
9194
bool MinimizeSize, AAResults *AA, AssumptionCache &AC,

‎llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1512,11 +1512,12 @@ Instruction *InstCombinerImpl::visitPHINode(PHINode &PN) {
15121512
// the blocks in the same order. This will help identical PHIs be eliminated
15131513
// by other passes. Other passes shouldn't depend on this for correctness
15141514
// however.
1515-
PHINode *FirstPN = cast<PHINode>(PN.getParent()->begin());
1516-
if (&PN != FirstPN)
1517-
for (unsigned I = 0, E = FirstPN->getNumIncomingValues(); I != E; ++I) {
1515+
auto Res = PredOrder.try_emplace(PN.getParent());
1516+
if (!Res.second) {
1517+
const auto &Preds = Res.first->second;
1518+
for (unsigned I = 0, E = PN.getNumIncomingValues(); I != E; ++I) {
15181519
BasicBlock *BBA = PN.getIncomingBlock(I);
1519-
BasicBlock *BBB = FirstPN->getIncomingBlock(I);
1520+
BasicBlock *BBB = Preds[I];
15201521
if (BBA != BBB) {
15211522
Value *VA = PN.getIncomingValue(I);
15221523
unsigned J = PN.getBasicBlockIndex(BBB);
@@ -1531,6 +1532,10 @@ Instruction *InstCombinerImpl::visitPHINode(PHINode &PN) {
15311532
// this in this case.
15321533
}
15331534
}
1535+
} else {
1536+
// Remember the block order of the first encountered phi node.
1537+
append_range(Res.first->second, PN.blocks());
1538+
}
15341539

15351540
// Is there an identical PHI node in this basic block?
15361541
for (PHINode &IdenticalPN : PN.getParent()->phis()) {

‎llvm/test/Transforms/InstCombine/binop-phi-operands.ll

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ define i32 @add_const_incoming0_nonspeculative(i1 %b, i32 %x, i32 %y) {
3939
; CHECK-NEXT: [[TMP0:%.*]] = add i32 [[X:%.*]], [[Y:%.*]]
4040
; CHECK-NEXT: br label [[THEN]]
4141
; CHECK: then:
42-
; CHECK-NEXT: [[R:%.*]] = phi i32 [ [[TMP0]], [[IF]] ], [ 59, [[ENTRY:%.*]] ]
42+
; CHECK-NEXT: [[R:%.*]] = phi i32 [ 59, [[ENTRY:%.*]] ], [ [[TMP0]], [[IF]] ]
4343
; CHECK-NEXT: ret i32 [[R]]
4444
;
4545
entry:
@@ -210,7 +210,7 @@ define i64 @or_const_incoming01(i1 %b, i64 %x, i64 %y) {
210210
; CHECK-NEXT: [[TMP0:%.*]] = or i64 [[X:%.*]], [[Y:%.*]]
211211
; CHECK-NEXT: br label [[THEN]]
212212
; CHECK: then:
213-
; CHECK-NEXT: [[R:%.*]] = phi i64 [ [[TMP0]], [[IF]] ], [ 19, [[ENTRY:%.*]] ]
213+
; CHECK-NEXT: [[R:%.*]] = phi i64 [ 19, [[ENTRY:%.*]] ], [ [[TMP0]], [[IF]] ]
214214
; CHECK-NEXT: ret i64 [[R]]
215215
;
216216
entry:
@@ -285,7 +285,7 @@ define i8 @ashr_const_incoming0(i1 %b, i8 %x, i8 %y) {
285285
; CHECK-NEXT: [[TMP0:%.*]] = ashr i8 [[X:%.*]], [[Y:%.*]]
286286
; CHECK-NEXT: br label [[THEN]]
287287
; CHECK: then:
288-
; CHECK-NEXT: [[R:%.*]] = phi i8 [ [[TMP0]], [[IF]] ], [ 5, [[ENTRY:%.*]] ]
288+
; CHECK-NEXT: [[R:%.*]] = phi i8 [ 5, [[ENTRY:%.*]] ], [ [[TMP0]], [[IF]] ]
289289
; CHECK-NEXT: ret i8 [[R]]
290290
;
291291
entry:

‎llvm/test/Transforms/InstCombine/merging-multiple-stores-into-successor.ll

Lines changed: 52 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
2-
; RUN: opt < %s -passes='instcombine<no-verify-fixpoint>' -S | FileCheck %s
2+
; RUN: opt < %s -passes=instcombine -S | FileCheck %s
33

4-
; FIXME: This currently doesn't reach a fix point, because we don't
5-
; canonicalize the operand order of newly added phi nodes.
4+
target datalayout = "n32"
65

76
@var_7 = external global i8, align 1
87
@var_1 = external global i32, align 4
@@ -305,3 +304,53 @@ sink:
305304
%val = load ptr, ptr %alloca
306305
ret ptr %val
307306
}
307+
308+
309+
define void @pr46688(i1 %cond, i32 %x, i16 %d, ptr %p1, ptr %p2) {
310+
; CHECK-LABEL: @pr46688(
311+
; CHECK-NEXT: entry:
312+
; CHECK-NEXT: br i1 [[COND:%.*]], label [[IF:%.*]], label [[ELSE:%.*]]
313+
; CHECK: if:
314+
; CHECK-NEXT: br label [[EXIT:%.*]]
315+
; CHECK: else:
316+
; CHECK-NEXT: br label [[EXIT]]
317+
; CHECK: exit:
318+
; CHECK-NEXT: [[DONV_PN:%.*]] = zext i16 [[D:%.*]] to i32
319+
; CHECK-NEXT: [[THR_PN:%.*]] = lshr i32 [[DONV_PN]], [[X:%.*]]
320+
; CHECK-NEXT: [[THR1_PN:%.*]] = lshr i32 [[THR_PN]], [[X]]
321+
; CHECK-NEXT: [[THR2_PN:%.*]] = lshr i32 [[THR1_PN]], [[X]]
322+
; CHECK-NEXT: [[STOREMERGE:%.*]] = lshr i32 [[THR2_PN]], [[X]]
323+
; CHECK-NEXT: [[STOREMERGE1:%.*]] = trunc i32 [[STOREMERGE]] to i16
324+
; CHECK-NEXT: store i16 [[STOREMERGE1]], ptr [[P1:%.*]], align 2
325+
; CHECK-NEXT: store i32 [[STOREMERGE]], ptr [[P2:%.*]], align 4
326+
; CHECK-NEXT: ret void
327+
;
328+
entry:
329+
br i1 %cond, label %if, label %else
330+
331+
if:
332+
%conv = zext i16 %d to i32
333+
%shr = lshr i32 %conv, %x
334+
%shr1 = lshr i32 %shr, %x
335+
%shr2 = lshr i32 %shr1, %x
336+
%shr3 = lshr i32 %shr2, %x
337+
%conv4 = trunc i32 %shr3 to i16
338+
store i16 %conv4, ptr %p1, align 2
339+
%conv5 = and i32 %shr3, 65535
340+
store i32 %conv5, ptr %p2, align 4
341+
br label %exit
342+
343+
else:
344+
%donv = zext i16 %d to i32
345+
%thr = lshr i32 %donv, %x
346+
%thr1 = lshr i32 %thr, %x
347+
%thr2 = lshr i32 %thr1, %x
348+
%thr3 = lshr i32 %thr2, %x
349+
%donv4 = trunc i32 %thr3 to i16
350+
store i16 %donv4, ptr %p1, align 2
351+
store i32 %thr3, ptr %p2, align 4
352+
br label %exit
353+
354+
exit:
355+
ret void
356+
}

‎llvm/test/Transforms/InstCombine/select.ll

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,7 @@ define i32 @test26(i1 %cond) {
487487
; CHECK: jump:
488488
; CHECK-NEXT: br label [[RET]]
489489
; CHECK: ret:
490-
; CHECK-NEXT: [[B:%.*]] = phi i32 [ 10, [[JUMP]] ], [ 20, [[ENTRY:%.*]] ]
490+
; CHECK-NEXT: [[B:%.*]] = phi i32 [ 20, [[ENTRY:%.*]] ], [ 10, [[JUMP]] ]
491491
; CHECK-NEXT: ret i32 [[B]]
492492
;
493493
entry:
@@ -508,7 +508,7 @@ define i32 @test26_logical(i1 %cond) {
508508
; CHECK: jump:
509509
; CHECK-NEXT: br label [[RET]]
510510
; CHECK: ret:
511-
; CHECK-NEXT: [[B:%.*]] = phi i32 [ 10, [[JUMP]] ], [ 20, [[ENTRY:%.*]] ]
511+
; CHECK-NEXT: [[B:%.*]] = phi i32 [ 20, [[ENTRY:%.*]] ], [ 10, [[JUMP]] ]
512512
; CHECK-NEXT: ret i32 [[B]]
513513
;
514514
entry:
@@ -2113,7 +2113,7 @@ define i32 @select_phi_same_condition(i1 %cond, i32 %x, i32 %y, i32 %z) {
21132113
; CHECK: if.false:
21142114
; CHECK-NEXT: br label [[MERGE]]
21152115
; CHECK: merge:
2116-
; CHECK-NEXT: [[S:%.*]] = phi i32 [ [[Z:%.*]], [[IF_FALSE]] ], [ [[X:%.*]], [[IF_TRUE]] ]
2116+
; CHECK-NEXT: [[S:%.*]] = phi i32 [ [[X:%.*]], [[IF_TRUE]] ], [ [[Z:%.*]], [[IF_FALSE]] ]
21172117
; CHECK-NEXT: ret i32 [[S]]
21182118
;
21192119
entry:
@@ -2250,7 +2250,7 @@ define i32 @select_phi_same_condition_switch(i1 %cond, i32 %x, i32 %y) {
22502250
; CHECK: if.false:
22512251
; CHECK-NEXT: br label [[MERGE]]
22522252
; CHECK: merge:
2253-
; CHECK-NEXT: [[S:%.*]] = phi i32 [ [[Y:%.*]], [[IF_FALSE]] ], [ [[X]], [[IF_TRUE]] ], [ [[X]], [[IF_TRUE]] ]
2253+
; CHECK-NEXT: [[S:%.*]] = phi i32 [ [[X]], [[IF_TRUE]] ], [ [[X]], [[IF_TRUE]] ], [ [[Y:%.*]], [[IF_FALSE]] ]
22542254
; CHECK-NEXT: ret i32 [[S]]
22552255
;
22562256
entry:
@@ -2287,7 +2287,7 @@ define i32 @transit_different_values_through_phi(i1 %cond, i1 %cond2) {
22872287
; CHECK: if.false:
22882288
; CHECK-NEXT: br label [[MERGE]]
22892289
; CHECK: merge:
2290-
; CHECK-NEXT: [[S:%.*]] = phi i32 [ 3, [[IF_FALSE]] ], [ 2, [[IF_TRUE_2]] ], [ 1, [[IF_TRUE_1]] ]
2290+
; CHECK-NEXT: [[S:%.*]] = phi i32 [ 1, [[IF_TRUE_1]] ], [ 2, [[IF_TRUE_2]] ], [ 3, [[IF_FALSE]] ]
22912291
; CHECK-NEXT: ret i32 [[S]]
22922292
; CHECK: exit:
22932293
; CHECK-NEXT: ret i32 0
@@ -2321,7 +2321,7 @@ define i32 @select_phi_degenerate(i1 %cond, i1 %cond2) {
23212321
; CHECK-NEXT: entry:
23222322
; CHECK-NEXT: br i1 [[COND:%.*]], label [[LOOP:%.*]], label [[EXIT:%.*]]
23232323
; CHECK: loop:
2324-
; CHECK-NEXT: [[SELECT:%.*]] = phi i32 [ [[IV_INC:%.*]], [[LOOP]] ], [ 0, [[ENTRY:%.*]] ]
2324+
; CHECK-NEXT: [[SELECT:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_INC:%.*]], [[LOOP]] ]
23252325
; CHECK-NEXT: [[IV_INC]] = add i32 [[SELECT]], 1
23262326
; CHECK-NEXT: br i1 [[COND2:%.*]], label [[LOOP]], label [[EXIT2:%.*]]
23272327
; CHECK: exit:
@@ -2416,7 +2416,7 @@ define i32 @test_select_into_phi_not_idom_inverted(i1 %cond, i32 %A, i32 %B) {
24162416
; CHECK: if.false:
24172417
; CHECK-NEXT: br label [[MERGE]]
24182418
; CHECK: merge:
2419-
; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[B:%.*]], [[IF_FALSE]] ], [ [[A:%.*]], [[IF_TRUE]] ]
2419+
; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[A:%.*]], [[IF_TRUE]] ], [ [[B:%.*]], [[IF_FALSE]] ]
24202420
; CHECK-NEXT: br label [[EXIT:%.*]]
24212421
; CHECK: exit:
24222422
; CHECK-NEXT: ret i32 [[SEL]]
@@ -2449,7 +2449,7 @@ define i32 @test_select_into_phi_not_idom_inverted_2(i1 %cond, i32 %A, i32 %B)
24492449
; CHECK: if.false:
24502450
; CHECK-NEXT: br label [[MERGE]]
24512451
; CHECK: merge:
2452-
; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[B:%.*]], [[IF_FALSE]] ], [ [[A:%.*]], [[IF_TRUE]] ]
2452+
; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[A:%.*]], [[IF_TRUE]] ], [ [[B:%.*]], [[IF_FALSE]] ]
24532453
; CHECK-NEXT: br label [[EXIT:%.*]]
24542454
; CHECK: exit:
24552455
; CHECK-NEXT: ret i32 [[SEL]]
@@ -2483,7 +2483,7 @@ define i32 @test_select_into_phi_not_idom_no_dom_input_1(i1 %cond, i32 %A, i32 %
24832483
; CHECK: if.false:
24842484
; CHECK-NEXT: br label [[MERGE]]
24852485
; CHECK: merge:
2486-
; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[A:%.*]], [[IF_FALSE]] ], [ [[C]], [[IF_TRUE]] ]
2486+
; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[C]], [[IF_TRUE]] ], [ [[A:%.*]], [[IF_FALSE]] ]
24872487
; CHECK-NEXT: br label [[EXIT:%.*]]
24882488
; CHECK: exit:
24892489
; CHECK-NEXT: ret i32 [[SEL]]
@@ -2517,7 +2517,7 @@ define i32 @test_select_into_phi_not_idom_no_dom_input_2(i1 %cond, i32 %A, i32 %
25172517
; CHECK-NEXT: [[C:%.*]] = load i32, ptr [[P:%.*]], align 4
25182518
; CHECK-NEXT: br label [[MERGE]]
25192519
; CHECK: merge:
2520-
; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[C]], [[IF_FALSE]] ], [ [[B:%.*]], [[IF_TRUE]] ]
2520+
; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[B:%.*]], [[IF_TRUE]] ], [ [[C]], [[IF_FALSE]] ]
25212521
; CHECK-NEXT: br label [[EXIT:%.*]]
25222522
; CHECK: exit:
25232523
; CHECK-NEXT: ret i32 [[SEL]]

0 commit comments

Comments
 (0)