[PATCH] D93305: [AMDGPU][GlobalISel] Transform (fadd (fmul x, y), z) -> (fma x, y, z)

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:110-111
 
+  /// \return true if the combine is running prior to legalization and if \p
+  /// Query is legal on the target.
+  bool isLegal(const LegalityQuery &Query) const;
----------------
Comment seems wrong. Should it be "... running after legalization ..."?


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2717
+  virtual bool isFMAFasterThanFMulAndFAdd(const MachineFunction &MF,
+                                          const LLT) const {
+    return false;
----------------
"const LLT" seems weird for a parameter type. Is there a reason for the const?


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2729
+  /// TargetOpcode::G_FMUL which will be distributed into an fadd/fsub.
+  virtual bool isFMADLegal(const MachineInstr &MI, const LLT Ty) const {
+    assert((MI.getOpcode() == TargetOpcode::G_FADD ||
----------------
"const LLT" seems weird.

But why do we need this isFMADLegal function, which calls into selectiondag legalization code? Can't you use CombinerHelper isLegal methods instead?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3906
+
+bool CombinerHelper::isContractableFMul(const MachineInstr &MI,
+                                        bool AllowFusionGlobally) {
----------------
This could be static, just like isContractable?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3910
+    return false;
+  return AllowFusionGlobally || MI.getFlag(MachineInstr::MIFlag::FmReassoc) ||
+         MI.getFlag(MachineInstr::MIFlag::FmContract);
----------------
`return AllowFusionGlobally || isContractable(MI);`?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3927
+
+  auto *MF = MI.getParent()->getParent();
+  const auto &TLI = *MF->getSubtarget().getTargetLowering();
----------------
MI.getMF()


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3939
+  bool HasFMA = TLI.isFMAFasterThanFMulAndFAdd(*MF, DstType) &&
+                (!LegalOperations || isLegal({TargetOpcode::G_FMA, {DstType}}));
+
----------------
This expression is exactly what "isLegalOrBeforeLegalization" does, so you didn't need to create isLegal.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3951
+  if (!AllowFusionGlobally && !isContractable(MI) &&
+      !MI.getFlag(MachineInstr::MIFlag::FmReassoc))
+    return false;
----------------
!isContractable already implies !MI.FmReassoc so you don't need to check it again here.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3330
+  unsigned Mods;
+  std::tie(Src, Mods) = selectVOP3ModsImpl(Root);
   return {{
----------------
Can you submit this change as a separate patch? (Does it affect any existing lit tests?)


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

https://reviews.llvm.org/D93305