This is an archive of the discontinued LLVM Phabricator instance.

[SLC] Fix shrinking of pow()
ClosedPublic

Authored by evandro on Jul 31 2018, 3:21 PM.

Details

Summary

Properly shrink pow() to powf() as a binary function and, when no other simplification applies, do not discard it.

Diff Detail

Event Timeline

evandro created this revision.Jul 31 2018, 3:21 PM
efriedma added inline comments.Jul 31 2018, 3:45 PM
llvm/test/Transforms/InstCombine/double-float-shrink-1.ll
363

It looks like optimizeBinaryDoubleFP doesn't do all the same checks optimizeUnaryDoubleFP does? optimizeUnaryDoubleFP intentionally chooses not to optimize operations where the result isn't truncated because the extra bits might actually be useful.

evandro added inline comments.Jul 31 2018, 4:00 PM
llvm/test/Transforms/InstCombine/double-float-shrink-1.ll
363

Arguably, this should not shrink.

evandro added inline comments.Jul 31 2018, 4:07 PM
llvm/test/Transforms/InstCombine/double-float-shrink-1.ll
363

Besides here, optimizeBinaryDoubleFP() is also used for f{min,max}() and copysign(). Neither of the latter functions need the extra bits in the result.

spatel added inline comments.Jul 31 2018, 4:07 PM
llvm/test/Transforms/InstCombine/double-float-shrink-1.ll
363

An argument in favor of shrinking: it may reduce the number of instructions as shown in this test.

evandro added inline comments.Jul 31 2018, 4:14 PM
llvm/test/Transforms/InstCombine/double-float-shrink-1.ll
363

And the series in powf() is probably shorter and faster than in pow().

evandro updated this revision to Diff 158435.Jul 31 2018, 6:07 PM
evandro edited the summary of this revision. (Show Details)
spatel added a comment.Aug 1 2018, 9:18 AM

I think we should commit this based on fixing the miscompile alone; we can debate the other diff in a follow-on patch if needed.
But let's see if @efriedma agrees.

I think we should commit this based on fixing the miscompile alone; we can debate the other diff in a follow-on patch if needed.

The miscompile was never noticed because the shrinking was always discarded. It seems sensible to me that both are fixed in one swoop.

evandro added inline comments.Aug 2 2018, 9:10 AM
llvm/test/Transforms/InstCombine/double-float-shrink-1.ll
363

On the other hand, this shrink is only performed when -enable-double-float-shrink is specified or with FMF, which implies the attribute afn.

I'd like to get this right... both the CheckRetType check, and the "infinite loop" check from optimizeUnaryDoubleFP are probably relevant. Can we share code between the two functions?

Maybe the CheckRetType check isn't critical if we're only doing this transform by default when afn is enabled, but it's still losing a lot of bits, particularly if the input is supposed to be an exact number. pow(2.f, 0.5f) is a lot different from (double)powf(2.f, 0.5f).

(If you want to fix the potential miscompile for 7.0, I'd accept a patch to just completely disable shrinking for pow.)

I'd like to get this right... both the CheckRetType check, and the "infinite loop" check from optimizeUnaryDoubleFP are probably relevant. Can we share code between the two functions?

Maybe the CheckRetType check isn't critical if we're only doing this transform by default when afn is enabled, but it's still losing a lot of bits, particularly if the input is supposed to be an exact number. pow(2.f, 0.5f) is a lot different from (double)powf(2.f, 0.5f).

(If you want to fix the potential miscompile for 7.0, I'd accept a patch to just completely disable shrinking for pow.)

Just to make it clear, CheckRetType only matters for optimizeUnaryDoubleFP(); optimizeBinaryDoubleFP() doesn't have.

I agree with you, but wonder how this could ever work. It didn't, so probably nobody ever used it. So, why was it added? If it has no defensible purpose, then, by all means, it should be dropped.

So, why was it added?

I would assume someone added it alongside a bunch of other operations where it matters more, and didn't write enough tests.

In general, the point of the transform is to catch code where someone accidentally writes "float z = pow(x, y);" instead of "float z = powf(x, y);" (And sometimes, it isn't practical to fix the code.)

Alternatively, optimizeBinaryDoubleFP() could be fixed to shrink just float f = pow(), while keeping its other users, f{min,max}() and copysign() happy, which should be easy to satisfy, given how simple these functions are.

Refactored the helper functions for shrinking unary and binary functions into a single one, while keeping all their functionality, in rL338905.

evandro updated this revision to Diff 159055.Aug 3 2018, 11:04 AM

Keep the double precision pow() when the result is too.

efriedma accepted this revision.Aug 3 2018, 2:48 PM

LGTM

This revision is now accepted and ready to land.Aug 3 2018, 2:48 PM
This revision was automatically updated to reflect the committed changes.