[DAG] use SDNode flags 'nsz' to enable fadd/fsub with zero folds

[spatel]

Summary

As discussed in D24815, let's start the process of killing off the broken fast-math global state housed in TargetOptions and eliminate the need for function-level fast-math attributes.

Looks like I started this mission in D9708...and promptly got distracted for almost a year. :)

Here we enable two similar folds that are possible when we don't care about signed-zero:
fadd nsz x, 0 --> x
fsub nsz 0, x --> -x

Note that although the test cases include a 'sin' function call, I'm side-stepping the FMF-on-calls question (and lack of support in the DAG) for now. It's not needed for these tests - isNegatibleForFree/GetNegatedExpression just look through a ISD::FSIN node.

[spatel]

Ping.

[spatel]

Ping * 2.

[echristo]

This seems reasonable to me. I'd like to see something that adds the attribute to the instructions at some point, but as an intermediate step this is fine.

-eric

[spatel]

In D25297#575826, @echristo wrote:

This seems reasonable to me. I'd like to see something that adds the attribute to the instructions at some point, but as an intermediate step this is fine.

Do you mean propagate the FMF flags on the returned SDNode? Eg:

return DAG.getNode(ISD::FNEG, DL, VT, N1, Flags);

Oops...I forgot to do that in *this* patch.

[spatel]

In D25297#575880, @spatel wrote:

In D25297#575826, @echristo wrote:

This seems reasonable to me. I'd like to see something that adds the attribute to the instructions at some point, but as an intermediate step this is fine.

Do you mean propagate the FMF flags on the returned SDNode? Eg:

return DAG.getNode(ISD::FNEG, DL, VT, N1, Flags);

Oops...I forgot to do that in *this* patch.

Although in my defense, putting the Flags on an FNEG is an academic exercise at this point because we only have FMF on binary SDNodes. The flags will be dropped in getNode() for the unary node.

[echristo]

*nod* I'm not worried about it.

-eric

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

case ISD::FADD:
     if (char V = isNegatibleForFree(Op.getOperand(0), LegalOperations, TLI,
                                     Options, Depth + 1))
       return V;
     // fold (fneg (fadd A, B)) -> (fsub (fneg B), A)
     return isNegatibleForFree(Op.getOperand(1), LegalOperations, TLI, Options,
                               Depth + 1);
   case ISD::FSUB:
     // We can't turn -(A-B) into B-A when we honor signed zeros.
-    if (!Options->UnsafeFPMath) return 0;
+    if (!Options->UnsafeFPMath && !Op.getNode()->getFlags()->hasNoSignedZeros())
+      return 0;
 
     // fold (fneg (fsub A, B)) -> (fsub B, A)
     return 1;
 
   case ISD::FMUL:
   case ISD::FDIV:
     if (Options->HonorSignDependentRoundingFPMath()) return 0;
 
if (isNegatibleForFree(Op.getOperand(0), LegalOperations,
                                               LegalOperations, Depth+1),
                          Op.getOperand(1), Flags);
     // fold (fneg (fadd A, B)) -> (fsub (fneg B), A)
     return DAG.getNode(ISD::FSUB, SDLoc(Op), Op.getValueType(),
                        GetNegatedExpression(Op.getOperand(1), DAG,
                                             LegalOperations, Depth+1),
                        Op.getOperand(0), Flags);
   case ISD::FSUB:
-    // We can't turn -(A-B) into B-A when we honor signed zeros.
-    assert(Options.UnsafeFPMath);
-
     // fold (fneg (fsub 0, B)) -> B
     if (ConstantFPSDNode *N0CFP = dyn_cast<ConstantFPSDNode>(Op.getOperand(0)))
       if (N0CFP->isZero())
         return Op.getOperand(1);
 
     // fold (fneg (fsub A, B)) -> (fsub B, A)
     return DAG.getNode(ISD::FSUB, SDLoc(Op), Op.getValueType(),
                        Op.getOperand(1), Op.getOperand(0), Flags);
