Absorb std.math.big.rational logic into std.math.big.int; fix @intFromFloat safety check
#24188
+1,341
−1,082
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mlugg
added
the
breaking
mlugg
changed the title
@intFromFloat safety check@intFromFloat safety check
mlugg
force-pushed
the
intfromfloat-safety
branch
from
49bf21d to
b15215e
Compare
This allows legalizations to be added that aren't used by zig1 without affecting the size of zig1.
Compiler needs a subset of the legalization features enabled.
These conversion routines accept a `round` argument to control how the result is rounded and return whether the result is exact. Most callers wanted this functionality and had hacks around it being missing. Also delete `std.math.big.rational` because it was only being used for float conversion, and using rationals for that is a lot more complex than necessary. It also required an allocator, whereas the new integer routines only need to be passed enough memory to store the result.
This safety check was completely broken; it triggered unchecked illegal behavior *in order to implement the safety check*. You definitely can't do that! Instead, we must explicitly check the boundaries. This is a tiny bit fiddly, because we need to make sure we do floating-point rounding in the correct direction, and also handle the fact that the operation truncates so the boundary works differently for min vs max. Instead of implementing this safety check in Sema, there are now dedicated AIR instructions for safety-checked intfromfloat (two instructions; which one is used depends on the float mode). Currently, no backend directly implements them; instead, a `Legalize.Feature` is added which expands the safety check, and this feature is enabled for all backends we currently test, including the LLVM backend. The `u0` case is still handled in Sema, because Sema needs to check for that anyway due to the comptime-known result. The old safety check here was also completely broken and has therefore been rewritten. In that case, we just check for 'abs(input) < 1.0'. I've added a bunch of test coverage for the boundary cases of `@intFromFloat`, both for successes (in `test/behavior/cast.zig`) and failures (in `test/cases/safety/`). Resolves: ziglang#24161
There will be more call sites to `preparePanicId` as we transition away from safety checks in Sema towards safety checked instructions; it's silly for them to all have this clunky usage.
jacobly0
force-pushed
the
intfromfloat-safety
branch
from
b15215e to
d90068d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First, @jacobly0 rewrote a bunch of bigint logic for me, and that's... actually most of the lines in this PR. Then is what it was meant to be; see below.
This is breaking because it removes
std.math.big.rational.This safety check was completely broken; it triggered unchecked illegal behavior in order to implement the safety check. You definitely can't do that! Instead, we must explicitly check the boundaries. This is a tiny bit fiddly, because we need to make sure we do floating-point rounding in the correct direction, and also handle the fact that the operation truncates so the boundary works differently for min vs max.
Instead of implementing this safety check in Sema, there are now dedicated AIR instructions for safety-checked intfromfloat (two instructions; which one is used depends on the float mode). Currently, no backend directly implements them; instead, a
Legalize.Featureis added which expands the safety check, and this feature is enabled for all backends we currently test, including the LLVM backend.The
u0case is still handled in Sema, because Sema needs to check for that anyway due to the comptime-known result. The old safety check here was also completely broken and has therefore been rewritten. In that case, we just check for 'abs(input) < 1.0'.I've added a bunch of test coverage for the boundary cases of
@intFromFloat, both for successes (intest/behavior/cast.zig) and failures (intest/cases/safety/).Resolves: #24161