[PATCH] D43090: GlobalISel: IRTranslate llvm.fmuladd.* intrinsic

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:765
+        TLI.isFMAFasterThanFMulAndFAdd(TLI.getValueType(*DL, CI.getType()))) {
+      MIRBuilder.buildInstr(TargetOpcode::G_FMA, Dst, Op0, Op1, Op2);
+    } else {
----------------
volkan wrote:
> aditya_nandakumar wrote:
> > qcolombet wrote:
> > > So far we stayed away of "optimizations" in the IRTranslator.
> > > I wonder if this part of the lowering should be part of a combiner instead.
> > > 
> > > In particular, the language reference says:
> > > Fusion is not guaranteed, even if the target platform supports it.
> > > 
> > > What do you think?
> > That's also what I said when I spoke with him in person. Unfortunately, currently there's no other place to put it (to avoid duplicating across all targets).
> Yes, fusion is not guaranteed but I think we still need to translate this to either one of the options. If IRTranslator doesn't do that, every target would have to handle this in somewhere else as the combiner is optional.
> Unfortunately, currently there's no other place to put it (to avoid duplicating across all targets).

Couldn't this be offered as a generic combine (I mean G_FMUL (G_FADD) => FMA)?

I know this is optional, but this would be shared and is also sort of an optimization.

>  fusion is not guaranteed but I think we still need to translate this to either one of the options.

Yes, what I am suggesting is that fmuladd could always be expanded into G_FMUL (G_FADD). Just remove the FMA part.

> If IRTranslator doesn't do that, every target would have to handle this in somewhere else as the combiner is optional.

Just to make just we are talking about the same thing, I agree we need to *always* lower that intrinsic in IRTranslator, but G_FMUL(G_FADD) is sufficient for that part.


https://reviews.llvm.org/D43090