[llvm] [InstCombine] Generalize and consolidate phi translation check (PR #106051)
via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 26 03:13:08 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-transforms
Author: Nikita Popov (nikic)
<details>
<summary>Changes</summary>
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.
No compile-time impact but improvements on stage2 builds, so this helps clang: https://llvm-compile-time-tracker.com/compare.php?from=32679e10a9b66405c340213993f65b2edf5a794a&to=e99c004d372891fa23b79de92003fe123143dac2&stat=instructions:u
---
Full diff: https://github.com/llvm/llvm-project/pull/106051.diff
3 Files Affected:
- (modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+2-40)
- (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+24-3)
- (modified) llvm/test/Transforms/InstCombine/phi-select-constant.ll (+4-4)
``````````diff
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
``````````
</details>
https://github.com/llvm/llvm-project/pull/106051
More information about the llvm-commits
mailing list