return DAG.getNode(ISD::FSUB, DL, VT, N0,
                        GetNegatedExpression(N1, DAG, LegalOperations), Flags);
 
   // fold (fadd (fneg A), B) -> (fsub B, A)
   if ((!LegalOperations || TLI.isOperationLegalOrCustom(ISD::FSUB, VT)) &&
       isNegatibleForFree(N0, LegalOperations, TLI, &Options) == 2)
     return DAG.getNode(ISD::FSUB, DL, VT, N1,
                        GetNegatedExpression(N0, DAG, LegalOperations), Flags);
 
+  // FIXME: Auto-upgrade the target/function-level option.
+  if (Options.UnsafeFPMath || N->getFlags()->hasNoSignedZeros()) {
+    // fold (fadd A, 0) -> A
+    if (ConstantFPSDNode *N1C = isConstOrConstSplatFP(N1))
+      if (N1C->isZero())
+        return N0;
+  }
+
   // If 'unsafe math' is enabled, fold lots of things.
   if (Options.UnsafeFPMath) {
     // No FP constant should be created after legalization as Instruction
     // Selection pass has a hard time dealing with FP constants.
     bool AllowNewConst = (Level < AfterLegalizeDAG);
 
-    // fold (fadd A, 0) -> A
-    if (ConstantFPSDNode *N1C = isConstOrConstSplatFP(N1))
-      if (N1C->isZero())
-        return N0;
-
     // fold (fadd (fadd x, c1), c2) -> (fadd x, (fadd c1, c2))
     if (N1CFP && N0.getOpcode() == ISD::FADD && N0.getNode()->hasOneUse() &&
         isConstantFPBuildVectorOrConstantFP(N0.getOperand(1)))
       return DAG.getNode(ISD::FADD, DL, VT, N0.getOperand(0),
                          DAG.getNode(ISD::FADD, DL, VT, N0.getOperand(1), N1,
                                      Flags),
                          Flags);
 
SDValue DAGCombiner::visitFSUB(SDNode *N) {
   if (N0CFP && N1CFP)
     return DAG.getNode(ISD::FSUB, dl, VT, N0, N1, Flags);
 
   // fold (fsub A, (fneg B)) -> (fadd A, B)
   if (isNegatibleForFree(N1, LegalOperations, TLI, &Options))
     return DAG.getNode(ISD::FADD, dl, VT, N0,
                        GetNegatedExpression(N1, DAG, LegalOperations), Flags);
 
-  // If 'unsafe math' is enabled, fold lots of things.
-  if (Options.UnsafeFPMath) {
-    // (fsub A, 0) -> A
-    if (N1CFP && N1CFP->isZero())
-      return N0;
-
+  // FIXME: Auto-upgrade the target/function-level option.
+  if (Options.UnsafeFPMath || N->getFlags()->hasNoSignedZeros()) {
     // (fsub 0, B) -> -B
     if (N0CFP && N0CFP->isZero()) {
       if (isNegatibleForFree(N1, LegalOperations, TLI, &Options))
         return GetNegatedExpression(N1, DAG, LegalOperations);
       if (!LegalOperations || TLI.isOperationLegal(ISD::FNEG, VT))
         return DAG.getNode(ISD::FNEG, dl, VT, N1);
     }
+  }
+
+  // If 'unsafe math' is enabled, fold lots of things.
+  if (Options.UnsafeFPMath) {
+    // (fsub A, 0) -> A
+    if (N1CFP && N1CFP->isZero())
+      return N0;
 
     // (fsub x, x) -> 0.0
     if (N0 == N1)
       return DAG.getConstantFP(0.0f, dl, VT);
 
     // (fsub x, (fadd x, y)) -> (fneg y)
     // (fsub x, (fadd y, x)) -> (fneg y)
     if (N1.getOpcode() == ISD::FADD) {

test/CodeGen/X86/negative-sin.ll

 ; CHECK-NEXT:    retq
 ;
   %f = fsub double 0.0, %e
   %g = call double @sin(double %f) readonly
   %h = fsub double 0.0, %g
   ret double %h
 }
 
-; FIXME:
 ; 'fast' implies no-signed-zeros, so the negates fold away.
 ; The 'sin' does not need any fast-math-flags for this transform.
 
 define double @fast(double %e) nounwind {
 ; CHECK-LABEL: fast:
 ; CHECK:       # BB#0:
-; CHECK-NEXT:    pushq %rax
-; CHECK-NEXT:    vxorpd %xmm1, %xmm1, %xmm1
-; CHECK-NEXT:    vsubsd %xmm0, %xmm1, %xmm0
-; CHECK-NEXT:    callq sin
-; CHECK-NEXT:    vxorpd %xmm1, %xmm1, %xmm1
-; CHECK-NEXT:    vsubsd %xmm0, %xmm1, %xmm0
-; CHECK-NEXT:    popq %rax
-; CHECK-NEXT:    retq
+; CHECK-NEXT:    jmp sin
 ;
   %f = fsub fast double 0.0, %e
   %g = call double @sin(double %f) readonly
   %h = fsub fast double 0.0, %g
   ret double %h
 }
 
-; FIXME:
 ; No-signed-zeros is all that we need for this transform.
 
 define double @nsz(double %e) nounwind {
 ; CHECK-LABEL: nsz:
 ; CHECK:       # BB#0:
-; CHECK-NEXT:    pushq %rax
-; CHECK-NEXT:    vxorpd %xmm1, %xmm1, %xmm1
-; CHECK-NEXT:    vsubsd %xmm0, %xmm1, %xmm0
-; CHECK-NEXT:    callq sin
-; CHECK-NEXT:    vxorpd %xmm1, %xmm1, %xmm1
-; CHECK-NEXT:    vsubsd %xmm0, %xmm1, %xmm0
-; CHECK-NEXT:    popq %rax
-; CHECK-NEXT:    retq
+; CHECK-NEXT:    jmp sin
 ;
   %f = fsub nsz double 0.0, %e
   %g = call double @sin(double %f) readonly
   %h = fsub nsz double 0.0, %g
   ret double %h
 }
 
-; FIXME:
 ; The 1st negate is strict, so we can't kill that sub, but the 2nd disappears.
 
 define double @semi_strict1(double %e) nounwind {
 ; CHECK-LABEL: semi_strict1:
 ; CHECK:       # BB#0:
 ; CHECK-NEXT:    pushq %rax
 ; CHECK-NEXT:    vxorpd %xmm1, %xmm1, %xmm1
 ; CHECK-NEXT:    vsubsd %xmm0, %xmm1, %xmm0
 ; CHECK-NEXT:    callq sin
-; CHECK-NEXT:    vxorpd %xmm1, %xmm1, %xmm1
-; CHECK-NEXT:    vsubsd %xmm0, %xmm1, %xmm0
+; CHECK-NEXT:    vxorpd {{.*}}(%rip), %xmm0, %xmm0
 ; CHECK-NEXT:    popq %rax
 ; CHECK-NEXT:    retq
 ;
   %f = fsub double 0.0, %e
   %g = call double @sin(double %f) readonly
   %h = fsub nsz double 0.0, %g
   ret double %h
 }
 
-; FIXME:
 ; The 2nd negate is strict, so we can't kill it. It becomes an add of zero instead.
 
 define double @semi_strict2(double %e) nounwind {
 ; CHECK-LABEL: semi_strict2:
 ; CHECK:       # BB#0:
 ; CHECK-NEXT:    pushq %rax
-; CHECK-NEXT:    vxorpd %xmm1, %xmm1, %xmm1
-; CHECK-NEXT:    vsubsd %xmm0, %xmm1, %xmm0
 ; CHECK-NEXT:    callq sin
 ; CHECK-NEXT:    vxorpd %xmm1, %xmm1, %xmm1
-; CHECK-NEXT:    vsubsd %xmm0, %xmm1, %xmm0
+; CHECK-NEXT:    vaddsd %xmm1, %xmm0, %xmm0
 ; CHECK-NEXT:    popq %rax
 ; CHECK-NEXT:    retq
 ;
   %f = fsub nsz double 0.0, %e
   %g = call double @sin(double %f) readonly
   %h = fsub double 0.0, %g
   ret double %h
 }