[PATCH] D50035: [SLC] Expand simplification of pow() for vector types

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1171-1172
+  // Bail out if simplifying libcalls to pow() is disabled.
+  if (!hasUnaryFloatFn(TLI, Ty, LibFunc_pow, LibFunc_powf, LibFunc_powl))
+    return nullptr;
+
----------------
evandro wrote:
> spatel wrote:
> > evandro wrote:
> > > spatel wrote:
> > > > evandro wrote:
> > > > > spatel wrote:
> > > > > > evandro wrote:
> > > > > > > spatel wrote:
> > > > > > > > Why do we need this check? In all of the affected transforms, we're not transforming *to* pow(). Is there a known bug without this check? Given that we have a pow intrinsic, I don't see the problem.
> > > > > > > Without this check, `pow()` would be simplified even with `-disable-simplify-libcalls` and `pow-3.ll` would fail.
> > > > > > No, that test still passes (we don't convert) because we check for the availability of sqrt() before trying the transform.
> > > > > > 
> > > > > > I suppose this raises a question: if -disable-simplify-libcalls is specified, should that preclude simplifications from LLVM intrinsics that mimic libcalls to other LLVM intrinsics that mimic libcalls?
> > > > > > ...but that's probably best left to another patch or asked on llvm-dev.
> > > > > `-disable-simplify-libcalls` disables all functions, including `pow()` itself.  As I understand it, no optimization should then be performed on libcalls.  Again, without these lines, `pow-3.ll` fails.
> > > > There must be something different between our local machines then. I'm applying this patch on an unmodified trunk r339434, deleting the check for pow(), and running 'make check'. Everything passes. 
> > > > 
> > > > And I even stepped through in the debugger to confirm the reason as I stated earlier - the sqrt() check fails, so there is no transform. Can you step through and see how that check is bypassed on your machine?
> > > You're right.  We're looking at different stages of the tree.  
> > > 
> > > Regardless, without this check, `pow()` could still be shrunk below, which I don't think is desired with `-disable-simplify-libcalls`.
> > No, the shrinking is guarded by TLI->getName(LibFunc_pow) and so that won't happen independent of this patch. I added tests for this at rL339467.
> Which is virtually the same test.  I'm confused...
Confused by the tests? The 1st test is exactly what you proposed to add here. The 2nd test is identical except it proves the same behavior for the intrinsic rather than the libcall. The checks are auto-generated and show that no shrinking is happening even without this patch.

Confused by the fact that your local results don't match trunk? I can't help there. :)
What is the next patch in this sequence to review? Let's make sure it's rebased after this gets committed, so we're looking at the same code/tests.


https://reviews.llvm.org/D50035