[InstCombine] Remove redundant fptrunc of select fold (NFCI)

[nikic]

This was subsumed by FoldOpIntoSelect() in #116073, and has incorrect FMF propagation on top.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

This was subsumed by FoldOpIntoSelect() in #116073, and has incorrect FMF propagation on top.

---

Full diff: https://github.com/llvm/llvm-project/pull/117182.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp (-18)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
index 6c2554ea73b7f8..c97be6ffc75675 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
@@ -1866,24 +1866,6 @@ Instruction *InstCombinerImpl::visitFPTrunc(FPTruncInst &FPT) {
 
       return UnaryOperator::CreateFNegFMF(InnerTrunc, Op);
     }
-
-    // If we are truncating a select that has an extended operand, we can
-    // narrow the other operand and do the select as a narrow op.
-    Value *Cond, *X, *Y;
-    if (match(Op, m_Select(m_Value(Cond), m_FPExt(m_Value(X)), m_Value(Y))) &&
-        X->getType() == Ty) {
-      // fptrunc (select Cond, (fpext X), Y --> select Cond, X, (fptrunc Y)
-      Value *NarrowY = Builder.CreateFPTrunc(Y, Ty);
-      Value *Sel = Builder.CreateSelect(Cond, X, NarrowY, "narrow.sel", Op);
-      return replaceInstUsesWith(FPT, Sel);
-    }
-    if (match(Op, m_Select(m_Value(Cond), m_Value(Y), m_FPExt(m_Value(X)))) &&
-        X->getType() == Ty) {
-      // fptrunc (select Cond, Y, (fpext X) --> select Cond, (fptrunc Y), X
-      Value *NarrowY = Builder.CreateFPTrunc(Y, Ty);
-      Value *Sel = Builder.CreateSelect(Cond, NarrowY, X, "narrow.sel", Op);
-      return replaceInstUsesWith(FPT, Sel);
-    }
   }
 
   if (auto *II = dyn_cast<IntrinsicInst>(FPT.getOperand(0))) {

[artagnon]

I think this LGTM, except maybe change NFCI to NFC in the title? I also looked at the subsuming commit, and I think we can be confident that this is an NFC.

[dtcxzyw]

Can you pre-commit this test: https://alive2.llvm.org/ce/z/XNQvu5

define half @src(half %394) {
entry:
  %395 = fpext half %394 to double
   %396 = fcmp olt double %395, 0.000000e+00
   %397 = select i1 %396, double 0.000000e+00, double %395
   %398 = fptrunc double %397 to half
  ret half %398
}

define half @tgt(half %394) {
entry:
   %396 = fcmp olt half %394, 0.000000e+00
   %397 = select i1 %396, half 0.000000e+00, half %394
  ret half %397
}

[nikic]

@dtcxzyw Done in f81f47e.

This case is doubly-blocked, first by

llvm-project/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

Lines 176 to 184 in f81f47e

	// We are casting a select. Try to fold the cast into the select if the
	// select does not have a compare instruction with matching operand types
	// or the select is likely better done in a narrow type.
	// Creating a select with operands that are different sizes than its
	// condition may inhibit other folds and lead to worse codegen.
	auto *Cmp = dyn_cast<CmpInst>(Sel->getCondition());
	if (!Cmp || Cmp->getOperand(0)->getType() != Sel->getType() ||
	(CI.getOpcode() == Instruction::Trunc &&
	shouldChangeType(CI.getSrcTy(), CI.getType()))) {

which checks for a comparison with same types and then

llvm-project/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

Lines 1696 to 1709 in f81f47e

	// Test if a FCmpInst instruction is used exclusively by a select as
	// part of a minimum or maximum operation. If so, refrain from doing
	// any other folding. This helps out other analyses which understand
	// non-obfuscated minimum and maximum idioms. And in this case, at
	// least one of the comparison operands has at least one user besides
	// the compare (the select), which would often largely negate the
	// benefit of folding anyway.
	if (auto *CI = dyn_cast<FCmpInst>(SI->getCondition())) {
	if (CI->hasOneUse()) {
	Value *Op0 = CI->getOperand(0), *Op1 = CI->getOperand(1);
	if ((TV == Op0 && FV == Op1) || (FV == Op0 && TV == Op1))
	return nullptr;
	}
	}

blocks specifically the min/max pattern.

[nikic]

A complicating factor is that we perform this canonicalization:

define float @t4(double %a) {
; CHECK-LABEL: @t4(
; CHECK-NEXT:    [[DOTINV:%.*]] = fcmp oge double [[A:%.*]], 5.000000e+00
; CHECK-NEXT:    [[TMP1:%.*]] = select i1 [[DOTINV]], double 5.000000e+00, double [[A]]
; CHECK-NEXT:    [[TMP2:%.*]] = fptrunc double [[TMP1]] to float
; CHECK-NEXT:    ret float [[TMP2]]
;
  %1 = fcmp ult double %a, 5.0
  %2 = fptrunc double %a to float
  %3 = select i1 %1, float %2, float 5.0
  ret float %3
}

So just special casing fptrunc also doesn't work, because we'd infinite loop with this transform. So I'm inclined to just leave this alone...

[dtcxzyw]

LGTM. Remember to update the test f81f47e :)

[nikic]

@dtcxzyw But do we really want to merge this despite the regression?

[dtcxzyw]

@dtcxzyw But do we really want to merge this despite the regression?

This pattern looks weird and should be caused by an unexpected upcasting. I believe it is easy to be fixed in the source code with the help of linters.

BTW, before fixing the regression, we should fold fptrunc max/min(fpext(X), fpext(Y)) first: https://alive2.llvm.org/ce/z/WtxWAM. It is enough for some applications pursuing the floating point performance.

So I agree that we just leave it alone.