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

[Mateja Marjanovic via Phabricator via llvm-commits]

matejam 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;
----------------
foad wrote:
> Comment seems wrong. Should it be "... running after legalization ..."?
That seems ok, I'll change that.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2717
+  virtual bool isFMAFasterThanFMulAndFAdd(const MachineFunction &MF,
+                                          const LLT) const {
+    return false;
----------------
foad wrote:
> "const LLT" seems weird for a parameter type. Is there a reason for the const?
I don't think there is a good reason for that, I'll change that.


================
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 ||
----------------
foad wrote:
> "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?
I'll change the const LLT.
I tried to find a way to call methods like "hasMadF16", "hasFP64FP16Denormals", etc. from CombinerHelper, but using adjusted SelectionDAG legalization code seemed much easier and prettier to me.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3906
+
+bool CombinerHelper::isContractableFMul(const MachineInstr &MI,
+                                        bool AllowFusionGlobally) {
----------------
foad wrote:
> This could be static, just like isContractable?
It probably should, I'll change that.


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


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


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3939
+  bool HasFMA = TLI.isFMAFasterThanFMulAndFAdd(*MF, DstType) &&
+                (!LegalOperations || isLegal({TargetOpcode::G_FMA, {DstType}}));
+
----------------
foad wrote:
> This expression is exactly what "isLegalOrBeforeLegalization" does, so you didn't need to create isLegal.
"isLegalOrBeforeLegalization" returns the value of "!LI || LI->getAction(Query).Action == LegalizeActions::Legal"
and  isLegal returns the value of "LI && LI->getAction(Query).Action == LegalizeActions::Legal", so I thinks it's not exactly the same.


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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3330
+  unsigned Mods;
+  std::tie(Src, Mods) = selectVOP3ModsImpl(Root);
   return {{
----------------
foad wrote:
> Can you submit this change as a separate patch? (Does it affect any existing lit tests?)
Yes, it should be in a separate patch, it was added here by mistake.


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

https://reviews.llvm.org/D93305