Optimize square root squared (PR21126)

[spatel]

Summary

When unsafe-fp-math is enabled, we can turn sqrt(X) * sqrt(X) into X.

This can happen in the real world when calculating x ** 3/2. This occurs in test-suite/SingleSource/Benchmarks/BenchmarkGame/n-body.c.

[hfinkel]

LGTM.

test/Transforms/InstCombine/fmul.ll

130 ↗

(On Diff #14338)

We don't need the "unsafe-fp-math"="true" because you're just checking the 'fast' on the fmul, right? If you don't need it, please remove it.

[spatel]

test/Transforms/InstCombine/fmul.ll

130 ↗

(On Diff #14338)

Thanks, Hal! Yes - the 'fast' alone is enough to do this optimization. I'm not a fan of the instruction-level fast-math flags...because it leads to another question. What should happen in the 2nd test case if it looks like this:

define double @sqrt_squared2(double %f) #0 {

%sqrt = call double @llvm.sqrt.f64(double %f)
%mul1 = fmul fast double %sqrt, %sqrt
%mul2 = fmul double %mul1, %sqrt
ret double %mul2

}

When the %mul1 operand is replaced in the 2nd fmul instruction, does that fmul instruction now become fast too? Is fast infectious?

[hfinkel]

test/Transforms/InstCombine/fmul.ll

130 ↗

(On Diff #14338)

I think that you should change your mind ;)

The motivation behind the fast-math flags, in addition to providing client more-precise control over what instructions might be folded, was to create a "fast math" system that could survive inlining. As a result, the answer to your last question is no, it is not infectious, it is designed so that inlined "fast math" functions can have their instructions retain that property without affecting operations from the caller (which might not be "fast math" -- a particular problem when you consider LTO).

[spatel]

test/Transforms/InstCombine/fmul.ll

130 ↗

(On Diff #14338)

I'd love to love this. :)

..but, you see what happens in this example:

1. There's no function-level attribute for unsafe-math.

2. When we kill the first fmul, we eliminate the only trace of 'fast'-ness in this function (ignore for the moment that the llvm.sqrt intrinsic isn't IEEE-754 compliant).

3. After the optimization, we have a seemingly safe sqrt and a safe fmul. But the entirety of the code that will be produced for this function is decidedly unsafe. We just deleted the knowledge that the original function was not safe.

So in fact, fast is infectious, but silently so! LTO users should be aware that linking any unsafe code produces an overall unsafe program?

[hfinkel]

test/Transforms/InstCombine/fmul.ll

130 ↗

(On Diff #14338)

Sure, but that's normally what the user wants! They want fast semantics for the routines in one module, and precise semantics for routines in another, then they want LTO to inline one into the other.

The fast flags are not meant to preserve a record of non-strict semantics -- that's not the optimizers job ;) -- but rather are intended to allow optimization control.

[spatel]

test/Transforms/InstCombine/fmul.ll

130 ↗

(On Diff #14338)

Ok, sold. :)

I was just pretending to be an FP wonk. If the real FP wonks complain, please protect this optimization!

I'll fix up the testcase and get this checked in.

[spatel]

Closed by commit rL218906 (authored by @spatel).

llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp

if (AllowReassociate && isFiniteNonZeroFp(C)) {
             RI->copyFastMathFlags(&I);
             return RI;
           }
         }
       }
     }
   }
 
+  // sqrt(X) * sqrt(X) -> X
+  if (AllowReassociate && (Op0 == Op1))
+    if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Op0))
+      if (II->getIntrinsicID() == Intrinsic::sqrt)
+        return ReplaceInstUsesWith(I, II->getOperand(0));
 
   // Under unsafe algebra do:
   // X * log2(0.5*Y) = X*log2(Y) - X
   if (AllowReassociate) {
     Value *OpX = nullptr;
     Value *OpY = nullptr;
     IntrinsicInst *Log2;
     detectLog2OfHalf(Op0, OpY, Log2);

llvm/trunk/test/Transforms/InstCombine/fmul.ll

define float @test11(float %x, float %y) {
   %a = fadd fast float %x, 1.0
   %b = fadd fast float %y, 2.0
   %c = fadd fast float %a, %b
   ret float %c
 ; CHECK-LABEL: @test11(
 ; CHECK-NOT: fadd float
 ; CHECK: fadd fast float
 }
+
+; PR21126: http://llvm.org/bugs/show_bug.cgi?id=21126
+; With unsafe/fast math, sqrt(X) * sqrt(X) is just X.
+declare double @llvm.sqrt.f64(double)
+
+define double @sqrt_squared1(double %f) #0 {
+  %sqrt = call double @llvm.sqrt.f64(double %f)
+  %mul = fmul fast double %sqrt, %sqrt
+  ret double %mul
+; CHECK-LABEL: @sqrt_squared1(
+; CHECK-NEXT: ret double %f
+}
+
+; With unsafe/fast math, sqrt(X) * sqrt(X) is just X, 
+; but make sure another use of the sqrt is intact.
+; Note that the remaining fmul is altered but is not 'fast'
+; itself because it was not marked 'fast' originally. 
+; Thus, we have an overall fast result, but no more indication of
+; 'fast'ness in the code.
+define double @sqrt_squared2(double %f) #0 {
+  %sqrt = call double @llvm.sqrt.f64(double %f)
+  %mul1 = fmul fast double %sqrt, %sqrt
+  %mul2 = fmul double %mul1, %sqrt
+  ret double %mul2
+; CHECK-LABEL: @sqrt_squared2(
+; CHECK-NEXT: %sqrt = call double @llvm.sqrt.f64(double %f)
+; CHECK-NEXT: %mul2 = fmul double %sqrt, %f
+; CHECK-NEXT: ret double %mul2
+}