[PATCH] D84930: [AArch64] Consider instruction-level contract FMFs in combiner patterns.

Mon Aug 3 08:06:06 PDT 2020

fhahn added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:4123
+  // We can fuse FMUL & FADD, if fusion is either allowed globally by the target
+  // options or if both instructions have the contract fast-math flag.
+  TargetOptions Options = Root.getParent()->getParent()->getTarget().Options;
----------------
dmgreen wrote:
> Do both instructions need the fast-math flag, or just the add? I guess it's safer to check both, but the code would end up being simpler if it was just the add.
> 
> Reading DAGCombiner::visitFADDForFMACombine it's a bit messy, but seems like it only really considers the add:
> https://godbolt.org/z/3ejrhE
> Reading the code though, I'm not sure that's really what it is intending to do.
Indeed it seems like it should be sufficient on the FADD/FSUB. Given that is what DAGCOmbine does, it should be safe to do so in the machine combiner too (plus it ensures consistency between -O3 and other levels). It also makes the code much simpler. 

I also found `llvm/test/CodeGen/AArch64/neon-fma-FMF.ll`, which tests with different `contract` flags and it specifically checks for fusion with contract on `FADD` and no fusion with contract on `FMUL`. I added `-O3`, which checks the machine-combiner path as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84930