Saturating casts between integers and floats#45205
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
hanna-kruppe
force-pushed
the
saturating-casts
branch
from
190b73b to
1cb1073
Compare
There was a problem hiding this comment.
Since you have tests on both sides of the boundary for positive values, the other interesting value to test here is -2147483904., which is the other side of the boundary for negative values.
Also, if it's interesting to test these boundaries for signed conversions, the unsigned boundaries may also be interesting: 4294967040. and 4294967296.0 on the upper boundary, and -0.99999994 and -1. on the lower.
There was a problem hiding this comment.
It'd also be interesting to test a number in (-1,-0.5), such as -0.9, to make sure it's properly rounded up to 0.
src/librustc_trans/mir/constant.rs
Outdated
There was a problem hiding this comment.
Only u128 → f32 is the problematic cast here, so the 64 => branch should never be entered (i.e. it should be a bug!). There's no problem representing u128::MAX in f64 as a finite number.
src/librustc_trans/mir/rvalue.rs
Outdated
There was a problem hiding this comment.
Yes it should be, and that would have caught a bug - I applied the f32 overflow check even when casting to f64. Will add tests for that.
hanna-kruppe
force-pushed
the
saturating-casts
branch
from
1cb1073 to
dd25630
Compare
|
Incorportated the suggestions by @sunfishcode and @kennytm. |
src/librustc_trans/mir/constant.rs
Outdated
There was a problem hiding this comment.
Do you really need to hardcode this value?
There was a problem hiding this comment.
I don't need to, I just didn't see a way to calculate it from existing constants that is simpler to understand and verify, at least to me. However, I didn't put too much thought into it and suggestions are welcome.
There was a problem hiding this comment.
Where does the 8 come from? The float mantissa has 23 bits. Add the implicit bit, you get 24, which is six times UPDATE: see comment belowf. println!("{:x}", std::f32::MAX as u128); gives me six times f as well, and its the same according to the internet.
There was a problem hiding this comment.
IMO it would be best to have a constant somewhere (maybe in the file?) and use it in both places, e.g. representing it as 0xffffff_u128 << (128 - 24), which is IMO the clearest way to display the number.
There was a problem hiding this comment.
OH I get it, the 8 is needed due to rounding. Disregard my last 2 comments! I still think a constant somewhere with a nice explanation would be cool.
There was a problem hiding this comment.
Since it seems to be a number of 1s at the top of u128, how about computing it from the number of significand bits?
EDIT: Oh wow, github didn't show me @est31's comments.
There was a problem hiding this comment.
Regardless of whether it's hardcoded or computed, I'm not sure where it should live if it's a shared definition. I guess I could chuck it in common or something, but ehhhh
There was a problem hiding this comment.
I'd put it in rustc_const_math::float.
src/librustc_trans/mir/rvalue.rs
Outdated
|
cc @est31 @nagisa @alexcrichton Who's better qualified to review floating-point code? |
src/librustc_trans/mir/constant.rs
Outdated
There was a problem hiding this comment.
This is valid rust code, isn't it?
println!("{}", {let w: * const u32 = {let v = 0u32; &v}; w} as usize);
There was a problem hiding this comment.
Yes, but this is constant evaluation and pointer->integer casts in constant expression do not compile:
static Y: i32 = 0;
static Z: usize = &Y as usize;
src/librustc_trans/mir/rvalue.rs
Outdated
There was a problem hiding this comment.
You don't need the match here, the width is checked above.
There was a problem hiding this comment.
I guess that's true now. It wasn't before @kennytm pointed out that I needed the width check up there. (Same in constant evaluation.)
|
What's up with travis? The run-pass test fails with an error that looks plausible and platform-independent, but I can't reproduce it (on x86_64-windows-msvc). |
src/librustc_trans/mir/constant.rs
Outdated
There was a problem hiding this comment.
Same here, the match is not needed.
src/librustc_trans/mir/constant.rs
Outdated
There was a problem hiding this comment.
Where does the 8 come from? The float mantissa has 23 bits. Add the implicit bit, you get 24, which is six times UPDATE: see comment belowf. println!("{:x}", std::f32::MAX as u128); gives me six times f as well, and its the same according to the internet.
hanna-kruppe
force-pushed
the
saturating-casts
branch
from
dd25630 to
1fbfeca
Compare
|
@est31 This constant isn't f32::MAX, it's f32::MAX + 0.5 ULP. Evidently the constant is not as clear as I'd hoped, so I'll figure out a way to compute it that hopefully makes it clearer what's happening. How about The -1 is pretty ugly as well ¯\_(ツ)_/¯ |
src/librustc_trans/mir/rvalue.rs
Outdated
There was a problem hiding this comment.
🤣
[00:13:30] error: expected expression, found `,`
[00:13:30] --> /checkout/src/librustc_trans/mir/rvalue.rs:835:40
[00:13:30] |
[00:13:30] 835 | let infinity = consts::bitcast(, float_ty);
[00:13:30] | ^
1fbfeca to
d96d318
Compare
src/librustc_trans/mir/rvalue.rs
Outdated
There was a problem hiding this comment.
I don't know if you're interested in micro-optimizing at this point, but if you are: it looks like this code does 4 selects for f32->i32; it would be a little shorter to take advantage of the fact that fptoui/fptosi in LLVM return undef rather than having UB, so you could do the fptosi/fptoui first, and then use selects to pick between the fptosi/fptui result value and the integer min/max/0 values in the various exceptional cases. That way, you'd always need at most 3 selects.
There was a problem hiding this comment.
I played around with this option a bit but I didn't quickly find a way to write the first two selects do the clipping with two selects such that ity::MIN comes out if x is NaN. If that's not possible, i.e., a separate NaN check is always necessary, the current code is shorter (two selects) for casts such as f32 -> u16 or f64 -> f32. But perhaps I'm missing the forest for the trees? If someone can come up with a way to do three selects for signed types and two for unsigned casts, I'd gladly implement that (after I find time to address the other nits).
There was a problem hiding this comment.
Here's a way to do three selects for signed and two for unsigned, which should be generalizable to other bitwidths:
define i32 @fptosi(float %f) {
%t = fptosi float %f to i32
%c0 = fcmp olt float %f, 0xC1E0000000000000 ; -0x1p31
%c1 = fcmp oge float %f, 0x41E0000000000000 ; 0x1p31
%c2 = fcmp uno float %f, 0.000000e+00
%s0 = select i1 %c0, i32 -2147483648, i32 %t
%s1 = select i1 %c1, i32 2147483647, i32 %s0
%s2 = select i1 %c2, i32 0, i32 %s1
ret i32 %s2
}define i32 @fptoui(float %f) {
%t = fptoui float %f to i32
%c0 = fcmp ule float %f, 0.000000e+00
%c1 = fcmp oge float %f, 0x41F0000000000000 ; 0x1p32
%s0 = select i1 %c0, i32 0, i32 %t
%s1 = select i1 %c1, i32 -1, i32 %s0
ret i32 %s1
}
shepmaster
added
the
S-waiting-on-review
src/librustc_apfloat/lib.rs
Outdated
There was a problem hiding this comment.
Ehm, just use wrapping_neg? Somehow I forgot to.
There was a problem hiding this comment.
Oh, right, that's a thing.
hanna-kruppe
force-pushed
the
saturating-casts
branch
2 times, most recently
from
763bf51 to
7a894a8
Compare
|
@eddyb IIRC you wanted to run crater with the variant that errors on overflow/NaN in trans const eval? That's implemented now. (That it's implemented now also means this shouldn't be merged in the current state until we've had a crater run.) |
kennytm
added
S-waiting-on-crater
|
@bors try |
|
⌛ Trying commit 3c3082fe0763462f05c8cee7c9abe14092fa171d with merge cc787eba2ca6e3cf90a5e6b04f4fedd924787b68... |
hanna-kruppe
force-pushed
the
saturating-casts
branch
from
3cf487c to
21a89f9
Compare
|
☔ The latest upstream changes (presumably #45666) made this pull request unmergeable. Please resolve the merge conflicts. |
This affects regular code generation as well as constant evaluation in trans, but not the HIR constant evaluator because that one returns an error for overflowing casts and NaN-to-int casts. That error is conservatively correct and we should be careful to not accept more code in constant expressions. The changes to code generation are guarded by a new -Z flag, to be able to evaluate the performance impact. The trans constant evaluation changes are unconditional because they have no run time impact and don't affect type checking either.
hanna-kruppe
force-pushed
the
saturating-casts
branch
from
21a89f9 to
ce46649
Compare
|
Rebased. Travis is green. |
|
@bors r+ |
|
📌 Commit ce46649 has been approved by |
|
⌛ Testing commit ce46649 with merge 3da399728e98a72205b3563482de647c49972154... |
|
💔 Test failed - status-travis |
|
cc #45676. Details |
|
@bors r+ |
|
📌 Commit ef0b999 has been approved by |
kennytm
added
S-waiting-on-bors
Saturating casts between integers and floats Introduces a new flag, `-Z saturating-float-casts`, which makes code generation for int->float and float->int casts safe (`undef`-free), implementing [the saturating semantics laid out by](#10184 (comment)) @jorendorff for float->int casts and overflowing to infinity for `u128::MAX` -> `f32`. Constant evaluation in trans was changed to behave like HIR const eval already did, i.e., saturate for u128->f32 and report an error for problematic float->int casts. Many thanks to @eddyb, whose APFloat port simplified many parts of this patch, and made HIR constant evaluation recognize dangerous float casts as mentioned above. Also thanks to @ActuallyaDeviloper whose branchless implementation served as inspiration for this implementation. cc #10184 #41799 fixes #45134
|
☀️ Test successful - status-appveyor, status-travis |
| } | ||
|
|
||
| #[no_mangle] | ||
| pub fn f64_to_u8(x: f32) -> u16 { |
There was a problem hiding this comment.
Arg types does not match function name. Should be either f32_to_u16(x: f32) -> u16 or f64_to_u8(x: f64) -> u8.
12 participants
Introduces a new flag,
-Z saturating-float-casts, which makes code generation for int->float and float->int casts safe (undef-free), implementing the saturating semantics laid out by @jorendorff for float->int casts and overflowing to infinity foru128::MAX->f32.Constant evaluation in trans was changed to behave like HIR const eval already did, i.e., saturate for u128->f32 and report an error for problematic float->int casts.
Many thanks to @eddyb, whose APFloat port simplified many parts of this patch, and made HIR constant evaluation recognize dangerous float casts as mentioned above.
Also thanks to @ActuallyaDeviloper whose branchless implementation served as inspiration for this implementation.
cc #10184 #41799
fixes #45134