Page MenuHomePhabricator

[InstCombine] Expand the simplification of log()
Needs ReviewPublic

Authored by evandro on Wed, Sep 4, 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

Event Timeline

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

Patch uploaded without context.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1710

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

2888

not Intrinsic::log10?

llvm/test/Transforms/InstCombine/log-pow.ll
93

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

evandro marked 3 inline comments as done.Wed, Sep 4, 4:22 PM
evandro added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1710

Apparently so.

2888

We don't have one yet.

llvm/test/Transforms/InstCombine/log-pow.ll
93

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

evandro updated this revision to Diff 218806.Wed, Sep 4, 4:23 PM
efriedma added inline comments.Wed, Sep 4, 4:44 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2888

LangRef says we have one; is it wrong?

llvm/test/Transforms/InstCombine/log-pow.ll
93

Okay.

evandro marked 6 inline comments as done.Wed, Sep 4, 4:52 PM
evandro added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
2888

No, but perhaps I am.

evandro updated this revision to Diff 218813.Wed, Sep 4, 5:01 PM
evandro marked an inline comment as done.
evandro updated this revision to Diff 218815.Wed, Sep 4, 5:06 PM
xbolva00 added inline comments.Fri, Sep 6, 2:11 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1802

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.Fri, Sep 6, 2:24 PM
evandro added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1802

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

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