Page MenuHomePhabricator

[InstCombine] Expand the simplification of log()
ClosedPublic

Authored by evandro on Sep 4 2019, 3:51 PM.

Details

Summary

Expand the simplification of special cases of log() to include log2() and log10() as well as intrinsics and more types.

Diff Detail

Repository
rL LLVM

Event Timeline

evandro created this revision.Sep 4 2019, 3:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2019, 3:51 PM

Patch uploaded without context.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1710 ↗(On Diff #218803)

Does this do the right thing for non-double libcalls?

2888 ↗(On Diff #218803)

not Intrinsic::log10?

llvm/test/Transforms/InstCombine/log-pow.ll
93 ↗(On Diff #218803)

I'm not sure why you want to remove test coverage for log10f?

evandro marked 3 inline comments as done.Sep 4 2019, 4:22 PM
evandro added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1710 ↗(On Diff #218803)

Apparently so.

2888 ↗(On Diff #218803)

We don't have one yet.

llvm/test/Transforms/InstCombine/log-pow.ll
93 ↗(On Diff #218803)

log10g() is actually tested elsewhere and this results in a more radical simplification.

evandro updated this revision to Diff 218806.Sep 4 2019, 4:23 PM
efriedma added inline comments.Sep 4 2019, 4:44 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2888 ↗(On Diff #218803)

LangRef says we have one; is it wrong?

llvm/test/Transforms/InstCombine/log-pow.ll
93 ↗(On Diff #218803)

Okay.

evandro marked 6 inline comments as done.Sep 4 2019, 4:52 PM
evandro added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2888 ↗(On Diff #218803)

No, but perhaps I am.

evandro updated this revision to Diff 218813.Sep 4 2019, 5:01 PM
evandro marked an inline comment as done.
evandro updated this revision to Diff 218815.Sep 4 2019, 5:06 PM
xbolva00 added inline comments.Sep 6 2019, 2:11 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1807 ↗(On Diff #218815)

Can you extract this code and comment to helper function?

Can be used also a few lines below.

evandro marked 2 inline comments as done.Sep 6 2019, 2:24 PM
evandro added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1807 ↗(On Diff #218815)

Can do, using the existing LibCallSimplifier::replaceAllUsesWith() for this purpose.

evandro updated this revision to Diff 219186.Sep 6 2019, 3:11 PM
evandro marked an inline comment as done.
evandro updated this revision to Diff 219424.Sep 9 2019, 1:27 PM

Please also clang format the parts you modified, otherwise this looks ok.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1807 ↗(On Diff #218815)

Thanks

xbolva00 added inline comments.Sep 15 2019, 10:14 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1788 ↗(On Diff #219424)

ArgFnName?

1790 ↗(On Diff #219424)

ArgLibFn?

Same above. We should use something else than "Lb" I think.

evandro marked an inline comment as done.Sep 16 2019, 10:57 AM
evandro marked an inline comment as done.Sep 16 2019, 11:00 AM
evandro added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1788 ↗(On Diff #219424)

Just using the same scheme as for the Log CallInst...

evandro updated this revision to Diff 220364.Sep 16 2019, 11:18 AM

Ok.

Any futher comments by @efriedma ?

hiraditya accepted this revision.Sep 29 2019, 10:02 AM
This revision is now accepted and ready to land.Sep 29 2019, 10:02 AM
evandro marked 2 inline comments as done.Sep 30 2019, 1:16 PM
This revision was automatically updated to reflect the committed changes.
spatel added a comment.Oct 9 2019, 4:11 AM

This patch is blamed for a compiler crash in PR43617:
https://bugs.llvm.org/show_bug.cgi?id=43617

efriedma added inline comments.Oct 9 2019, 12:11 PM
llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp
1923

This should be using the overload of getLibFunc that takes a CallSite, instead of expanding it out by hand. This formulation skips checks that should happen otherwise (specifically, that it's not an indirect call, that the call isn't marked nobuiltin, and the function has an appropriate signature).

evandro marked an inline comment as done.Oct 9 2019, 1:29 PM
evandro added a subscriber: craig.topper.
evandro added inline comments.
llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp
1923

True, but, as @craig.topper said, the cast at line 1919 above might be returning nullptr.

craig.topper added inline comments.Oct 9 2019, 1:38 PM
llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp
1923

The CallSite version specifically checks that getCalledFunction doesn't return null. Because the case where it returns null is for an indirect call.

For the record, the issue reported in PR43617 was fixed by rL374243 and a test case was added by rL374453.