Page MenuHomePhabricator

[APFloat] Extend converting from special strings
ClosedPublic

Authored by ekatz on Nov 3 2019, 12:52 PM.

Details

Summary

Add support for converting Signaling NaN, and a NaN Payload from string.

The NaNs (the string "nan" or "NaN") may be prefixed with 's' or 'S' for defining a Signaling NaN.
A payload for a NaN can be specified as a suffix. It may be a octal/decimal/hexadecimal number in parentheses or without.

Diff Detail

Event Timeline

ekatz created this revision.Nov 3 2019, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2019, 12:52 PM
ekatz added a reviewer: scanon.Nov 3 2019, 8:26 PM
ekatz added a reviewer: arsenm.

Please add a test case.

ekatz updated this revision to Diff 228193.Nov 7 2019, 2:08 AM

Added unit test.

Are you trying to match some specific grammar here?

We use string->float conversion in a bunch of places, including places where the grammar is specified by some external standard; I'm a little concerned we could be accidentally extending the user-visible grammar in some context. (Maybe those places account for this issue already? Not sure.)

ekatz added a comment.EditedNov 19 2019, 10:45 PM

Are you trying to match some specific grammar here?

We use string->float conversion in a bunch of places, including places where the grammar is specified by some external standard; I'm a little concerned we could be accidentally extending the user-visible grammar in some context. (Maybe those places account for this issue already? Not sure.)

There is no unified syntax for those "specials"; but these are pretty acceptable representations that I added.
Some are pretty straight forward, like the case of NaN: we had "nan" and "NaN", in many places (in the web) you may see references to it as "NAN", so why not support it as well?
Others are extensions, so we may express "Signaling NaN" or explicit "Quiet NaN". And of course, there is the payload. Why not support it?

I think it is a welcoming extension to LLVM, while Clang (and other frontends) doesn't (and shouldn't) rely on the formats allowed by APFloat, but by its own restrictions.

  • Please, also note that SNaN with PAYLOAD is needed for the tests in D69774.
hfinkel added inline comments.Dec 13 2019, 5:30 PM
llvm/lib/Support/APFloat.cpp
2702

My impression is that this logic involving NDiff is here to make sure that the case matches, correct? This makes the code hard to follow. Frankly, if this is just a feature for us (to make it easier to write FP numbers in textual LLVM IR), why bother checking for the case matching at all? Just do a case-insensitive match. Otherwise, you can also match and then just check that: isupper(str[0]) == isupper(str[2]), no?

ekatz marked an inline comment as done.Jan 7 2020, 11:17 AM
ekatz added inline comments.
llvm/lib/Support/APFloat.cpp
2702

I think that the key of your comment is that:

...the code hard to follow.

I agree, and I decided to implement a simpler version (maybe not as efficient, but it is negligible anyway).

ekatz updated this revision to Diff 236659.Jan 7 2020, 12:07 PM
ekatz edited the summary of this revision. (Show Details)

Reduced the patch to only the extensions of:

  • Signaling NaN
  • NaN Pyaload

LGTM with nits

llvm/lib/Support/APFloat.cpp
2647

For only this, I think == 's' or =='S' would be clearer

2666

demorgan this

2673

Initialize to 10 and remove the else case?

ekatz updated this revision to Diff 239339.Jan 21 2020, 8:58 AM

Done requested nit changes.

arsenm accepted this revision.Jan 21 2020, 9:01 AM
This revision is now accepted and ready to land.Jan 21 2020, 9:01 AM
This revision was automatically updated to reflect the committed changes.