This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] pow(x, +/- 0.0) -> 1.0
ClosedPublic

Authored by jfb on Sep 5 2019, 5:26 PM.

Details

Summary

This isn't an important optimization at all... We're already doing:

pow(x, 0.0) -> 1.0

My patch merely teaches instcombine that -0.0 does the same.

However, doing this fixes an AMAZING bug! Compile this program:

extern "C" double pow(double, double);
double boom(double base) {
  return pow(base, -0.0);
}

With:

clang++ ~/Desktop/fast-math.cpp -ffast-math -O2 -S

And clang will crash with a signal. Wow, fast math is so fast it ICEs the
compiler! Arguably, the generated math is infinitely fast.

What's actually happening is that we recurse infinitely in getPow. In debug we
hit its assertion:

assert(Exp != 0 && "Incorrect exponent 0 not handled");

We avoid this entire mess if we instead recognize that an exponent of positive
and negative zero yield 1.0.

rdar://problem/54598300

Diff Detail

Repository
rL LLVM

Event Timeline

jfb created this revision.Sep 5 2019, 5:26 PM
arsenm added a subscriber: arsenm.Sep 5 2019, 5:32 PM
arsenm added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1566–1567 ↗(On Diff #219010)

You can use m_AnyZeroFP

1566–1567 ↗(On Diff #219010)

Also gives free vector support

llvm/test/Transforms/InstCombine/pow-0.ll
30 ↗(On Diff #219010)

Can you add some vectors?

jfb updated this revision to Diff 219012.Sep 5 2019, 5:39 PM
  • Address arsenm's comments
jfb marked 3 inline comments as done.Sep 5 2019, 5:39 PM
arsenm accepted this revision.Sep 5 2019, 5:50 PM

LGTM

This revision is now accepted and ready to land.Sep 5 2019, 5:50 PM
scanon accepted this revision.Sep 5 2019, 5:51 PM

"pow(x, ±0) returns 1 for any x, even a NaN"

LGTM.

xbolva00 accepted this revision.Sep 6 2019, 2:31 AM
xbolva00 added a subscriber: xbolva00.

lg too

This revision was automatically updated to reflect the committed changes.
spatel added a comment.Sep 6 2019, 9:26 AM

Sorry - I didn't see this review, only the bug report that came in today:
https://bugs.llvm.org/show_bug.cgi?id=43233

I made an identical code change, but I didn't include as many tests. Feel free to commit those or let me know if you want me to.

jfb added a comment.Sep 6 2019, 9:31 AM

Sorry - I didn't see this review, only the bug report that came in today:
https://bugs.llvm.org/show_bug.cgi?id=43233

I made an identical code change, but I didn't include as many tests. Feel free to commit those or let me know if you want me to.

Already committed the tests :)