Uh oh!
There was an error while loading. Please reload this page.
Saturating casts between integers and floats - #45205
Conversation
rust-highfive
commented
Oct 11, 2017
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
190b73b to
1cb1073CompareThere 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.
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.
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.
1cb1073 to
dd25630CompareIncorportated the suggestions by @sunfishcode and @kennytm. |
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.
cc @est31@nagisa@alexcrichton Who's better qualified to review floating-point code? |
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;
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). |
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.
dd25630 to
1fbfecaCompare@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 ¯\_(ツ)_/¯ |
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
d96d318CompareThere 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:
definei32@fptosi(float%f) {
%t = fptosifloat%ftoi32%c0 = fcmpoltfloat%f, 0xC1E0000000000000; -0x1p31%c1 = fcmpogefloat%f, 0x41E0000000000000; 0x1p31%c2 = fcmpunofloat%f, 0.000000e+00%s0 = selecti1%c0, i32 -2147483648, i32%t%s1 = selecti1%c1, i322147483647, i32%s0%s2 = selecti1%c2, i320, i32%s1reti32%s2
}definei32@fptoui(float%f) {
%t = fptouifloat%ftoi32%c0 = fcmpulefloat%f, 0.000000e+00%c1 = fcmpogefloat%f, 0x41F0000000000000; 0x1p32%s0 = selecti1%c0, i320, i32%t%s1 = selecti1%c1, i32 -1, i32%s0reti32%s1
}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.
763bf51 to
7a894a8Comparehanna-kruppe
commented
Oct 18, 2017
@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
commented
Oct 18, 2017
@bors try |
bors
commented
Oct 18, 2017
⌛ Trying commit 3c3082fe0763462f05c8cee7c9abe14092fa171d with merge cc787eba2ca6e3cf90a5e6b04f4fedd924787b68... |
3cf487c to
21a89f9Comparebors
commented
Nov 7, 2017
☔ 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.
21a89f9 to
ce46649Comparehanna-kruppe
commented
Nov 7, 2017
Rebased. Travis is green. |
eddyb
commented
Nov 7, 2017
@bors r+ |
bors
commented
Nov 7, 2017
📌 Commit ce46649 has been approved by |
bors
commented
Nov 7, 2017
⌛ Testing commit ce46649 with merge 3da399728e98a72205b3563482de647c49972154... |
bors
commented
Nov 8, 2017
💔 Test failed - status-travis |
cc #45676. Details |
eddyb
commented
Nov 8, 2017
@bors r+ |
bors
commented
Nov 8, 2017
📌 Commit ef0b999 has been approved by |
bors
commented
Nov 8, 2017
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#41799fixes#45134
bors
commented
Nov 8, 2017
☀️ 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.
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