[InstCombine] Complete folding of fneg-of-fabs

[Chenbing.Zheng]

Summary

This patch add a function foldSelectToFabs, and do more combine for
fneg-of-fabs.

With 'nsz'

fold (X <  +/-0.0) ? X : -X or (X <= +/-0.0) ? X : -X to -fabs(x)
fold (X >  +/-0.0) ? X : -X or (X >= +/-0.0) ? X : -X to -fabs(x)

[Chenbing.Zheng]

format correction

[spatel]

This doesn't solve the root problem. If we start with the existing code in the test that is changing, it will not become fneg(fabs) like we want?
https://alive2.llvm.org/ce/z/e_uhUQ

We should extend the code under InstCombinerImpl::visitSelectInst() to match "nabs" patterns. Search for: "// Canonicalize select with fcmp to fabs()."

[Chenbing.Zheng]

address comments

[Chenbing.Zheng]

update test

[spatel]

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

2812

We don't check 'nnan', so this comment is wrong.

I see that it is copied from above, so that suggests a better solution than only fixing it on this line: can you reduce the code duplication by moving the existing fabs transforms to a helper function or lambda?

Maybe it is easier if we match the negated value in the select first (and give it the name 'X'), then match the compare?

[Chenbing.Zheng]

Thanks for spatel's suggestion , that's very useful.

[spatel]

In D123830#3466496, @Chenbing.Zheng wrote:

Thanks for spatel's suggestion , that's very useful.

Yes, this looks nicer. See inline for some small improvements.

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

2503

Swap should be a bool parameter, and the caller can use "for (bool Swap : {false, true} )".
Or we can move the FNeg part of the transform into this function and avoid the parameter completely?

2506

If we name this value 'X', then the code would match the comments, so that seems easier to read - see the next comments for a possible reduction.

2512–2513

We can move this statement to above the function declaration now. It describes the complete behavior of this function.

2532–2533

This logic seems more complicated than needed. If we always match the TrueVal as -X, then we can get the swapped predicate and reduce the code to something like this:

if (!match(TrueVal, m_FNeg(m_Specific(FalseVal))) || !SI.hasNoSignedZeros())
  return nullptr;

if (Swap)
  Pred = FCmpInst::getSwappedPredicate(Pred);

// With nsz:
// fold (X <  +/-0.0) ? -X : X or (X <= +/-0.0) ? -X : X to fabs(X)
// fold (X >  +/-0.0) ? -X : X or (X >= +/-0.0) ? X : -X to -fabs(x)
bool IsLTOrLE = Pred == FCmpInst::FCMP_OLT || Pred == FCmpInst::FCMP_OLE ||
                Pred == FCmpInst::FCMP_ULT || Pred == FCmpInst::FCMP_ULE;
bool IsGTOrGE = Pred == FCmpInst::FCMP_OGT || Pred == FCmpInst::FCMP_OGE ||
                Pred == FCmpInst::FCMP_UGT || Pred == FCmpInst::FCMP_UGE;

if (IsLTOrLE)
  return Builder.CreateUnaryIntrinsic(Intrinsic::fabs, FalseVal, &SI);
if (IsGTOrGE) {
  IsNeg = true;
  return Builder.CreateUnaryIntrinsic(Intrinsic::fabs, FalseVal, &SI);
}

[Chenbing.Zheng]

address comments

[spatel]

LGTM

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

