[PATCH] fold: sqrt(x * x * y) -> fabs(x) * sqrt(y)

Sanjay Patel spatel at rotateright.com
Wed Oct 15 10:25:47 PDT 2014


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1270
@@ +1269,3 @@
+  if (Instruction *I = dyn_cast<Instruction>(Op)) {
+    if (I->getOpcode() == Instruction::FMul && I->hasUnsafeAlgebra()) {
+      // We're looking for a repeated factor in a multiplication tree,
----------------
hfinkel wrote:
> spatel wrote:
> > hfinkel wrote:
> > > Hrmm... is this right, or do we need to check the function attribute here? I'm not sure that the argument having the unsafe-algebra flag means that we can change its use in a non-strict way.
> > I think this goes back to the discussion of whether 'fast' is infectious in http://reviews.llvm.org/D5584. :)
> > In that case, we optimized away a sqrt call even though it wasn't explicitly marked 'fast'. This is a similar transform. We've just reversed the order of the fmul and sqrt.
> > 
> > I thought I saw some other precedence for just using the instruction-level flags, but I'm not finding it now. There's an optimization for log2() in InstCombineMulDivRem that checks whether the log2 intrinsic hasUnsafeAlgebra...I don't know how that is even specified in IR. I thought the IR fast-math flags only apply to fmul, fdiv, fadd, fsub, and frem? At least, that's what the LangRef says. FWIW, that log2 optimization doesn't appear to ever trigger, and I don't see any test case for it (r169025).
> > In that case, we optimized away a sqrt call even though it wasn't explicitly marked 'fast'. This is a similar transform. We've just reversed the order of the fmul and sqrt.
> 
> Right, but in that case we were optimizing the fmul, and it was the "outer" operation, and it had fast. In this case, the sqrt is the outer operation, and we need to check its equivalent.
> 
> Generally speaking, fast-math flags cannot infect users (only operands), except for some result assumptions, because of the inlining use case.
> 
> > FWIW, that log2 optimization doesn't appear to ever trigger, and I don't see any test case for it (r169025).
> 
> Seems like this is a bug (I would not object to enhancing the IR to support fast-math flags on intrinsics, but as it stands, it is a bug).
> 
Ah - ok, that sounds reasonable. Is the outer/inner distinction specified somewhere? In the LangRef?

And yes, I think if we're committed to instruction-level IR fast-math-flags, then intrinsics deserve those decorations too.

How about this for now: I'll redo this patch and testcases to check for a function-level attribute, add a TODO comment that we should revisit it if and when intrinsics get FMF support, and open a bug to add FMF support to intrinsics.

We had also run into the issue of FMF on intrinsics in http://reviews.llvm.org/D5222. We weren't sure if we could just treat the function-level attribute as a convenience, but I don't think we can do that in the following case: inlining/LTO where strict code gets inlined into fast code. The strict code is the "inner" logic in that case...so even though it wouldn't have IR-level FMF, it should bend to the "outer" function's attributes and IR-level FMF?

http://reviews.llvm.org/D5787






More information about the llvm-commits mailing list