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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Just took the opportunity to have a quick look at this patch, given that it recently got reverted.



================
Comment at: llvm/include/llvm/CodeGen/MachineCombinerPattern.h:182-183
+
+  FNMADDS,
+  FNMADDD,
 };
----------------
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?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5421
+    MachineOperand &MO = Root.getOperand(1);
+    MachineInstr *MI = MRI.getUniqueVRegDef(MO.getReg());
+    if ((MI->getOpcode() == Opcode) &&
----------------
You'll need to add a check that `MI != nullptr`

>From the doxygen comment:

  /// getUniqueVRegDef - Return the unique machine instr that defines the
  /// specified virtual register or null if none is found.  If there are
  /// multiple definitions or no definition, return null.



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5437
+  case AArch64::FNEGDr:
+    Found |= Match(AArch64::FMADDDrrr, MachineCombinerPattern::FNMADDD);
+    break;
----------------
nit: if you just `return Match(..)` here, then you can avoid the variable `Found`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149260