[llvm] [InstCombine] Generalize and consolidate phi translation check (PR #106051)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 4 06:05:43 PDT 2024
https://github.com/nikic updated https://github.com/llvm/llvm-project/pull/106051
>From 6ef4b86d8f77f58a13ff9847de980b73811eb7ac 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 ad2a620081bcd9..34f2a5048b56ab 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -1730,8 +1730,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)
@@ -1784,9 +1783,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 38f1680b7368d721d8f26c745ee3e0bf99b13dcc Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Wed, 4 Sep 2024 15:05:09 +0200
Subject: [PATCH 2/2] Adjust comment
---
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 34f2a5048b56ab..8195e0539305cc 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -1793,7 +1793,7 @@ Instruction *InstCombinerImpl::foldOpIntoPhi(Instruction &I, PHINode *PN) {
if (!I)
continue;
- // Can phi-translate phi nodes in the same block.
+ // Phi-translate can handle phi nodes in the same block.
if (isa<PHINode>(I))
if (I->getParent() == PN->getParent())
continue;
More information about the llvm-commits
mailing list