[SelectionDAG] Removing FABS folding from DAGCombiner

[mike.dvoretsky]

Summary

Since D44550 introduced FABS pattern folding in InstCombine, this patch removes the now-redundant code that causes https://bugs.llvm.org/show_bug.cgi?id=36600.

The llc test was removed as InstCombine isn't part of the llc pipeline.

[spatel]

I don't mind removing x86 noise, but we still need to ensure that the bug doesn't resuscitate. Please rebase with:
https://reviews.llvm.org/rL327995

[mike.dvoretsky]

Rebased. PR36600 appears fixed.

[arsenm]

There is supposed to be redundancy between instcombine and dagcombiner. Patterns can appear during lowering that won’t appear in instcombine

[mike.dvoretsky]

In D44683#1044354, @arsenm wrote:

There is supposed to be redundancy between instcombine and dagcombiner. Patterns can appear during lowering that won’t appear in instcombine

If it would be preferred to keep the code, then I could basically recreate D44091 in full. I would note that currently there are no tests in which this pattern is produced specifically in DAG.

[spatel]

In D44683#1044354, @arsenm wrote:

There is supposed to be redundancy between instcombine and dagcombiner. Patterns can appear during lowering that won’t appear in instcombine

I agree that there should be partial redundancy between these passes, but if we are confident that a pattern should not be created, then trying to match it is just overhead.

In this case, we have ISD::FABS, so if there's some existing code that is creating compare and select when it could be creating FABS directly, then I think it should be fixed to not do that.

[spatel]

LGTM.

[mike.dvoretsky]

@spatel, could you please commit this? I don't have commit rights.

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::SimplifySelectCC(const SDLoc &DL, SDValue N0, SDValue N1,
   if (SCC.getNode()) AddToWorklist(SCC.getNode());
 
   if (ConstantSDNode *SCCC = dyn_cast_or_null<ConstantSDNode>(SCC.getNode())) {
     // fold select_cc true, x, y -> x
     // fold select_cc false, x, y -> y
     return !SCCC->isNullValue() ? N2 : N3;
   }
 
-  // Check to see if we can simplify the select into an fabs node
-  if (ConstantFPSDNode *CFP = dyn_cast<ConstantFPSDNode>(N1)) {
-    // Allow either -0.0 or 0.0
-    if (CFP->isZero()) {
-      // select (setg[te] X, +/-0.0), X, fneg(X) -> fabs
-      if ((CC == ISD::SETGE || CC == ISD::SETGT) &&
-          N0 == N2 && N3.getOpcode() == ISD::FNEG &&
-          N2 == N3.getOperand(0))
-        return DAG.getNode(ISD::FABS, DL, VT, N0);
-
-      // select (setl[te] X, +/-0.0), fneg(X), X -> fabs
-      if ((CC == ISD::SETLT || CC == ISD::SETLE) &&
-          N0 == N3 && N2.getOpcode() == ISD::FNEG &&
-          N2.getOperand(0) == N3)
-        return DAG.getNode(ISD::FABS, DL, VT, N3);
-    }
-  }
-
   // Turn "(a cond b) ? 1.0f : 2.0f" into "load (tmp + ((a cond b) ? 0 : 4)"
   // where "tmp" is a constant pool entry containing an array with 1.0 and 2.0
   // in it.  This is a win when the constant is not otherwise available because
   // it replaces two constant pool loads with one.  We only do this if the FP
   // type is known to be legal, because if it isn't, then we are before legalize
   // types an we want the other legalization to happen first (e.g. to avoid
   // messing with soft float) and if the ConstantFP is not legal, because if
   // it is legal, we may not need to store the FP constant in a constant pool.

llvm/trunk/test/CodeGen/AArch64/fabs.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=aarch64-unknown-unknown | FileCheck %s
 
-; FIXME:
-; PR36600: https://bugs.llvm.org/show_bug.cgi?id=36600
+; Test against PR36600: https://bugs.llvm.org/show_bug.cgi?id=36600
 ; This is not fabs. If X = -0.0, it should return -0.0 not 0.0.
 
 define double @not_fabs(double %x) #0 {
 ; CHECK-LABEL: not_fabs:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    fabs d0, d0
+; CHECK-NEXT:    fneg d1, d0
+; CHECK-NEXT:    fcmp d0, #0.0
+; CHECK-NEXT:    fcsel d0, d1, d0, le
 ; CHECK-NEXT:    ret
   %cmp = fcmp nnan ole double %x, 0.0
   %sub = fsub nnan double -0.0, %x
   %cond = select i1 %cmp, double %sub, double %x
   ret double %cond
 }
 
 ; Try again with different type, predicate, and compare constant.
 
 define float @still_not_fabs(float %x) #0 {
 ; CHECK-LABEL: still_not_fabs:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    fabs s0, s0
