[PATCH] D149260: [AArch64] Emit FNMADD instead of FNEG(FMADD)

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5437
+  case AArch64::FNEGDr:
+    Found |= Match(AArch64::FMADDDrrr, MachineCombinerPattern::FNMADDD);
+    break;
----------------
craig.topper wrote:
> MattDevereau wrote:
> > MattDevereau wrote:
> > > david-arm wrote:
> > > > Shouldn't we also be doing FMSUB here too, to ensure we are also requiring the nsz flag for the existing fnmsub combine?
> > > I don't think FNEG(FMSUB) and FNMSUB are equivalent, so we can't assume FNMSUB from a FNEG perspective like we can with FNMAD:
> > > 
> > > FNMSUB = (a * b) - c                    //e.g. (12 * 4) - 8 = 32
> > > FNEG(FMSUB) = -(-(a*b) + c)        //e.g. -(12 * 4) + 8 = 40
> > Furthering on from this FNMSUB and FNEG(FMSUB) are equal (oops, calcuator misuse) but there's already patterns that exist for emitting FNMSUBs. Adding tests similar to the ones added in this patch don't generate FNEG(FMSUB) but instead correctly emit FNMSUB already.
> They may not be equal for cases where combinations where a*b==0.0 or -0.0 and c==0.0 or -0.0.
> 
> ```
> fneg(fadd A, B) != fadd(fneg(A), fneg(B)) if (A==0.0 and B==-0.0) or (A==-0.0 and B=0.0)
> 
> fneg(fadd 0.0, -0.0) -> fneg(0.0) -> -0.0
> fadd(fneg(0.0), fneg(-0.0)) -> fadd(-0.0, 0.0) -> 0.0
> ```
> 
> This is why I asked for the no signed zeros check.
I guess its's really

fneg(fadd A,B) != fadd(fneg(A), fneg(B)) if (A==-B)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149260