GlobalISel: IRTranslate llvm.fmuladd.* intrinsic

[qcolombet]

lib/CodeGen/GlobalISel/IRTranslator.cpp

765

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?

[aditya_nandakumar]

lib/CodeGen/GlobalISel/IRTranslator.cpp

765

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).

[volkan]

lib/CodeGen/GlobalISel/IRTranslator.cpp

765

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.

[qcolombet]

lib/CodeGen/GlobalISel/IRTranslator.cpp

765

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.

[dsanders]

lib/CodeGen/GlobalISel/IRTranslator.cpp

765

My initial thought was that this ought to be a combine. However, I think this intrinsic is intended as a way to say you're expecting either a fused-multiply-add or a multiply-add (some targets have both) but don't care whether the rounding step happens or not, you just want whatever is fastest. We could handle this as a combine but it would cost compile-time to find the optimization opportunity in the expanded code and if we go that route then the intrinsic doesn't really serve a purpose compared to separate mul/add LLVM-IR instructions anymore.

[qcolombet]

lib/CodeGen/GlobalISel/IRTranslator.cpp

765

Arguably the compiler knows better (critical path, resources usages, and whatnot) and that wouldn't be the first time that we silently drop an intrinsic in favor of regular instructions (we did that for pretty much all the intel shuffle instructions for instance IIRC). Thus, keeping this as an optimization makes sense to me.

That said, I also get your point and I am fine with either solutions. Given it looks like we have 1 vs. 3 votes, I guess we keep the fancy expansion here :).

Thoughts?

[rovka]

lib/CodeGen/GlobalISel/IRTranslator.cpp

765

FWIW, I'm with Quentin on this one. The LangRef says it's up to the code generator to decide:

The ‘llvm.fmuladd.*’ intrinsic functions represent multiply-add expressions that can be fused if the code generator determines that (a) the target instruction set has support for a fused operation, and (b) that the fused operation is more efficient than the equivalent, separate pair of mul and add instructions.

I'd also nominate the fsub -0.0, x -> fneg transform to be moved from the irtranslator into the combiner.

That's just my 2 cents, I don't mind terribly if the patch goes in as is (or maybe with a FIXME) and we revisit some other time.

[dsanders]

lib/CodeGen/GlobalISel/IRTranslator.cpp

765

I'm ok with it either way, but if we're going to defer it then we should probably translate it to an intrinsic and legalize it into the mul+add/fma form.

I'd also nominate the fsub -0.0, x -> fneg transform to be moved from the irtranslator into the combiner.

I agree. IIRC, a lot of targets like the fsub representation but I know there's a few that like fneg. We need to handle this somewhere where the target can decide.

[volkan]

lib/CodeGen/GlobalISel/IRTranslator.cpp

765

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.

Oh, I thought you were suggesting translating this in the combiner pass. In that case, we can go for either one of the options and revisit this later as Diana suggested.

[qcolombet]

lib/CodeGen/GlobalISel/IRTranslator.cpp

765

Agree. I leave the final decision to you.
At the very least, we want a FIXME here.

[volkan]

Added a TODO about moving a part of the translation to the combiner.

lib/CodeGen/GlobalISel/IRTranslator.cpp

case Intrinsic::log2:
     return true;
   case Intrinsic::fma:
     MIRBuilder.buildInstr(TargetOpcode::G_FMA)
         .addDef(getOrCreateVReg(CI))
         .addUse(getOrCreateVReg(*CI.getArgOperand(0)))
         .addUse(getOrCreateVReg(*CI.getArgOperand(1)))
         .addUse(getOrCreateVReg(*CI.getArgOperand(2)));
     return true;
