easing the constraint for isNegatibleForFree and GetNegatedExpression

[mcberg2017]

Summary

Here we relax the old constraint which utilized unsafe with the TargetOption flag HonorSignDependentRoundingFPMathOption, with the assertion that unsafe is no longer needed or never was required for correctness on FDIV/FMUL.

[mcberg2017]

Tests which depend upon HonorSignDependentRoundingFPMathOption for FMUL/FDIV:

test/CodeGen/AMDGPU/fmuladd.f16.ll
test/CodeGen/AMDGPU/fmuladd.f32.ll
test/CodeGen/AMDGPU/mad-mix.ll
test/CodeGen/AMDGPU/v_mac.ll
test/CodeGen/AMDGPU/v_mac_f16.ll
test/CodeGen/PowerPC/fneg.ll
test/CodeGen/X86/dag-fmf-cse.ll

I determined this by switching the value of the flag test to the opposite boolean value as in the code in the view.

[mcberg2017]

There is also some discussion needed if we want to add a guard FMF flag to FMUL and FDIV either together or as separate and different flags within negation context.

[mcberg2017]

I will be adding a test for the fadd path shortly.

[mcberg2017]

With the test that hits negation context for both unsafe and fmf for fadd.

[mcberg2017]

Note, once D47909 is checked in I will add the cleanup code for getNode here which covers fadd, fmul and fsub.

[spatel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

722

Can you explain this diff? Is there an existing test for this? If not, please add 1.
IIUC, UnsafeFPMath overrode HonorSignDependenRoundingFPMathOption. Ie, if we had unsafe, then it didn't matter what honor-sign said, it was safe to do this transform. Now, we're removing that override?

[mcberg2017]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

722

There is a list above in the earlier comments about tests which are modified by HonorSignDependentRoundingFPMathOption's state so we are covered.

By and large you statement is correct.

My question then becomes: Is it sufficient for the case where fmf is controlling optimizations on IR, i.e. where Unsafe is false, that HonorSignDependentRoundingFPMathOption's state is sufficient to proceed?

Or should we add a fmf constraint for the case where Unsafe is false to allow more optimization? If so, nsz, other flags" Possibly different ones for fmul and fdiv.

If HonorSignDependentRoundingFPMathOption is sufficient, then this code review becomes only about fadd.

[spatel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

722

The listed tests are affected by removing this fold (inverting the logic), but that doesn't tell us what effect HonorSignDependentRoundingFPMathOption was supposed to have when combined with unsafe math. We need tests that toggle each of those params to make this change.

The only tests that I see that use that target option are:
test/CodeGen/ARM/build-attributes.ll
test/CodeGen/ARM/fnmul.ll

cc @efriedma in case he has any insight or historical perspective.

You can move the fadd diff into its own patch, so that doesn't have to stall while determining the fmul/fdiv part of this.

[efriedma]

HonorSignDependentRounding is pretty clearly a failed experiment; I'd suggest killing it completely (in a separate patch).

[mcberg2017]

Given Eli's feedback, I think we can keep fadd together here, I have removed the checks fully and added a test.

[mcberg2017]

test/CodeGen/ARM/fnmul.ll

6 ↗

(On Diff #151270)

Now the baseline question becomes, do we want fmul/fdiv negation only under Unsafe or do we want to ungate it?
If just Unsafe, I modify this test case and split fadd out.

[spatel]

test/CodeGen/ARM/fnmul.ll

6 ↗

(On Diff #151270)

The fmul/fdiv negation fold is safe without any flags. In IR, we currently canonicalize the other way for fmul:

// Sink negation: -X * Y --> -(X * Y)

...but I'd still prefer that we split this change up from the fadd diff to reduce risk.

[mcberg2017]

Removed fadd from this context

[spatel]

LGTM.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (!LegalOperations)
       return 1;
 
     // Don't invert constant FP values after legalization unless the target says
     // the negated constant is legal.
     return TLI.isOperationLegal(ISD::ConstantFP, VT) ||
       TLI.isFPImmLegal(neg(cast<ConstantFPSDNode>(Op)->getValueAPF()), VT);
   }
   case ISD::FADD:
-    // FIXME: determine better conditions for this xform.
-    if (!Options->UnsafeFPMath) return 0;
+    if (!Options->UnsafeFPMath && !Flags.hasNoSignedZeros())
+      return 0;
 
     // After operation legalization, it might not be legal to create new FSUBs.
     if (LegalOperations && !TLI.isOperationLegalOrCustom(ISD::FSUB, VT))
       return 0;
 
     // fold (fneg (fadd A, B)) -> (fsub (fneg A), B)
     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->NoSignedZerosFPMath &&
         !Flags.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 (Options->HonorSignDependentRoundingFPMathOption) return 0;
 
     // fold (fneg (fmul X, Y)) -> (fmul (fneg X), Y) or (fmul X, (fneg Y))
     if (char V = isNegatibleForFree(Op.getOperand(0), LegalOperations, TLI,
                                     Options, Depth + 1))
       return V;
 
     return isNegatibleForFree(Op.getOperand(1), LegalOperations, TLI, Options,
                               Depth + 1);
static SDValue GetNegatedExpression(SDValue Op, SelectionDAG &DAG,
   default: llvm_unreachable("Unknown code");
   case ISD::ConstantFP: {
     APFloat V = cast<ConstantFPSDNode>(Op)->getValueAPF();
     V.changeSign();
     return DAG.getConstantFP(V, SDLoc(Op), Op.getValueType());
   }
   case ISD::FADD:
     // FIXME: determine better conditions for this xform.
-    assert(Options.UnsafeFPMath);
+    assert(Options.UnsafeFPMath || Flags.hasNoSignedZeros());
 
     // fold (fneg (fadd A, B)) -> (fsub (fneg A), B)
     if (isNegatibleForFree(Op.getOperand(0), LegalOperations,
                            DAG.getTargetLoweringInfo(), &Options, Depth+1))
       return DAG.getNode(ISD::FSUB, SDLoc(Op), Op.getValueType(),
                          GetNegatedExpression(Op.getOperand(0), DAG,
                                               LegalOperations, Depth+1),
                          Op.getOperand(1), Flags);
if (ConstantFPSDNode *N0CFP = dyn_cast<ConstantFPSDNode>(Op.getOperand(0)))
         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);
 
   case ISD::FMUL:
   case ISD::FDIV:
-    assert(!Options.HonorSignDependentRoundingFPMath());
+    assert(!Options.HonorSignDependentRoundingFPMathOption);
 
     // fold (fneg (fmul X, Y)) -> (fmul (fneg X), Y)
     if (isNegatibleForFree(Op.getOperand(0), LegalOperations,
                            DAG.getTargetLoweringInfo(), &Options, Depth+1))
       return DAG.getNode(Op.getOpcode(), SDLoc(Op), Op.getValueType(),
                          GetNegatedExpression(Op.getOperand(0), DAG,
                                               LegalOperations, Depth+1),
                          Op.getOperand(1), Flags);