else
       // select op, true, (select cond, A, B) => select op, true, B
       // or     op, (select cond, A, B)       => select op, true, B
       //   if op = false implies condval = false.
       return SelectInst::Create(Op, One, B);
   }
 }
 
 Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
   Value *CondVal = SI.getCondition();
   Value *TrueVal = SI.getTrueValue();
   Value *FalseVal = SI.getFalseValue();
   Type *SelType = SI.getType();
 
   // FIXME: Remove this workaround when freeze related patches are done.
   // For select with undef operand which feeds into an equality comparison,
   // don't simplify it so loop unswitch can know the equality comparison
   // may have an undef operand. This is a workaround for PR31652 caused by
   // descrepancy about branch on undef between LoopUnswitch and GVN.
   if (match(TrueVal, m_Undef()) || match(FalseVal, m_Undef())) {
     if (llvm::any_of(SI.users(), [&](User *U) {
           ICmpInst *CI = dyn_cast<ICmpInst>(U);
           if (CI && CI->isEquality())
             return true;
           return false;
         })) {
       return nullptr;
     }
   }
 
   if (Value *V = SimplifySelectInst(CondVal, TrueVal, FalseVal,
                                     SQ.getWithInstruction(&SI)))
     return replaceInstUsesWith(SI, V);
 
   if (Instruction *I = canonicalizeSelectToShuffle(SI))
     return I;
 
   if (Instruction *I = canonicalizeScalarSelectOfVecs(SI, *this))
     return I;
 
   CmpInst::Predicate Pred;
 
   // Avoid potential infinite loops by checking for non-constant condition.
   // TODO: Can we assert instead by improving canonicalizeSelectToShuffle()?
   //       Scalar select must have simplified?
   if (SelType->isIntOrIntVectorTy(1) && !isa<Constant>(CondVal) &&
       TrueVal->getType() == CondVal->getType()) {
     // Folding select to and/or i1 isn't poison safe in general. impliesPoison
Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
   // (X >= +/-0.0) ? X : -X --> fabs(X)
   if (match(CondVal, m_FCmp(Pred, m_Specific(TrueVal), m_AnyZeroFP())) &&
       match(FalseVal, m_FNeg(m_Specific(TrueVal))) && SI.hasNoSignedZeros() &&
       (Pred == FCmpInst::FCMP_OGT || Pred == FCmpInst::FCMP_OGE ||
        Pred == FCmpInst::FCMP_UGT || Pred == FCmpInst::FCMP_UGE)) {
     Value *Fabs = Builder.CreateUnaryIntrinsic(Intrinsic::fabs, TrueVal, &SI);
     return replaceInstUsesWith(SI, Fabs);
   }
+  // With nnan and nsz
+  // (X <   +/-0.0) ? X : -X --> -fabs(x)
+  // (X <=  +/-0.0) ? X : -X --> -fabs(x)
+  if (match(CondVal, m_FCmp(Pred, m_Specific(TrueVal), m_AnyZeroFP())) &&
+      match(FalseVal, m_FNeg(m_Specific(TrueVal))) && SI.hasNoSignedZeros() &&
+      (Pred == FCmpInst::FCMP_OLT || Pred == FCmpInst::FCMP_OLE ||
+       Pred == FCmpInst::FCMP_ULT || Pred == FCmpInst::FCMP_ULE)) {
+    Value *Fabs = Builder.CreateUnaryIntrinsic(Intrinsic::fabs, TrueVal, &SI);
+    Instruction *NewFNeg = UnaryOperator::CreateFNeg(Fabs);
+    NewFNeg->setFastMathFlags(SI.getFastMathFlags());
+    return NewFNeg;
+  }
+  // With nnan and nsz
+  // (X >   +/-0.0) ? -X : X --> -fabs(x)
+  // (X >=  +/-0.0) ? -X : X --> -fabs(x)
+  if (match(CondVal, m_FCmp(Pred, m_Specific(FalseVal), m_AnyZeroFP())) &&
+      match(TrueVal, m_FNeg(m_Specific(FalseVal))) && SI.hasNoSignedZeros() &&
+      (Pred == FCmpInst::FCMP_OGT || Pred == FCmpInst::FCMP_OGE ||
+       Pred == FCmpInst::FCMP_UGT || Pred == FCmpInst::FCMP_UGE)) {
+    Value *Fabs = Builder.CreateUnaryIntrinsic(Intrinsic::fabs, FalseVal, &SI);
+    Instruction *NewFNeg = UnaryOperator::CreateFNeg(Fabs);
+    NewFNeg->setFastMathFlags(SI.getFastMathFlags());
+    return NewFNeg;
+  }
 
   // See if we are selecting two values based on a comparison of the two values.
   if (ICmpInst *ICI = dyn_cast<ICmpInst>(CondVal))
     if (Instruction *Result = foldSelectInstWithICmp(SI, ICI))
       return Result;
 
   if (Instruction *Add = foldAddSubSelect(SI, Builder))
     return Add;

llvm/test/Transforms/InstCombine/fneg.ll

 ;
   %fneg = fneg float %a
   %cmp = fcmp ogt float %a, %fneg
   %sel = select i1 %cmp, float %fneg, float %a
   %fneg1 = fneg nnan ninf nsz float %sel
   ret float %fneg1
 }
 
-; TODO: This should reduce to fneg-of-fabs.
-
 define float @fnabs(float %a) {
 ; CHECK-LABEL: @fnabs(
-; CHECK-NEXT:    [[CMP:%.*]] = fcmp olt float [[A:%.*]], 0.000000e+00
-; CHECK-NEXT:    [[A_NEG:%.*]] = fneg fast float [[A]]
-; CHECK-NEXT:    [[FNEG1:%.*]] = select fast i1 [[CMP]], float [[A]], float [[A_NEG]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast float @llvm.fabs.f32(float [[A:%.*]])
+; CHECK-NEXT:    [[FNEG1:%.*]] = fneg fast float [[TMP1]]
 ; CHECK-NEXT:    ret float [[FNEG1]]
 ;
   %fneg = fneg float %a
   %cmp = fcmp olt float %a, %fneg
   %sel = select i1 %cmp, float %fneg, float %a
   %fneg1 = fneg fast float %sel
   ret float %fneg1
 }
 
 define float @fnabs_1(float %a) {
 ; CHECK-LABEL: @fnabs_1(
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast float @llvm.fabs.f32(float [[A:%.*]])
+; CHECK-NEXT:    [[FNEG1:%.*]] = fneg fast float [[TMP1]]
+; CHECK-NEXT:    ret float [[FNEG1]]
+;
+  %fneg = fneg float %a
+  %cmp = fcmp ogt float %a, %fneg
+  %sel = select i1 %cmp, float %a, float %fneg
+  %fneg1 = fneg fast float %sel
+  ret float %fneg1
+}
+
+define float @fnabs_2(float %a) {
+; CHECK-LABEL: @fnabs_2(
 ; CHECK-NEXT:    [[TMP1:%.*]] = call nsz float @llvm.fabs.f32(float [[A:%.*]])
 ; CHECK-NEXT:    [[FNEG1:%.*]] = fneg float [[TMP1]]
 ; CHECK-NEXT:    ret float [[FNEG1]]
 ;
   %fneg = fneg float %a
   %cmp = fcmp olt float %a, %fneg
   %sel = select nsz i1 %cmp, float %fneg, float %a
   %fneg1 = fneg float %sel