Page MenuHomePhabricator

[ValueTracking] Add comment that CannotBeOrderedLessThanZero does the wrong thing for powi.
ClosedPublic

Authored by jlebar on Jan 19 2017, 4:48 PM.

Details

Summary

CannotBeOrderedLessThanZero(powi(x, exp)) returns true if
CannotBeOrderedLessThanZero(x). But powi(-0, exp) is negative if exp is
odd, so we actually want to return SignBitMustBeZero(x).

Except that also isn't right, because we want to return true if x is
NaN, even if x has a negative sign bit.

What we really need in order to fix this is a consistent approach in
this function to handling the sign bit of NaNs. Without this it's very
difficult to say what the correct behavior here is.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar created this revision.Jan 19 2017, 4:48 PM
efriedma edited edge metadata.Jan 19 2017, 5:17 PM

https://reviews.llvm.org/D28926 provides perfectly clear definitions of what those functions are supposed to return; if something is wrong, just fix it (or file a bug). From your description, it sounds like we need to split "SignBitOnly" into "NegZeroSignBitSignificant" and "NanSignBitSignificant"?

Oh, wait, I see, I really messed up my review of SignBitMustBeZero()... we return the wrong result for a ton of stuff. I'd still prefer a bug report with a testcase over a TODO comment.

we return the wrong result for a ton of stuff.

Yes. :) Again it's not clear to me what it's really supposed to do.

I'd still prefer a bug report with a testcase over a TODO comment.

How about if you file a bug I'll edit the code to add a reference to it? I'd like to have a breadcrumb in the code indicating that this is wrong so that the next person who comes along and notices the breakage doesn't waste days on it like I did.

jlebar updated this revision to Diff 85098.Jan 19 2017, 9:54 PM

Update comments. I made them shorter and added links to the bug.

I think this is an appropriate amount of brevity while still giving a reader
have some clue what's going on, but lmk what you think.

hfinkel edited edge metadata.Jan 25 2017, 4:54 PM

Update comments. I made them shorter and added links to the bug.

I think this is an appropriate amount of brevity while still giving a reader
have some clue what's going on, but lmk what you think.

Looks reasonable to me. @efriedma ?

efriedma accepted this revision.Jan 25 2017, 5:05 PM

LGTM.

This revision is now accepted and ready to land.Jan 25 2017, 5:05 PM
This revision was automatically updated to reflect the committed changes.