[InstCombine] allow fdiv folds with less than fully 'fast' ops

[spatel]

Summary

These folds raise 2 questions for me as I'm trying to eliminate the use of 'isFast' here:

1. Do we need both 'arcp' and 'reassoc' to do this? My understanding is that -freciprocal-math allows this:

(X / Y) / Z --> X * (1.0 / Y) * (1.0 / Z)

...which wouldn't be useful here in InstCombine, but that might be a target-specific transform for the DAG.
If we are allowed to reassociate, then we reduce the above as:
X * (1.0 / Y) * (1.0 / Z) --> X * (1.0 / (Y * Z)) --> (X * 1.0) / (Y * Z) --> X / (Y * Z)

...but gcc currently says we can do this with just -freciprocal-math:
https://godbolt.org/g/vx7JnL

Should we allow this with just 'arcp'?

2. We're applying the intersected FMF to the new fmul op. That results in strict fmul ops in the regression tests. Is that too conservative? Strict ops inhibit further reassociation and might cause more FMF removal in early-cse (because that also does FMF intersection).

[wristow]

...but gcc currently says we can do this with just -freciprocal-math

Regardless of what gcc is doing in this case, I think both 'reassoc' and 'arcp' are needed here. So I think your code is right.

[spatel]

In D43398#1011150, @wristow wrote:

...but gcc currently says we can do this with just -freciprocal-math

Regardless of what gcc is doing in this case, I think both 'reassoc' and 'arcp' are needed here. So I think your code is right.

Thanks. I wasn't sure what weight should be given to matching gcc-based customer expectations, so I thought it was worth noting here. Either way, this gets us closer to their behavior because we at least don't have to enable all of -ffast-math.

Any thoughts about the FMF on the fmul? It can be a separate patch if that's preferred, but I think this is the only transform around here that is intersecting FMF. We always propagate flags from the last op to intermediate ops in other cases. That makes more sense to me because the new op only exists to produce an intermediate result for our transformed fdiv, so it should have the same relaxations as the fdiv.

[spatel]

Patch updated:
No functional change from the last rev, but I cleaned up the surrounding folds, so this is the last use of isFast() in visitFDiv(). There's also one more test affected by this change.

[wristow]

Any thoughts about the FMF on the fmul? It can be a separate patch if that's preferred, but I think this is the only transform around here that is intersecting FMF. We always propagate flags from the last op to intermediate ops in other cases. That makes more sense to me because the new op only exists to produce an intermediate result for our transformed fdiv, so it should have the same relaxations as the fdiv.

That makes sense to me, too. And from my POV, it's sensible to do this in the same patch.

[spatel]

Patch updated:
Propagate FMF from the last fdiv to the new fmul.

[MatzeB]

Your reasoning makes sense to me. And I think it's fine to push this given no floating point expert objects.

[spatel]

Thanks for the reviews. I think we have unofficial approval. Can someone click the button to 'Accept' and/or issue the official 'looks good...'?

[wristow]

LGTM

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp

if (Fold && isNormalFp(Fold)) {
       Instruction *R = CreateDiv ? BinaryOperator::CreateFDiv(Fold, X)
                                  : BinaryOperator::CreateFMul(X, Fold);
       R->setFastMathFlags(I.getFastMathFlags());
       return R;
     }
     return nullptr;
   }
 
-  if (AllowReassociate) {
+  if (I.hasAllowReassoc() && I.hasAllowReciprocal()) {
     Value *X, *Y;
     if (match(Op0, m_OneUse(m_FDiv(m_Value(X), m_Value(Y)))) &&
         (!isa<Constant>(Y) || !isa<Constant>(Op1))) {
       // (X / Y) / Z => X / (Y * Z)
       Value *YZ = Builder.CreateFMul(Y, Op1);
       if (auto *YZInst = dyn_cast<Instruction>(YZ)) {
         FastMathFlags FMFIntersect = I.getFastMathFlags();
         FMFIntersect &= cast<Instruction>(Op0)->getFastMathFlags();

test/Transforms/InstCombine/fdiv.ll

;
   %div = fdiv <2 x float> %x, <float 4.0, float 3.0>
   ret <2 x float> %div
 }
 
 ; (X / Y) / Z --> X / (Y * Z)
 
 define float @div_with_div_numerator(float %x, float %y, float %z) {
 ; CHECK-LABEL: @div_with_div_numerator(
-; CHECK-NEXT:    [[TMP1:%.*]] = fmul ninf float [[Y:%.*]], [[Z:%.*]]
-; CHECK-NEXT:    [[DIV2:%.*]] = fdiv fast float [[X:%.*]], [[TMP1]]
+; CHECK-NEXT:    [[TMP1:%.*]] = fmul float [[Y:%.*]], [[Z:%.*]]
+; CHECK-NEXT:    [[DIV2:%.*]] = fdiv reassoc arcp float [[X:%.*]], [[TMP1]]
 ; CHECK-NEXT:    ret float [[DIV2]]
 ;
   %div1 = fdiv ninf float %x, %y
-  %div2 = fdiv fast float %div1, %z
+  %div2 = fdiv arcp reassoc float %div1, %z
   ret float %div2
 }
 
 ; Z / (X / Y) --> (Z * Y) / X
 
 define <2 x float> @div_with_div_denominator(<2 x float> %x, <2 x float> %y, <2 x float> %z) {
 ; CHECK-LABEL: @div_with_div_denominator(
-; CHECK-NEXT:    [[TMP1:%.*]] = fmul nnan <2 x float> [[Y:%.*]], [[Z:%.*]]
-; CHECK-NEXT:    [[DIV2:%.*]] = fdiv fast <2 x float> [[TMP1]], [[X:%.*]]
+; CHECK-NEXT:    [[TMP1:%.*]] = fmul <2 x float> [[Y:%.*]], [[Z:%.*]]
+; CHECK-NEXT:    [[DIV2:%.*]] = fdiv reassoc arcp <2 x float> [[TMP1]], [[X:%.*]]
 ; CHECK-NEXT:    ret <2 x float> [[DIV2]]
 ;
   %div1 = fdiv nnan <2 x float> %x, %y
-  %div2 = fdiv fast <2 x float> %z, %div1
+  %div2 = fdiv arcp reassoc <2 x float> %z, %div1
   ret <2 x float> %div2
 }
 
 ; Don't create an extra multiply if we can't eliminate the first div.
 
 declare void @use_f32(float)
 
 define float @div_with_div_numerator_extra_use(float %x, float %y, float %z) {