This is an archive of the discontinued LLVM Phabricator instance.

APFloat: allow 64-bit of payload
ClosedPublic

Authored by jfb on Dec 7 2018, 3:54 PM.

Details

Summary

The APFloat and Constant APIs taking an APInt allow arbitrary payloads, and that's great. There's a convenience API which takes an unsigned, and that's silly because it then directly creates a 64-bit APInt. Just change it to 64-bits directly.

Diff Detail

Repository
rL LLVM

Event Timeline

jfb created this revision.Dec 7 2018, 3:54 PM
jfb added a comment.Dec 7 2018, 3:56 PM

Testing-wise, this isn't tested in APFloatTest.cpp, so I'm keeping the status-quo.
The only place where it's used in an odd way is for the floating-literals.s test (which tries to match the GNU as behavior). The test makes me uneasy because it's using ~0 to create a .single NaN... and I have no idea what's the right behavior for .double... This is better fixed in a follow-up.

This truncates the payload to fit into the significand field. Presumably that includes setting the top bit of the significand to make this a NaN and not an infinity. Does it also always form a qNaN, or can you create an sNaN this way?

jfb added a comment.Dec 7 2018, 4:01 PM

This truncates the payload to fit into the significand field. Presumably that includes setting the top bit of the significand to make this a NaN and not an infinity. Does it also always form a qNaN, or can you create an sNaN this way?

Someone wrote a test for this! http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/APFloatTest.cpp?r1=97364&r2=97363&pathrev=97364

In D55460#1324143, @jfb wrote:

This truncates the payload to fit into the significand field. Presumably that includes setting the top bit of the significand to make this a NaN and not an infinity. Does it also always form a qNaN, or can you create an sNaN this way?

Someone wrote a test for this! http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/APFloatTest.cpp?r1=97364&r2=97363&pathrev=97364

Heh, blast from the past. I guess I could also have looked at the right implementation to see that it always made a qNaN.

Anyway, yes, this seems reasonable.

Oh, actually, please embellish the unit test to include some >32-bit payloads when building double-typed NaNs.

jfb updated this revision to Diff 177560.Dec 10 2018, 10:36 AM
  • Add tests
jfb added a comment.Dec 10 2018, 10:36 AM

Oh, actually, please embellish the unit test to include some >32-bit payloads when building double-typed NaNs.

Done, I added some for 32-bit as well, where the original weren't touching top bits either.

jfb updated this revision to Diff 177562.Dec 10 2018, 10:52 AM
  • Also add ConstantFP NaN getters which match the APFloat ones (with getQNaN / getSNaN and APInt parameters).
This revision is now accepted and ready to land.Dec 10 2018, 11:19 AM
This revision was automatically updated to reflect the committed changes.