This is an archive of the discontinued LLVM Phabricator instance.

[APFloat] Enlarge ExponentType to 32bit integer
ClosedPublic

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

Details

Summary

Enlarge the size of ExponentType from 16bit integer to 32bit. This is required to prevent exponent overflow/underflow.

Note that IEEEFloat size doesn't change in 64bit or 32bit compilation targets.

Fix PR34851.

Diff Detail

Event Timeline

ekatz created this revision.Nov 3 2019, 12:27 PM
arsenm added a subscriber: arsenm.Nov 3 2019, 1:17 PM
arsenm added inline comments.
llvm/unittests/ADT/APFloatTest.cpp
2675

EXPECT_EQ

3005

EXPECT_EQ

ekatz added a reviewer: scanon.Nov 3 2019, 8:27 PM
ekatz updated this revision to Diff 227923.Nov 5 2019, 10:55 AM

Changed to EXPECT_EQ as requested.

efriedma added inline comments.
llvm/lib/Support/APFloat.cpp
1014 ↗(On Diff #227923)

These static_casts don't do anything.

1218 ↗(On Diff #227923)

EXPONENT_MIN is -32768, which is more than twice as small as the smallest exponent in any format we support, right? If the exponent is smaller than that, can we "cheat", and pretend the result came out to -32768? The result is essentially zero anyway.

Do we need to handle EXPONENT_MAX?

llvm/unittests/ADT/APFloatTest.cpp
3002

Maybe check what happens with other rounding modes, where the result isn't zero?

ekatz updated this revision to Diff 229752.Nov 17 2019, 11:38 PM
ekatz edited the summary of this revision. (Show Details)

This is a simpler take for the solution.

Also, added tests for other rounding modes, as requested.

efriedma added inline comments.Nov 18 2019, 3:18 PM
llvm/include/llvm/ADT/APFloat.h
146

What's the effect of this on the size of APFloat on a 64-bit target? On a 32-bit target?

llvm/lib/Support/APFloat.cpp
994 ↗(On Diff #229752)

Does this have some functional effect? If it does, is there a test?

ekatz marked 2 inline comments as done.Nov 18 2019, 11:25 PM
ekatz added inline comments.
llvm/include/llvm/ADT/APFloat.h
146

APFloat remains the same size. I added a note about that in the description. Maybe it is not clear enough.

detail::IEEEFloat is the only record that has a field of ExponentType type. It remains the size of 24 bytes and alignment of 8 bytes (in both 32 bit and 64 bit targets).

llvm/lib/Support/APFloat.cpp
994 ↗(On Diff #229752)

Now that I think of it, I'll just separate it into another patch. It is a bug, revealed from the tests I've made.

ekatz updated this revision to Diff 229983.Nov 19 2019, 12:09 AM
ekatz retitled this revision from [APFloat] Handle exponent underflow correctly to [APFloat] Enlarge ExponentType to 32bit integer.

Removed changes unrelated to topic.

efriedma accepted this revision.Nov 19 2019, 11:58 AM

LGTM

llvm/include/llvm/ADT/APFloat.h
146

Okay. I missed the comment in the updated commit message.

This revision is now accepted and ready to land.Nov 19 2019, 11:58 AM
This revision was automatically updated to reflect the committed changes.