-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor float casting tests, add f16
and f128
#3688
Conversation
1f2b349
to
6177213
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running out of time now, will go on later this week hopefully
1fa0151
to
d47ead3
Compare
7c9f7e8
to
54bd203
Compare
54bd203
to
c602a88
Compare
7937970
to
dd1051d
Compare
tests/pass/float.rs
Outdated
let adj = if isigned { 1 } else { 0 }; | ||
|
||
let exp_overflow = 1 << (<$fty>::EXPONENT_BITS - 1 - adj); | ||
let exp_max = (1 << <$fty>::EXPONENT_BITS - adj) - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have absolutely no idea what this does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how to explain it concisely since I don't entirely understand it either, could use some suggestions. This comes from the "rounding loss" magic number tests:
test_both_cast::<f32, u32>((u32::MAX - 128) as f32, u32::MAX - 255); // rounding loss
assert_eq::<u32>((u32::MAX - 127) as f32 as u32, u32::MAX); // rounds up and then becomes unrepresentable
test_both_cast::<f64, u64>((u64::MAX - 1024) as f64, u64::MAX - 2047); // rounding loss
assert_eq::<u64>((u64::MAX - 1023) as f64 as u64, u64::MAX); // rounds up and then becomes unrepresentable
Basically it calculates the maximum value that will get rounded down to an integer less than Int::MAX
(that exact value being u32::MAX - 255
/ u64::MAX - 2047
), and checks the result. Then any number greater will get rounded up to Int::MAX
or above.
I had to reverse engineer the pattern, (exp_overflow, exp_max)
are my bad names for (128, 255)
and (1024, 2047)
. I renamed them to (nearest_rep_offset, imax_offset)
but even those don't seem great. Signed numbers aren't tested in the original but the pattern holds if you shift the bits an extra position (as I do with adj
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I added these tests. As you said: I was looking for the largest integer that does round-trip properly with a float. For f32 and u32, that's u32::MAX - 128
-- once you get any bigger, it becomes a float that saturates to u32:MAX
(and causes UB with the unchecked casts).
I have no idea how to compute these numbers, I found them experimentally. Maybe it's better to not try to make them part of the general test, and just spot-check them below -- with a comment explaining this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this pattern with f16 and f128 before and I think it holds true for those as well, but don't remember. rust-lang/rust#126608 should hopefully be in pretty soon so I'll probably just wait for that then update this PR, to get the extra two cases to test the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to stick to computing these numbers, then please add comments explaining what you are computing and how.
Is there a reason we can only have these tests when fbits == ibits
? I couldn't be bothered to figure out the largest representable number for all combinations of types, but if there is a way to compute this automatically then that should be doable, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally think I wrapped my head around this - the max integer that can be represented is a saturated mantissa (including the implicit bit), shifted to fully to the left in the integer. IOW, zero out the last F::EXPONENT_BITS of I::MAX if they have the same size, which is what subtracting 255 does (255 = 0b11111111, which is a f32's exponent mask). So x = u32::MAX - 255
Then the cutoff point between rounding to x and rounding to infinity will be midway between x and the integer's max, or adding back x + 255/2
. -255 + (255 / 2) = -128
, hence that number. Signed integers don't use the top bit for their maximum, so they get one extra bit in their lower range compared to their same-sized unsigned integer. This is what I was representing with adj
, without really understanding it.
I think this should be generalizable to anything where:
F::EXPONENT_BIAS >= I::BITS
, because it needs to be able to represent the magnitude of an integer with a learning1
. Basically no f16.I::BITS > (F::MANTISSA_BITS + 1)
, because if any integer bitpattern fits in the mantissa then this is pointless.
(This isn't so much an update for you, mostly just writing down my informal proof so I remember it :). scratchpad )
55d3459
to
74fa23b
Compare
27832b9
to
dfddc68
Compare
🎉 with a better understanding of the tests in #3688 (comment), everything seems to be passing for all four float types. Ready for a (hopefully) final look. @rustbot ready |
Rustup To unblock rust-lang/miri#3688
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job at figuring this out! I am following... maybe half of this.^^ But you left a ton of comments so I hope if someone ever needs to dig into this again they will be helpful...
I do have some kind of naive questions though.
bbfe577
to
e5ad83f
Compare
In future please avoid rebases unless there are conflicts or other reasons that require a rebase... it seems I now have no way to see the diff of what you changed since my earlier review. :/ |
All right, I think we're good to go. Thanks for your patience through all these reviews! Please squash the commits (ideally in a way that the force push has no diff). |
Do you mean a squashes or specifically rebase onto latest? I'm tend to aggressively squash / fixup to keep commit history clean, because I commit frequently and otherwise sometimes things get merged with a messy log (drives me crazy...). The actual change of base wasn't intentional, I just did it from a different checkout (need to get in the habit of merge-base, but I can never remember the exact incantation to get things right). In any case the change from my previous push is in https://github.com/rust-lang/miri/compare/dfddc6856e10ecf25f9808d83f5818430fc0ce67..bbfe57710ce7072ef7417188f2d3b1891382ea01#diff-64fd96f1f6fb00d3a7940a8e5c347ce262ea751844538a2c87d4b17574a01f51. Wish GH was just better hiding change-of-base differences. So I'll just wait until you're ready and then squash one last time 😆 (edit: oh that's now) |
This is an attempt to remove the magic from a lot of the numbers tested, which should make things easier when it is time to add `f16` and `f128`. A nice side effect is that these tests now cover all int <-> float conversions with the same amount of tests. Co-authored-by: Ralf Jung <[email protected]>
8346d77
to
561cd73
Compare
Okiedoke, squashed. Thanks for all the reviews! |
Force-pushing during review in general isn't great since github tends to lose diff then, but change-of-base makes it a lot worse. If you commit frequently you can still squash the new commits before you push to avoid force-pushes. :D @bors r+ |
☀️ Test successful - checks-actions |
Rustup To unblock rust-lang/miri#3688
Rustup To unblock rust-lang/miri#3688
This is an attempt to remove the magic from a lot of the numbers tested, which makes things easier when adding
f16
andf128
. A nice side effect is that these tests now cover all int <-> float conversions with the same amount of tests.