[llvm] [InstCombine] Generalize and consolidate phi translation check (PR #106051)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 27 11:50:36 PDT 2024
https://github.com/nikic updated https://github.com/llvm/llvm-project/pull/106051
>From 940223de17c42dde6982b478d14262aaeec3954a Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Thu, 22 Aug 2024 17:03:02 +0200
Subject: [PATCH 1/2] [InstCombine] Generalize and consolidate phi translation
check
The foldOpIntoPhi() transforms requires all operands to be
phi-translatable. This can be the case either because they are
phi nodes in the same block, or because the operand dominates
the block.
Currently, most callers of foldOpIntoPhi() satisfy this
pre-condition by requiring a constant operand, which trivially
dominates everything. Only selects had handling for variable
operands.
Move this logic into foldOpIntoPhi(), so things are handled
correctly if other callers are generalized. Also make the
implementation a bit more general by querying the dominator tree.
---
.../InstCombine/InstCombineSelect.cpp | 42 +------------------
.../InstCombine/InstructionCombining.cpp | 27 ++++++++++--
.../InstCombine/phi-select-constant.ll | 8 ++--
3 files changed, 30 insertions(+), 47 deletions(-)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index fcd11126073bf1..66f7c4592457c2 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1992,41 +1992,6 @@ Instruction *InstCombinerImpl::foldSelectInstWithICmp(SelectInst &SI,
return Changed ? &SI : nullptr;
}
-/// SI is a select whose condition is a PHI node (but the two may be in
-/// different blocks). See if the true/false values (V) are live in all of the
-/// predecessor blocks of the PHI. For example, cases like this can't be mapped:
-///
-/// X = phi [ C1, BB1], [C2, BB2]
-/// Y = add
-/// Z = select X, Y, 0
-///
-/// because Y is not live in BB1/BB2.
-static bool canSelectOperandBeMappingIntoPredBlock(const Value *V,
- const SelectInst &SI) {
- // If the value is a non-instruction value like a constant or argument, it
- // can always be mapped.
- const Instruction *I = dyn_cast<Instruction>(V);
- if (!I) return true;
-
- // If V is a PHI node defined in the same block as the condition PHI, we can
- // map the arguments.
- const PHINode *CondPHI = cast<PHINode>(SI.getCondition());
-
- if (const PHINode *VP = dyn_cast<PHINode>(I))
- if (VP->getParent() == CondPHI->getParent())
- return true;
-
- // Otherwise, if the PHI and select are defined in the same block and if V is
- // defined in a different block, then we can transform it.
- if (SI.getParent() == CondPHI->getParent() &&
- I->getParent() != CondPHI->getParent())
- return true;
-
- // Otherwise we have a 'hard' case and we can't tell without doing more
- // detailed dominator based analysis, punt.
- return false;
-}
-
/// We have an SPF (e.g. a min or max) of an SPF of the form:
/// SPF2(SPF1(A, B), C)
Instruction *InstCombinerImpl::foldSPFofSPF(Instruction *Inner,
@@ -3929,11 +3894,8 @@ Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
// See if we can fold the select into a phi node if the condition is a select.
if (auto *PN = dyn_cast<PHINode>(SI.getCondition()))
- // The true/false values have to be live in the PHI predecessor's blocks.
- if (canSelectOperandBeMappingIntoPredBlock(TrueVal, SI) &&
- canSelectOperandBeMappingIntoPredBlock(FalseVal, SI))
- if (Instruction *NV = foldOpIntoPhi(SI, PN))
- return NV;
+ if (Instruction *NV = foldOpIntoPhi(SI, PN))
+ return NV;
if (SelectInst *TrueSI = dyn_cast<SelectInst>(TrueVal)) {
if (TrueSI->getCondition()->getType() == CondVal->getType()) {
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index c3f79fe4f901ad..95199d18419afa 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -1731,8 +1731,7 @@ static Value *simplifyInstructionWithPHI(Instruction &I, PHINode *PN,
const DataLayout &DL,
const SimplifyQuery SQ) {
// NB: It is a precondition of this transform that the operands be
- // phi translatable! This is usually trivially satisfied by limiting it
- // to constant ops, and for selects we do a more sophisticated check.
+ // phi translatable!
SmallVector<Value *> Ops;
for (Value *Op : I.operands()) {
if (Op == PN)
@@ -1785,9 +1784,31 @@ Instruction *InstCombinerImpl::foldOpIntoPhi(Instruction &I, PHINode *PN) {
// Otherwise, we can replace *all* users with the new PHI we form.
}
+ // Check that all operands are phi-translatable.
+ for (Value *Op : I.operands()) {
+ if (Op == PN)
+ continue;
+
+ // Non-instructions never require phi-translation.
+ auto *I = dyn_cast<Instruction>(Op);
+ if (!I)
+ continue;
+
+ // Can phi-translate phi nodes in the same block.
+ if (isa<PHINode>(I))
+ if (I->getParent() == PN->getParent())
+ continue;
+
+ // Operand dominates the block, no phi-translation necessary.
+ if (DT.dominates(I, PN->getParent()))
+ continue;
+
+ // Not phi-translatable, bail out.
+ return nullptr;
+ }
+
// Check to see whether the instruction can be folded into each phi operand.
// If there is one operand that does not fold, remember the BB it is in.
- // If there is more than one or if *it* is a PHI, bail out.
SmallVector<Value *> NewPhiValues;
BasicBlock *NonSimplifiedBB = nullptr;
Value *NonSimplifiedInVal = nullptr;
diff --git a/llvm/test/Transforms/InstCombine/phi-select-constant.ll b/llvm/test/Transforms/InstCombine/phi-select-constant.ll
index 9d61891127690c..81a27811ec63f5 100644
--- a/llvm/test/Transforms/InstCombine/phi-select-constant.ll
+++ b/llvm/test/Transforms/InstCombine/phi-select-constant.ll
@@ -226,17 +226,17 @@ final:
define i32 @dominating_values_select_not_same_block(i1 %c1, i1 %c2, ptr %p, ptr %p2) {
; CHECK-LABEL: @dominating_values_select_not_same_block(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[A:%.*]] = load i32, ptr [[P:%.*]], align 4
; CHECK-NEXT: [[B:%.*]] = load i32, ptr [[P2:%.*]], align 4
; CHECK-NEXT: br i1 [[C1:%.*]], label [[FINAL:%.*]], label [[DELAY:%.*]]
; CHECK: delay:
+; CHECK-NEXT: [[A:%.*]] = load i32, ptr [[P:%.*]], align 4
+; CHECK-NEXT: [[TMP0:%.*]] = select i1 [[C2:%.*]], i32 [[A]], i32 [[B]]
; CHECK-NEXT: br label [[FINAL]]
; CHECK: final:
-; CHECK-NEXT: [[USE2:%.*]] = phi i1 [ false, [[ENTRY:%.*]] ], [ [[C2:%.*]], [[DELAY]] ]
+; CHECK-NEXT: [[USE2:%.*]] = phi i32 [ [[B]], [[ENTRY:%.*]] ], [ [[TMP0]], [[DELAY]] ]
; CHECK-NEXT: br label [[SPLIT:%.*]]
; CHECK: split:
-; CHECK-NEXT: [[VALUE:%.*]] = select i1 [[USE2]], i32 [[A]], i32 [[B]]
-; CHECK-NEXT: ret i32 [[VALUE]]
+; CHECK-NEXT: ret i32 [[USE2]]
;
entry:
%a = load i32, ptr %p
>From 79a7c933b5d29e9dc37614f567ae14a4bf1be781 Mon Sep 17 00:00:00 2001
From: Nikita Popov <nikita.ppv at gmail.com>
Date: Tue, 27 Aug 2024 20:47:46 +0200
Subject: [PATCH 2/2] avoid shadowing
---
.../Transforms/InstCombine/InstructionCombining.cpp | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 95199d18419afa..e6410f56651b02 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -1790,17 +1790,17 @@ Instruction *InstCombinerImpl::foldOpIntoPhi(Instruction &I, PHINode *PN) {
continue;
// Non-instructions never require phi-translation.
- auto *I = dyn_cast<Instruction>(Op);
- if (!I)
+ auto *OpI = dyn_cast<Instruction>(Op);
+ if (!OpI)
continue;
// Can phi-translate phi nodes in the same block.
- if (isa<PHINode>(I))
- if (I->getParent() == PN->getParent())
+ if (isa<PHINode>(OpI))
+ if (OpI->getParent() == PN->getParent())
continue;
// Operand dominates the block, no phi-translation necessary.
- if (DT.dominates(I, PN->getParent()))
+ if (DT.dominates(OpI, PN->getParent()))
continue;
// Not phi-translatable, bail out.
More information about the llvm-commits
mailing list