InstCombine: Canonicalize fast fmuladd to fmul + fadd

[arsenm]

ping

[hfinkel]

Under what circumstances do we create a 'fast' fmuladd. Maybe we should canonicalize a 'fast' fmuladd into a 'fast' mul and a 'fast' add instead?

lib/Transforms/InstCombine/InstCombineAddSub.cpp

434 ↗

(On Diff #83533)

You're missing a closing ) in the first expression.

test/Transforms/InstCombine/fmuladd-opt.ll

2 ↗

(On Diff #83533)

Missing closing ) here too.

[arsenm]

In D28448#673662, @hfinkel wrote:

Under what circumstances do we create a 'fast' fmuladd. Maybe we should canonicalize a 'fast' fmuladd into a 'fast' mul and a 'fast' add instead?

Clang emits this for a * b + c with -ffast-math -ffp-contract=on

[hfinkel]

In D28448#673675, @arsenm wrote:

In D28448#673662, @hfinkel wrote:

Under what circumstances do we create a 'fast' fmuladd. Maybe we should canonicalize a 'fast' fmuladd into a 'fast' mul and a 'fast' add instead?

Clang emits this for a * b + c with -ffast-math -ffp-contract=on

Interesting. As implemented -ffast-math implies -ffp-contract=fast:

const TargetOptions &Options = DAG.getTarget().Options;
bool AllowFusion =
    (Options.AllowFPOpFusion == FPOpFusion::Fast || Options.UnsafeFPMath);

I think that we should:

1. Have -ffast-math imply -ffp-contract=fast in Clang.

and/or

2. Canonicalize fast fmuladd into fast mul + fast add.

[hfinkel]

LGTM

[arsenm]

r295353

lib/Transforms/InstCombine/InstCombineCalls.cpp

if (isa<ConstantFP>(Arg0) && !isa<ConstantFP>(Arg1)) {
       II->setArgOperand(0, Arg1);
       II->setArgOperand(1, Arg0);
       return II;
     }
     if (Value *V = simplifyMinnumMaxnum(*II))
       return replaceInstUsesWith(*II, V);
     break;
   }
-  case Intrinsic::fma:
   case Intrinsic::fmuladd: {
+    // Canonicalize fast fmuladd to the separate fmul + fadd.
+    if (II->hasUnsafeAlgebra()) {
+      BuilderTy::FastMathFlagGuard Guard(*Builder);
+      Builder->setFastMathFlags(II->getFastMathFlags());
+      Value *Mul = Builder->CreateFMul(II->getArgOperand(0),
+                                       II->getArgOperand(1));
+      Value *Add = Builder->CreateFAdd(Mul, II->getArgOperand(2));
+      Add->takeName(II);
+      return replaceInstUsesWith(*II, Add);
+    }
+
+    LLVM_FALLTHROUGH;
+  }
+  case Intrinsic::fma: {
     Value *Src0 = II->getArgOperand(0);
     Value *Src1 = II->getArgOperand(1);
 
     // Canonicalize constants into the RHS.
     if (isa<Constant>(Src0) && !isa<Constant>(Src1)) {
       II->setArgOperand(0, Src1);
       II->setArgOperand(1, Src0);
       std::swap(Src0, Src1);

test/Transforms/InstCombine/fma.ll

 define float @fmuladd_fneg_x_fneg_y(float %x, float %y, float %z) {
   %x.fneg = fsub float -0.0, %x
   %y.fneg = fsub float -0.0, %y
   %fmuladd = call float @llvm.fmuladd.f32(float %x.fneg, float %y.fneg, float %z)
   ret float %fmuladd
 }
 
 ; CHECK-LABEL: @fmuladd_fneg_x_fneg_y_fast(
-; CHECK: %fmuladd = call fast float @llvm.fmuladd.f32(float %x, float %y, float %z)
+; CHECK-NEXT: %1 = fmul fast float %x, %y
+; CHECK-NEXT: %fmuladd = fadd fast float %1, %z
 define float @fmuladd_fneg_x_fneg_y_fast(float %x, float %y, float %z) {
   %x.fneg = fsub float -0.0, %x
   %y.fneg = fsub float -0.0, %y
   %fmuladd = call fast float @llvm.fmuladd.f32(float %x.fneg, float %y.fneg, float %z)
   ret float %fmuladd
 }
 
 ; CHECK-LABEL: @fmuladd_fneg_const_fneg_y(

 ; CHECK: %fmuladd = call float @llvm.fmuladd.f32(float %x, float %x, float %z)
 define float @fmuladd_fabs_x_fabs_x(float %x, float %z) {
   %x.fabs = call float @llvm.fabs.f32(float %x)
   %fmuladd = call float @llvm.fmuladd.f32(float %x.fabs, float %x.fabs, float %z)
   ret float %fmuladd
 }
 
 ; CHECK-LABEL: @fmuladd_fabs_x_fabs_x_fast(
-; CHECK: %fmuladd = call fast float @llvm.fmuladd.f32(float %x, float %x, float %z)
+; CHECK-NEXT: %1 = fmul fast float %x, %x
+; CHECK-NEXT: %fmuladd = fadd fast float %1, %z
 define float @fmuladd_fabs_x_fabs_x_fast(float %x, float %z) {
   %x.fabs = call float @llvm.fabs.f32(float %x)
   %fmuladd = call fast float @llvm.fmuladd.f32(float %x.fabs, float %x.fabs, float %z)
   ret float %fmuladd
 }
 
 ; CHECK-LABEL: @fma_k_y_z(
 ; CHECK: %fma = call float @llvm.fma.f32(float %y, float 4.000000e+00, float %z)
 define float @fma_k_y_z(float %y, float %z) {
   %fma = call float @llvm.fma.f32(float 4.0, float %y, float %z)
   ret float %fma
 }
 
 ; CHECK-LABEL: @fma_k_y_z_fast(
 ; CHECK: %fma = call fast float @llvm.fma.f32(float %y, float 4.000000e+00, float %z)
 define float @fma_k_y_z_fast(float %y, float %z) {
   %fma = call fast float @llvm.fma.f32(float 4.0, float %y, float %z)
   ret float %fma
 }
 
 ; CHECK-LABEL: @fmuladd_k_y_z_fast(
-; CHECK: %fmuladd = call fast float @llvm.fmuladd.f32(float %y, float 4.000000e+00, float %z)
+; CHECK: %1 = fmul fast float %y, 4.000000e+00
+; CHECK-NEXT: %fmuladd = fadd fast float %1, %z
 define float @fmuladd_k_y_z_fast(float %y, float %z) {
   %fmuladd = call fast float @llvm.fmuladd.f32(float 4.0, float %y, float %z)
   ret float %fmuladd
 }
 
 ; CHECK-LABEL: @fma_1_y_z(
 ; CHECK: %fma = fadd float %y, %z
 define float @fma_1_y_z(float %y, float %z) {