Page MenuHomePhabricator

[InstCombine] Simplify pow(2.0, itofp(y)) to ldexp(1.0, y)
ClosedPublic

Authored by evandro on Thu, Aug 8, 3:09 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

evandro created this revision.Thu, Aug 8, 3:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Aug 8, 3:09 PM

Does this require fast math due to rounding in the sitofp conversion? Off the top of my head, probably it doesn't matter because those cases would produce zero or infinity anyway, but it's worth explaining in the code.

xbolva00 added a comment.EditedThu, Aug 8, 3:20 PM

I think there is a general case... pow(powOf2, itofp(y)) to ldexp(1.0, log2(powOf2) * y).

My fast patch from yday.. https://pastebin.com/ETZpNby5 line 1350.

xbolva00 added inline comments.Thu, Aug 8, 3:22 PM
llvm/test/Transforms/InstCombine/pow_fp_int.ll
292 ↗(On Diff #214244)

Avoid this change.

xbolva00 added inline comments.Thu, Aug 8, 3:25 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1332 ↗(On Diff #214244)

There is one "emit ldexp" old code in SLC which could use emitBinaryFloatFnCall.

evandro marked an inline comment as done.Fri, Aug 9, 9:47 AM

Since exp2(itofp(x) is similarly simplified into ldexp(1.0, x) without accounting for any relaxation, methinks that this simplification should be fine too. Will note this.

evandro marked an inline comment as done.Fri, Aug 9, 9:50 AM
evandro added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1332 ↗(On Diff #214244)

Indeed, there are other places that could use emitBinaryFloatFnCall().

evandro updated this revision to Diff 214395.Fri, Aug 9, 9:51 AM

Yes, we can turn exp2 to ldexp as follow up patch.

xbolva00 retitled this revision from [InstCombine] Simplify pow() into ldexp() to [InstCombine] Simplify pow(2.0, itofp(y)) to ldexp(1.0, y).Fri, Aug 9, 10:00 AM
xbolva00 added inline comments.Fri, Aug 9, 11:03 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1327 ↗(On Diff #214395)

Can we extract this “fits in int” code to helper function? optimize exp2 checks it too, and that fold could use it too... and my powi fold too

(Optional)

evandro updated this revision to Diff 214412.Fri, Aug 9, 11:38 AM
evandro marked an inline comment as done.
xbolva00 accepted this revision.Fri, Aug 9, 11:41 AM

Nice!

(But please wait for @efriedma or @spatel too)

This revision is now accepted and ready to land.Fri, Aug 9, 11:41 AM

Week passed, it was enough time to comment it.

You can land it.

This revision was automatically updated to reflect the committed changes.