This is an archive of the discontinued LLVM Phabricator instance.

[SLC] Expand simplification of pow() for vector types
ClosedPublic

Authored by evandro on Jul 30 2018, 6:55 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

evandro created this revision.Jul 30 2018, 6:55 PM

This patch is still making multiple functional changes, so it needs to be split up.

IIUC, the changes to the match() statements are all functional diffs (maybe only using the llvm intrinsics with vector types?), but there are no tests for those?

I'm still distracted by the NFC whitespace, formatting, and comment diffs. Please make those before/after the functional change.

I updated the test file and added the baseline tests from this rev at rL338392. Use the script to auto-generate new CHECK lines.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1181 ↗(On Diff #158157)

This change should be its own patch. It fixes a miscompile - we were wrongly shrinking pow(x, y) as a unary function.

evandro marked an inline comment as done.Jul 31 2018, 3:25 PM
evandro updated this revision to Diff 158436.Jul 31 2018, 6:09 PM
evandro retitled this revision from [SLC] Fix precision shrinking for pow() to [SLC] Expand simplification of pow() for vector types.
evandro edited the summary of this revision. (Show Details)
spatel added inline comments.Aug 1 2018, 3:12 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1186 ↗(On Diff #158436)

IIUC, this is NFC, so you could just commit that before the other changes

1263–1265 ↗(On Diff #158436)

Is there a test for this transform?

evandro marked an inline comment as done.Aug 2 2018, 8:35 AM
evandro added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1263–1265 ↗(On Diff #158436)

Will do.

evandro updated this revision to Diff 158776.Aug 2 2018, 9:21 AM

Added test cases for vector types in pow(x, n).

evandro marked 2 inline comments as done.Aug 2 2018, 9:21 AM
spatel added inline comments.Aug 9 2018, 1:10 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1171–1172 ↗(On Diff #158776)

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.

evandro added inline comments.Aug 9 2018, 1:42 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1171–1172 ↗(On Diff #158776)

Without this check, pow() would be simplified even with -disable-simplify-libcalls and pow-3.ll would fail.

spatel added inline comments.Aug 9 2018, 3:39 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1171–1172 ↗(On Diff #158776)

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.

evandro added inline comments.Aug 10 2018, 8:47 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1171–1172 ↗(On Diff #158776)

-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.

spatel added inline comments.Aug 10 2018, 9:03 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1171–1172 ↗(On Diff #158776)

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?

evandro updated this revision to Diff 160153.Aug 10 2018, 11:17 AM

Added test for shrinking pow() when simplifying libcalls is disabled.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1171–1172 ↗(On Diff #158776)

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.

spatel added inline comments.Aug 10 2018, 1:14 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1171–1172 ↗(On Diff #158776)

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.

spatel accepted this revision.Aug 10 2018, 1:17 PM

I suppose the extra check doesn't hurt anything, and this is all disorganized anyway. The simplifications of calls to constants should really be in InstSimplify.
The vector changes look correct, so LGTM.

This revision is now accepted and ready to land.Aug 10 2018, 1:17 PM

Thank you.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1171–1172 ↗(On Diff #158776)

Which is virtually the same test. I'm confused...

spatel added inline comments.Aug 10 2018, 2:13 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1171–1172 ↗(On Diff #158776)

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.

The dependent patches can be found in the Stack below: D49273 and D50036.

Thank you.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1171–1172 ↗(On Diff #158776)

Sorry. I meant that TLI->getName(LibFunc_pow) is virtually the same inside hasUnaryFloatFn().

This revision was automatically updated to reflect the committed changes.
evandro marked 3 inline comments as done.
evandro added a subscriber: fhahn.Aug 17 2018, 8:01 AM