[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/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:
> matejam wrote:
> > 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.
> I was hoping that hasFP64FP16Denormals etc would be tested in AMDGPULegalizerInfo::AMDGPULegalizerInfo, where it decides whether G_FMAD is legal, so the normal isLegal* functions would work. But now I see that hasFP64FP16Denormals depends on function attributes, so it can't be done that way.
So do you think I should keep it this way?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3939
+  bool HasFMA = TLI.isFMAFasterThanFMulAndFAdd(*MF, DstType) &&
+                (!LegalOperations || isLegal({TargetOpcode::G_FMA, {DstType}}));
+
----------------
foad wrote:
> matejam wrote:
> > 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.
> I mean `!LegalOperations || isLegal(...)` is the same as `isLegalOrBeforeLegalization(...)`. Because `LegalOperations` is just another name for `LI`.
You're right, I'll change that. Thanks!


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

https://reviews.llvm.org/D93305