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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineCombinerPattern.h:182-183
+
+  FNMADDS,
+  FNMADDD,
 };
----------------
MattDevereau wrote:
> MattDevereau wrote:
> > sdesmalen wrote:
> > > Having two patterns, one for 32-bit values, and one for 64-bit values doesn't match what was done for FMSUB/FNMSUB. Can these be merged into 1 and use the register class used for the operands to determine which instruction to use?
> > I'm not sure what you mean? MachineCombinerPattern::FNMSUB isn't used in  `AArch64InstrInfo::genAlternativeCodeSequence` at all. The way FNMSUB is combined in this function has `MachineCombinerPattern::FMULSUBH_OP1` `MachineCombinerPattern::FMULSUBS_OP1` and `MachineCombinerPattern::FMULSUBD_OP1:` which all describe the number of bits. I'd need to do something like
> > 
> > 
> > ```
> > auto RC = MRI.getRegClass(MAD->getOperand(0).getReg());
> > if RC == 64bit
> >   opc = FNMADDDrrr
> > else if RC == 32bit
> >   opc = FNMADDSrrr
> > ```
> > which I can't see a clear example of
> I managed to achieve this with `Arch64::FPR32RegClass.hasSubClassEq(RC)` in the end
Thanks!


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5441
+  }
+}
+
----------------
nit: I don't know if some compilers could warn about the function not returning a value, but maybe you could `break` in the default case instead and `return false` here. Either that, or add a `llvm_unreachable()` here to make it clear that the function should have returned at this point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149260