+; CHECK-NEXT:    adrp x8, .LCPI1_0
+; CHECK-NEXT:    ldr s1, [x8, :lo12:.LCPI1_0]
+; CHECK-NEXT:    fneg s2, s0
+; CHECK-NEXT:    fcmp s0, s1
+; CHECK-NEXT:    fcsel s0, s0, s2, ge
 ; CHECK-NEXT:    ret
   %cmp = fcmp nnan oge float %x, -0.0
   %sub = fsub nnan float -0.0, %x
   %cond = select i1 %cmp, float %x, float %sub
   ret float %cond
 }
 
 attributes #0 = { "no-nans-fp-math"="true" }
 

llvm/trunk/test/CodeGen/X86/fabs.ll

 ; X64-LABEL: test1:
 ; X64:       # %bb.0:
 ; X64-NEXT:    andps {{.*}}(%rip), %xmm0
 ; X64-NEXT:    retq
   %Y = call float @fabsf(float %X) readnone
   ret float %Y
 }
 
-define double @test2(double %X) {
+define x86_fp80 @test2(x86_fp80 %X) {
 ; X87-LABEL: test2:
 ; X87:       # %bb.0:
-; X87-NEXT:    fldl {{[0-9]+}}(%esp)
-; X87-NEXT:    fldz
-; X87-NEXT:    fchs
-; X87-NEXT:    fxch %st(1)
-; X87-NEXT:    fucom %st(1)
-; X87-NEXT:    fstp %st(1)
-; X87-NEXT:    fnstsw %ax
-; X87-NEXT:    # kill: def $ah killed $ah killed $ax
-; X87-NEXT:    sahf
-; X87-NEXT:    fld %st(0)
-; X87-NEXT:    fchs
-; X87-NEXT:    jae .LBB1_2
-; X87-NEXT:  # %bb.1:
-; X87-NEXT:    fstp %st(1)
-; X87-NEXT:    fldz
-; X87-NEXT:  .LBB1_2:
-; X87-NEXT:    fstp %st(0)
-; X87-NEXT:    retl
-;
-; X87UNSAFE-LABEL: test2:
-; X87UNSAFE:       # %bb.0:
-; X87UNSAFE-NEXT:    fldl {{[0-9]+}}(%esp)
-; X87UNSAFE-NEXT:    fabs
-; X87UNSAFE-NEXT:    retl
-;
-; X64-LABEL: test2:
-; X64:       # %bb.0:
-; X64-NEXT:    movsd {{.*#+}} xmm1 = mem[0],zero
-; X64-NEXT:    movapd %xmm1, %xmm2
-; X64-NEXT:    unpcklpd {{.*#+}} xmm2 = xmm2[0],xmm1[0]
-; X64-NEXT:    xorpd %xmm0, %xmm2
-; X64-NEXT:    cmplesd %xmm0, %xmm1
-; X64-NEXT:    movapd %xmm1, %xmm3
-; X64-NEXT:    andnpd %xmm2, %xmm3
-; X64-NEXT:    andpd %xmm0, %xmm1
-; X64-NEXT:    orpd %xmm3, %xmm1
-; X64-NEXT:    movapd %xmm1, %xmm0
-; X64-NEXT:    retq
-  %Y = fcmp oge double %X, -0.0
-  %Z = fsub double -0.0, %X
-  %Q = select i1 %Y, double %X, double %Z
-  ret double %Q
-}
-
-define x86_fp80 @test3(x86_fp80 %X) {
-; X87-LABEL: test3:
-; X87:       # %bb.0:
 ; X87-NEXT:    fldt {{[0-9]+}}(%esp)
 ; X87-NEXT:    fabs
 ; X87-NEXT:    retl
 ;
-; X87UNSAFE-LABEL: test3:
+; X87UNSAFE-LABEL: test2:
 ; X87UNSAFE:       # %bb.0:
 ; X87UNSAFE-NEXT:    fldt {{[0-9]+}}(%esp)
 ; X87UNSAFE-NEXT:    fabs
 ; X87UNSAFE-NEXT:    retl
 ;
-; X64-LABEL: test3:
+; X64-LABEL: test2:
 ; X64:       # %bb.0:
 ; X64-NEXT:    fldt {{[0-9]+}}(%rsp)
 ; X64-NEXT:    fabs
 ; X64-NEXT:    retq
   %Y = call x86_fp80 @fabsl(x86_fp80 %X) readnone
   ret x86_fp80 %Y
 }