Refactor float casting tests, add f16 and f128

[tgross35]

This is an attempt to remove the magic from a lot of the numbers tested, which makes things easier when adding f16 and f128. A nice side effect is that these tests now cover all int <-> float conversions with the same amount of tests.

[RalfJung]

Running out of time now, will go on later this week hopefully

[RalfJung]

I have absolutely no idea what this does

[tgross35]

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).

[RalfJung]

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?

[tgross35]

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.

[RalfJung]

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?

[tgross35]

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 learning 1. 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 )

[tgross35]

🎉 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

[RalfJung]

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.

[RalfJung]

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. :/

[RalfJung]

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).

[tgross35]

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. :/

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)

[tgross35]

Okiedoke, squashed. Thanks for all the reviews!

[RalfJung]

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+

[bors]

📌 Commit 561cd73 has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit 561cd73 with merge ddbbecb...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing ddbbecb to master...