+  case Intrinsic::fmuladd: {
+    const TargetMachine &TM = MF->getTarget();
+    const TargetLowering &TLI = *MF->getSubtarget().getTargetLowering();
+    unsigned Dst = getOrCreateVReg(CI);
+    unsigned Op0 = getOrCreateVReg(*CI.getArgOperand(0));
+    unsigned Op1 = getOrCreateVReg(*CI.getArgOperand(1));
+    unsigned Op2 = getOrCreateVReg(*CI.getArgOperand(2));
+    if (TM.Options.AllowFPOpFusion != FPOpFusion::Strict &&
+        TLI.isFMAFasterThanFMulAndFAdd(TLI.getValueType(*DL, CI.getType()))) {
+      // TODO: Revisit this to see if we should move this part of the
+      // lowering to the combiner.
+      MIRBuilder.buildInstr(TargetOpcode::G_FMA, Dst, Op0, Op1, Op2);
+    } else {
+      LLT Ty = getLLTForType(*CI.getType(), *DL);
+      auto FMul = MIRBuilder.buildInstr(TargetOpcode::G_FMUL, Ty, Op0, Op1);
+      MIRBuilder.buildInstr(TargetOpcode::G_FADD, Dst, FMul, Op2);
+    }
+    return true;
+  }
   case Intrinsic::memcpy:
   case Intrinsic::memmove:
   case Intrinsic::memset:
     return translateMemfunc(CI, MIRBuilder, ID);
   case Intrinsic::eh_typeid_for: {
     GlobalValue *GV = ExtractTypeInfo(CI.getArgOperand(0));
     unsigned Reg = getOrCreateVReg(CI);
     unsigned TypeID = MF->getTypeIDFor(GV);

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator-fmuladd.ll

+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -o - -verify-machineinstrs -global-isel -stop-after=irtranslator -fp-contract=fast %s | FileCheck %s --check-prefix=FPFAST
+; RUN: llc -o - -verify-machineinstrs -global-isel -stop-after=irtranslator -fp-contract=off %s | FileCheck %s --check-prefix=FPOFF
+target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64--"
+
+define float @test_fmuladd(float %x, float %y, float %z) {
+  ; FPFAST-LABEL: name: test_fmuladd
+  ; FPFAST: bb.1 (%ir-block.0):
+  ; FPFAST:   liveins: $s0, $s1, $s2
+  ; FPFAST:   [[COPY:%[0-9]+]]:_(s32) = COPY $s0
+  ; FPFAST:   [[COPY1:%[0-9]+]]:_(s32) = COPY $s1
+  ; FPFAST:   [[COPY2:%[0-9]+]]:_(s32) = COPY $s2
+  ; FPFAST:   [[FMA:%[0-9]+]]:_(s32) = G_FMA [[COPY]], [[COPY1]], [[COPY2]]
+  ; FPFAST:   $s0 = COPY [[FMA]](s32)
+  ; FPFAST:   RET_ReallyLR implicit $s0
+  ; FPOFF-LABEL: name: test_fmuladd
+  ; FPOFF: bb.1 (%ir-block.0):
+  ; FPOFF:   liveins: $s0, $s1, $s2
+  ; FPOFF:   [[COPY:%[0-9]+]]:_(s32) = COPY $s0
+  ; FPOFF:   [[COPY1:%[0-9]+]]:_(s32) = COPY $s1
+  ; FPOFF:   [[COPY2:%[0-9]+]]:_(s32) = COPY $s2
+  ; FPOFF:   [[FMUL:%[0-9]+]]:_(s32) = G_FMUL [[COPY]], [[COPY1]]
+  ; FPOFF:   [[FADD:%[0-9]+]]:_(s32) = G_FADD [[FMUL]], [[COPY2]]
+  ; FPOFF:   $s0 = COPY [[FADD]](s32)
+  ; FPOFF:   RET_ReallyLR implicit $s0
+  %res = call float @llvm.fmuladd.f32(float %x, float %y, float %z)
+  ret float %res
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare float @llvm.fmuladd.f32(float, float, float) #0
+
+attributes #0 = { nounwind readnone speculatable }