[llvm] 2fda200 - [InstCombine] Canonicalize phi order for newly inserted nodes
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 27 00:56:32 PDT 2023
Author: Nikita Popov
Date: 2023-09-27T09:56:23+02:00
New Revision: 2fda200cd8c15773c640b943c28b94fcdb874646
URL: https://github.com/llvm/llvm-project/commit/2fda200cd8c15773c640b943c28b94fcdb874646
DIFF: https://github.com/llvm/llvm-project/commit/2fda200cd8c15773c640b943c28b94fcdb874646.diff
LOG: [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 https://github.com/llvm/llvm-project/issues/46688.
Added:
Modified:
llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
llvm/test/Transforms/InstCombine/binop-phi-operands.ll
llvm/test/Transforms/InstCombine/merging-multiple-stores-into-successor.ll
llvm/test/Transforms/InstCombine/select.ll
Removed:
################################################################################
diff --git a/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h b/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
index b16a60c423ab103..c1cea5649f769ab 100644
--- a/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
+++ b/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
@@ -86,6 +86,9 @@ class LLVM_LIBRARY_VISIBILITY InstCombiner {
/// Edges that are known to never be taken.
SmallDenseSet<std::pair<BasicBlock *, BasicBlock *>, 8> DeadEdges;
+ /// Order of predecessors to canonicalize phi nodes towards.
+ SmallDenseMap<BasicBlock *, SmallVector<BasicBlock *>, 8> PredOrder;
+
public:
InstCombiner(InstructionWorklist &Worklist, BuilderTy &Builder,
bool MinimizeSize, AAResults *AA, AssumptionCache &AC,
diff --git a/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp b/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
index 1ac4c2a80436e3d..00115abf6500597 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
@@ -1512,11 +1512,12 @@ Instruction *InstCombinerImpl::visitPHINode(PHINode &PN) {
// the blocks in the same order. This will help identical PHIs be eliminated
// by other passes. Other passes shouldn't depend on this for correctness
// however.
- PHINode *FirstPN = cast<PHINode>(PN.getParent()->begin());
- if (&PN != FirstPN)
- for (unsigned I = 0, E = FirstPN->getNumIncomingValues(); I != E; ++I) {
+ auto Res = PredOrder.try_emplace(PN.getParent());
+ if (!Res.second) {
+ const auto &Preds = Res.first->second;
+ for (unsigned I = 0, E = PN.getNumIncomingValues(); I != E; ++I) {
BasicBlock *BBA = PN.getIncomingBlock(I);
- BasicBlock *BBB = FirstPN->getIncomingBlock(I);
+ BasicBlock *BBB = Preds[I];
if (BBA != BBB) {
Value *VA = PN.getIncomingValue(I);
unsigned J = PN.getBasicBlockIndex(BBB);
@@ -1531,6 +1532,10 @@ Instruction *InstCombinerImpl::visitPHINode(PHINode &PN) {
// this in this case.
}
}
+ } else {
+ // Remember the block order of the first encountered phi node.
+ append_range(Res.first->second, PN.blocks());
+ }
// Is there an identical PHI node in this basic block?
for (PHINode &IdenticalPN : PN.getParent()->phis()) {
diff --git a/llvm/test/Transforms/InstCombine/binop-phi-operands.ll b/llvm/test/Transforms/InstCombine/binop-phi-operands.ll
index 9cc1a418cc57253..9e049837b0352b0 100644
--- a/llvm/test/Transforms/InstCombine/binop-phi-operands.ll
+++ b/llvm/test/Transforms/InstCombine/binop-phi-operands.ll
@@ -39,7 +39,7 @@ define i32 @add_const_incoming0_nonspeculative(i1 %b, i32 %x, i32 %y) {
; CHECK-NEXT: [[TMP0:%.*]] = add i32 [[X:%.*]], [[Y:%.*]]
; CHECK-NEXT: br label [[THEN]]
; CHECK: then:
-; CHECK-NEXT: [[R:%.*]] = phi i32 [ [[TMP0]], [[IF]] ], [ 59, [[ENTRY:%.*]] ]
+; CHECK-NEXT: [[R:%.*]] = phi i32 [ 59, [[ENTRY:%.*]] ], [ [[TMP0]], [[IF]] ]
; CHECK-NEXT: ret i32 [[R]]
;
entry:
@@ -210,7 +210,7 @@ define i64 @or_const_incoming01(i1 %b, i64 %x, i64 %y) {
; CHECK-NEXT: [[TMP0:%.*]] = or i64 [[X:%.*]], [[Y:%.*]]
; CHECK-NEXT: br label [[THEN]]
; CHECK: then:
-; CHECK-NEXT: [[R:%.*]] = phi i64 [ [[TMP0]], [[IF]] ], [ 19, [[ENTRY:%.*]] ]
+; CHECK-NEXT: [[R:%.*]] = phi i64 [ 19, [[ENTRY:%.*]] ], [ [[TMP0]], [[IF]] ]
; CHECK-NEXT: ret i64 [[R]]
;
entry:
@@ -285,7 +285,7 @@ define i8 @ashr_const_incoming0(i1 %b, i8 %x, i8 %y) {
; CHECK-NEXT: [[TMP0:%.*]] = ashr i8 [[X:%.*]], [[Y:%.*]]
; CHECK-NEXT: br label [[THEN]]
; CHECK: then:
-; CHECK-NEXT: [[R:%.*]] = phi i8 [ [[TMP0]], [[IF]] ], [ 5, [[ENTRY:%.*]] ]
+; CHECK-NEXT: [[R:%.*]] = phi i8 [ 5, [[ENTRY:%.*]] ], [ [[TMP0]], [[IF]] ]
; CHECK-NEXT: ret i8 [[R]]
;
entry:
diff --git a/llvm/test/Transforms/InstCombine/merging-multiple-stores-into-successor.ll b/llvm/test/Transforms/InstCombine/merging-multiple-stores-into-successor.ll
index b35fceef372c663..58174f21f767f96 100644
--- a/llvm/test/Transforms/InstCombine/merging-multiple-stores-into-successor.ll
+++ b/llvm/test/Transforms/InstCombine/merging-multiple-stores-into-successor.ll
@@ -1,8 +1,7 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt < %s -passes='instcombine<no-verify-fixpoint>' -S | FileCheck %s
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
-; FIXME: This currently doesn't reach a fix point, because we don't
-; canonicalize the operand order of newly added phi nodes.
+target datalayout = "n32"
@var_7 = external global i8, align 1
@var_1 = external global i32, align 4
@@ -305,3 +304,53 @@ sink:
%val = load ptr, ptr %alloca
ret ptr %val
}
+
+
+define void @pr46688(i1 %cond, i32 %x, i16 %d, ptr %p1, ptr %p2) {
+; CHECK-LABEL: @pr46688(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br i1 [[COND:%.*]], label [[IF:%.*]], label [[ELSE:%.*]]
+; CHECK: if:
+; CHECK-NEXT: br label [[EXIT:%.*]]
+; CHECK: else:
+; CHECK-NEXT: br label [[EXIT]]
+; CHECK: exit:
+; CHECK-NEXT: [[DONV_PN:%.*]] = zext i16 [[D:%.*]] to i32
+; CHECK-NEXT: [[THR_PN:%.*]] = lshr i32 [[DONV_PN]], [[X:%.*]]
+; CHECK-NEXT: [[THR1_PN:%.*]] = lshr i32 [[THR_PN]], [[X]]
+; CHECK-NEXT: [[THR2_PN:%.*]] = lshr i32 [[THR1_PN]], [[X]]
+; CHECK-NEXT: [[STOREMERGE:%.*]] = lshr i32 [[THR2_PN]], [[X]]
+; CHECK-NEXT: [[STOREMERGE1:%.*]] = trunc i32 [[STOREMERGE]] to i16
+; CHECK-NEXT: store i16 [[STOREMERGE1]], ptr [[P1:%.*]], align 2
+; CHECK-NEXT: store i32 [[STOREMERGE]], ptr [[P2:%.*]], align 4
+; CHECK-NEXT: ret void
+;
+entry:
+ br i1 %cond, label %if, label %else
+
+if:
+ %conv = zext i16 %d to i32
+ %shr = lshr i32 %conv, %x
+ %shr1 = lshr i32 %shr, %x
+ %shr2 = lshr i32 %shr1, %x
+ %shr3 = lshr i32 %shr2, %x
+ %conv4 = trunc i32 %shr3 to i16
+ store i16 %conv4, ptr %p1, align 2
+ %conv5 = and i32 %shr3, 65535
+ store i32 %conv5, ptr %p2, align 4
+ br label %exit
+
+else:
+ %donv = zext i16 %d to i32
+ %thr = lshr i32 %donv, %x
+ %thr1 = lshr i32 %thr, %x
+ %thr2 = lshr i32 %thr1, %x
+ %thr3 = lshr i32 %thr2, %x
+ %donv4 = trunc i32 %thr3 to i16
+ store i16 %donv4, ptr %p1, align 2
+ store i32 %thr3, ptr %p2, align 4
+ br label %exit
+
+exit:
+ ret void
+}
diff --git a/llvm/test/Transforms/InstCombine/select.ll b/llvm/test/Transforms/InstCombine/select.ll
index 82e163d93f0f1ab..b3764cfb97d407b 100644
--- a/llvm/test/Transforms/InstCombine/select.ll
+++ b/llvm/test/Transforms/InstCombine/select.ll
@@ -487,7 +487,7 @@ define i32 @test26(i1 %cond) {
; CHECK: jump:
; CHECK-NEXT: br label [[RET]]
; CHECK: ret:
-; CHECK-NEXT: [[B:%.*]] = phi i32 [ 10, [[JUMP]] ], [ 20, [[ENTRY:%.*]] ]
+; CHECK-NEXT: [[B:%.*]] = phi i32 [ 20, [[ENTRY:%.*]] ], [ 10, [[JUMP]] ]
; CHECK-NEXT: ret i32 [[B]]
;
entry:
@@ -508,7 +508,7 @@ define i32 @test26_logical(i1 %cond) {
; CHECK: jump:
; CHECK-NEXT: br label [[RET]]
; CHECK: ret:
-; CHECK-NEXT: [[B:%.*]] = phi i32 [ 10, [[JUMP]] ], [ 20, [[ENTRY:%.*]] ]
+; CHECK-NEXT: [[B:%.*]] = phi i32 [ 20, [[ENTRY:%.*]] ], [ 10, [[JUMP]] ]
; CHECK-NEXT: ret i32 [[B]]
;
entry:
@@ -2113,7 +2113,7 @@ define i32 @select_phi_same_condition(i1 %cond, i32 %x, i32 %y, i32 %z) {
; CHECK: if.false:
; CHECK-NEXT: br label [[MERGE]]
; CHECK: merge:
-; CHECK-NEXT: [[S:%.*]] = phi i32 [ [[Z:%.*]], [[IF_FALSE]] ], [ [[X:%.*]], [[IF_TRUE]] ]
+; CHECK-NEXT: [[S:%.*]] = phi i32 [ [[X:%.*]], [[IF_TRUE]] ], [ [[Z:%.*]], [[IF_FALSE]] ]
; CHECK-NEXT: ret i32 [[S]]
;
entry:
@@ -2250,7 +2250,7 @@ define i32 @select_phi_same_condition_switch(i1 %cond, i32 %x, i32 %y) {
; CHECK: if.false:
; CHECK-NEXT: br label [[MERGE]]
; CHECK: merge:
-; CHECK-NEXT: [[S:%.*]] = phi i32 [ [[Y:%.*]], [[IF_FALSE]] ], [ [[X]], [[IF_TRUE]] ], [ [[X]], [[IF_TRUE]] ]
+; CHECK-NEXT: [[S:%.*]] = phi i32 [ [[X]], [[IF_TRUE]] ], [ [[X]], [[IF_TRUE]] ], [ [[Y:%.*]], [[IF_FALSE]] ]
; CHECK-NEXT: ret i32 [[S]]
;
entry:
@@ -2287,7 +2287,7 @@ define i32 @transit_
diff erent_values_through_phi(i1 %cond, i1 %cond2) {
; CHECK: if.false:
; CHECK-NEXT: br label [[MERGE]]
; CHECK: merge:
-; CHECK-NEXT: [[S:%.*]] = phi i32 [ 3, [[IF_FALSE]] ], [ 2, [[IF_TRUE_2]] ], [ 1, [[IF_TRUE_1]] ]
+; CHECK-NEXT: [[S:%.*]] = phi i32 [ 1, [[IF_TRUE_1]] ], [ 2, [[IF_TRUE_2]] ], [ 3, [[IF_FALSE]] ]
; CHECK-NEXT: ret i32 [[S]]
; CHECK: exit:
; CHECK-NEXT: ret i32 0
@@ -2321,7 +2321,7 @@ define i32 @select_phi_degenerate(i1 %cond, i1 %cond2) {
; CHECK-NEXT: entry:
; CHECK-NEXT: br i1 [[COND:%.*]], label [[LOOP:%.*]], label [[EXIT:%.*]]
; CHECK: loop:
-; CHECK-NEXT: [[SELECT:%.*]] = phi i32 [ [[IV_INC:%.*]], [[LOOP]] ], [ 0, [[ENTRY:%.*]] ]
+; CHECK-NEXT: [[SELECT:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_INC:%.*]], [[LOOP]] ]
; CHECK-NEXT: [[IV_INC]] = add i32 [[SELECT]], 1
; CHECK-NEXT: br i1 [[COND2:%.*]], label [[LOOP]], label [[EXIT2:%.*]]
; CHECK: exit:
@@ -2416,7 +2416,7 @@ define i32 @test_select_into_phi_not_idom_inverted(i1 %cond, i32 %A, i32 %B) {
; CHECK: if.false:
; CHECK-NEXT: br label [[MERGE]]
; CHECK: merge:
-; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[B:%.*]], [[IF_FALSE]] ], [ [[A:%.*]], [[IF_TRUE]] ]
+; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[A:%.*]], [[IF_TRUE]] ], [ [[B:%.*]], [[IF_FALSE]] ]
; CHECK-NEXT: br label [[EXIT:%.*]]
; CHECK: exit:
; 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)
; CHECK: if.false:
; CHECK-NEXT: br label [[MERGE]]
; CHECK: merge:
-; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[B:%.*]], [[IF_FALSE]] ], [ [[A:%.*]], [[IF_TRUE]] ]
+; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[A:%.*]], [[IF_TRUE]] ], [ [[B:%.*]], [[IF_FALSE]] ]
; CHECK-NEXT: br label [[EXIT:%.*]]
; CHECK: exit:
; 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 %
; CHECK: if.false:
; CHECK-NEXT: br label [[MERGE]]
; CHECK: merge:
-; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[A:%.*]], [[IF_FALSE]] ], [ [[C]], [[IF_TRUE]] ]
+; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[C]], [[IF_TRUE]] ], [ [[A:%.*]], [[IF_FALSE]] ]
; CHECK-NEXT: br label [[EXIT:%.*]]
; CHECK: exit:
; 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 %
; CHECK-NEXT: [[C:%.*]] = load i32, ptr [[P:%.*]], align 4
; CHECK-NEXT: br label [[MERGE]]
; CHECK: merge:
-; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[C]], [[IF_FALSE]] ], [ [[B:%.*]], [[IF_TRUE]] ]
+; CHECK-NEXT: [[SEL:%.*]] = phi i32 [ [[B:%.*]], [[IF_TRUE]] ], [ [[C]], [[IF_FALSE]] ]
; CHECK-NEXT: br label [[EXIT:%.*]]
; CHECK: exit:
; CHECK-NEXT: ret i32 [[SEL]]
More information about the llvm-commits
mailing list