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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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 ||
----------------
matejam wrote:
> 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?
Right, the FP mode is not a property of the target. It's a property of the FP mode of the function, which is contextually valid or not. The legalizer rules are not allowed to differ based on context like that


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

https://reviews.llvm.org/D93305