[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/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?
I'm not sure. Alternatively you could just check "isLegalOrBeforeLegalizer" on the FMAD instructions. Then pre-legalization we would always combine to FMAD, and then the legalizer might decide that the FMAD is not legal and turn it into either MUL+ADD or FMA. Would that work?


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

https://reviews.llvm.org/D93305