[PATCH] D130232: Reassoc FMF should not optimize FMA(a, 0, b) to (b)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15036
 
-  if (UnsafeFPMath) {
+  if (Options.UnsafeFPMath) {
     if (N0CFP && N0CFP->isZero())
----------------
svenvh wrote:
> spatel wrote:
> > We are slowly trying to get away from using the global settings by replacing those checks with fast-math-flags. Can we do that here?
> > 
> > This should match the conditions that we use to simplify X * 0.0. That's done via DAG.simplifyFPBinop():
> > 
> >   // X * 0.0 --> 0.0
> >   if (Opcode == ISD::FMUL && Flags.hasNoNaNs() && Flags.hasNoSignedZeros())
> >     if (YC->getValueAPF().isZero())
> >       return getConstantFP(0.0, SDLoc(Y), Y.getValueType());
> > 
> Sure, but is it okay to do the modernization in a separate followup commit?  This change just intends to fix the immediate bug (i.e. restore the status quo to before 8e570c3390d20, which is where this bug was introduced).
> 
> I guess the followup patch will be like this then?
> ```
> @@ -15026,7 +15026,7 @@ SDValue DAGCombiner::visitFMA(SDNode *N) {
>         CostN1 == TargetLowering::NegatibleCost::Cheaper))
>      return DAG.getNode(ISD::FMA, DL, VT, NegN0, NegN1, N2);
>  
> -  if (Options.UnsafeFPMath) {
> +  if (N->getFlags().hasNoNaNs() && N->getFlags().hasNoSignedZeros()) {
>      if (N0CFP && N0CFP->isZero())
>        return N2;
>      if (N1CFP && N1CFP->isZero())
> ```
Right - it should be a one-line patch. Just need to see if any regression tests fail with that. If you want to do that as another patch, please put a FIXME note on it in this patch, so we don't lose track that it needs to be fixed.


================
Comment at: llvm/test/CodeGen/AArch64/neon-fma-FMF.ll:64
+; CHECK: fmadd
+; CHECK-NOT: fadd
+; CHECK-NOT: fsub
----------------
Prefer to add positive CHECK lines for correctness rather than NOT lines for the absence of one particular kind of wrong.
It would be great to update this whole file using the script at "utils/update_llc_test_checks.py" - that way, it's complete and easier to update.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130232/new/

https://reviews.llvm.org/D130232