[PATCH] D69547: DAG: Add function context to isFMAFasterThanFMulAndFAdd

Thu Oct 31 09:06:27 PDT 2019

cameron.mcinally added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2549
     return false;
   }

----------------
arsenm wrote:
> cameron.mcinally wrote:
> > Agreed, the AArch64 requirements are ugly.
> > 
> > It doesn't look like the AArch64 hooks need the Function arg. Could we just make the MachineFunction argument optional; to consolidate the two functions? E.g.
> > 
> > ```
> >   virtual bool isFMAFasterThanFMulAndFAdd(EVT, const MachineFunction &MF = nullptr) const {
> > ```
> > 
> > Or is that not allowed with some virtual functions? I can't remember the details...
> > 
> That would still be using EVT, which I would prefer to avoid spreading. I also wouldn't like using an optional argument here, because then there's a risk of new uses not passing it
The intention seems correct, but I don't have a lot of confidence reviewing a change like that. Maybe @bogner does?

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

https://reviews.llvm.org/D69547