Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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? |
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1263–1265 ↗ | (On Diff #158436) | Will do. |
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. |
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. |
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? |
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. |
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? |
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. |
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. |
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.
Thank you.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp | ||
---|---|---|
1171–1172 ↗ | (On Diff #158776) | Which is virtually the same test. I'm confused... |
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. :) |
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(). |