Nominating

brson on 31 Oct 2013

accepted for P-high, same reasoning as #10183

pnkfelix on 7 Nov 2013

I don't think this is backwards incompatible at a language level. It will not cause code that was working OK to stop working. Nominating.

pcwalton on 22 Nov 2013

changing to P-high, same reasoning as #10183

pnkfelix on 19 Dec 2013

How do we propose to solve this and #10185? Since whether behaviour is defined or not depends on the dynamic value of the number being cast, it seems the only solution is to insert dynamic checks. We seem to agree we do not want to do that for arithmetic overflow, are we happy to do it for cast overflow?

nrc on 12 Sep 2014

We could add an intrinsic to LLVM that performs a "safe conversion". @zwarich may have other ideas.

pcwalton on 12 Sep 2014

AFAIK the only solution at the moment is to use the target-specific intrinsics. That's what JavaScriptCore does, at least according to someone I asked.

zwarich on 12 Sep 2014

Oh, that's easy enough then.

pcwalton on 12 Sep 2014

ping @pnkfelix is this covered by the new overflow checking stuff?

nrc on 24 Apr 2015

These casts are not checked by rustc with debug assertions.

bluss on 7 May 2015

I'm happy to handle this, but I need a concrete solution. I personally think that it should be checked along with overflowing integer arithmetic, as it's a very similar issue. I don't really mind what we do though.

Note that this issue is currently causing an ICE when used in certain constant expressions.

Aatch on 13 Sep 2015

This allows violating memory safety in safe rust, example from this forum post:

Undefs, huh? Undefs are fun. They tend to propagate. After a few minutes of wrangling..

#[inline(never)]
pub fn f(ary: &[u8; 5]) -> &[u8] {
    let idx = 1e100f64 as usize;
    &ary[idx..]
}

fn main() {
    println!("{}", f(&[1; 5])[0xdeadbeef]);
}

segfaults on my system (latest nightly) with -O.

bluss on 13 Sep 2015

Marking with I-unsound given the violation of memory safety in safe rust.

steveklabnik on 8 Oct 2015

@bluss , this does not segfualt for me, just gives an assertion error. untagging since i was the one who added it

steveklabnik on 29 Oct 2015

Sigh, I forgot the -O, re-tagging.

steveklabnik on 29 Oct 2015

re-nominating for P-high. Apparently this was at some point P-high but got lower over time. This seems pretty important for correctness.

EDIT: didn’t react to triage comment, adding label manually.

nagisa on 21 Feb 2016

It seems like the precedent from the overflow stuff (e.g. for shifting) is to just settle on some behavior. Java seems to produce the result modulo the range, which seems not unreasonable; I'm not sure just what kind of LLVM code we'd need to handle that.

nikomatsakis on 25 Feb 2016

According to https://docs.oracle.com/javase/specs/jls/se7/html/jls-5.html#jls-5.1.3 Java also guarantees that NaN values are mapped to 0 and infinities to the minimum/maximum representable integer. Moreover the Java rule for the conversion is more complex than just wrapping, it can be a combination of saturation (for the conversion to int or long) and wrapping (for the conversion to smaller integral types, if needed). Replicating the whole conversion algorithm from Java is certainly possible, but it would require a fair amount of operations for every cast. In particular, in order to ensure that the result of a fpto[us]i operation in LLVM does not exhibit undefined behaviour, a range check would be needed.

As an alternative, I would suggest that float->int casts are guaranteed to only be valid if the truncation of the original value can be represented as a value of the destination type (or maybe as [iu]size?) and to have assertions on debug builds that trigger a panic when the value has not been represented faithfully.

The main advantages of the Java approach are that the conversion function is total, but this also means that unexpected behaviour might creep in: it would prevent undefined behaviour, but it would be easy to be tricked into not checking if the cast actually made any sense (this is unfortunately true also for the other casts :worried: ).

The other approach matches the one currently used for arithmetic operations: simple & efficient implementation in release, panics triggered by range checking in debug. Unfortunately unlike other as casts, this would make such conversion checked, which can be surprising to the user (although maybe the analogy to arithmetic operations can help here). This would also break some code, but AFAICT it should only happen for code which is currently relying on undefined behaviour (i.e. it would replace the undefined behaviour "let's return any integer, you obviously don't care which" with a panic).

ranma42 on 26 Feb 2016

👍1

The problem isn't "let's return any integer, you obviously don't care which", it is that it causes an undef which isn't a random value but rather a nasal demon value and LLVM is allowed to assume the undef never occurs enabling optimizations that do horrible incorrect things. If it was a random value, but crucially not undef, then that would be enough to fix the soundness issues. We don't need to define how unrepresentable values are represented, we just need to prevent undef.

retep998 on 26 Feb 2016

👍2

Discussed in @rust-lang/compiler meeting. The most consistent course of action remains:

when overflow checks are enabled, check for illegal casts and panic.
otherwise, we need a fallback behavior, it should be something which has minimal (ideally, zero) runtime cost for valid values, but the precise behavior is not that important, so long as it is not LLVM undef.

The main problem is that we need a concrete suggestion for option 2.

nikomatsakis on 3 Mar 2016

triage: P-medium

nikomatsakis on 3 Mar 2016

@nikomatsakis Does as ever currently panic in debug builds? If it doesn't, for consistency and predictability it seems preferable to keep it that way. (I think it _should have_, just like arithmetic, but that's a separate, and past, debate.)

glaebhoerl on 5 Mar 2016

otherwise, we need a fallback behavior, it should be something which has minimal (ideally, zero) runtime cost for valid values, but the precise behavior is not that important, so long as it is not LLVM undef.

Concrete suggestion: extract digits and exponent as u64 and bitshift digits by exponent.

fn f64_as_u64(f: f64) -> u64 {
    let (mantissa, exponent, _sign) = f.integer_decode();
    mantissa >> ((-exponent) & 63)
}

Yes it's not zero cost, but it's somewhat optimizable (would be better if we marked integer_decode inline) and at least deterministic. A future MIR-pass that expands a float->int cast could probably analyze whether the float is guaranteed to be ok to cast and skip this heavy conversion.

oli-obk on 9 Mar 2016

😕1

Does LLVM not have platform intrinsics for the conversion functions?

EDIT: @zwarich said (a long time ago):

AFAIK the only solution at the moment is to use the target-specific intrinsics. That's what JavaScriptCore does, at least according to someone I asked.

Why even bother panicking? AFAIK, @glaebhoerl is correct, as is supposed to truncate/extend, _not_ check the operands.

eddyb on 9 Mar 2016

On Sat, Mar 05, 2016 at 03:47:55AM -0800, Gábor Lehel wrote:

@nikomatsakis Does as ever currently panic in debug builds? If it doesn't, for consistency and predictability it seems preferable to keep it that way. (I think it _should have_, just like arithmetic, but that's a separate, and past, debate.)

True. I find that persuasive.

nikomatsakis on 10 Mar 2016

On Wed, Mar 09, 2016 at 02:31:05AM -0800, Eduard-Mihai Burtescu wrote:

Does LLVM not have platform intrinsics for the conversion functions?

EDIT:

AFAIK the only solution at the moment is to use the target-specific intrinsics. That's what JavaScriptCore does, at least according to someone I asked.

Why even bother panicking? AFAIK, @glaebhoerl is correct, as is supposed to truncate/extend, _not_ check the operands.

Yes, I think I was mistaken before. as is the "unchecked truncation"
operator, for better or worse, and it seems best to stay consistent
with that philosophy. Using target-specific intrinsics may be a perfectly
fine solution though?

nikomatsakis on 10 Mar 2016

@nikomatsakis: it seems the behavior hasn't been defined yet? Can you give an update about the planning regarding that?

sanmai-NL on 17 Jun 2016

Just ran into this with much smaller numbers

    let x: f64 = -1.0;
    x as u8

Results in 0, 16, etc. depending on optimizations, I was hoping it would be defined as 255 so I don't have to write x as i16 as u8.

gmorenz on 21 Aug 2016

@gmorenz Did you try !0u8?

vks on 24 Aug 2016

In context that wouldn't make sense, I was getting the f64 from a transformation on data sent over the network, with a range of [-255, 255]. I was hoping it would wrap nicely (in the exact way that <i32> as u8 wraps).

gmorenz on 24 Aug 2016

Here's a recent LLVM proposal to "kill undef" http://lists.llvm.org/pipermail/llvm-dev/2016-October/106182.html , though I'm hardly knowledgeable enough to know whether or not this would automagically resolve this issue.

bstrie on 19 Oct 2016

🎉1

They're replacing undef with poison, the semantics being slightly different. It's not going to make int -> float casts defined behavior.

notriddle on 28 Nov 2016

We probably should provide some explicit way to do a saturating cast? I’ve wanted that exact behaviour just now.

nagisa on 2 Dec 2016

👍1

Seems like this should be marked I-crash, given https://github.com/rust-lang/rust/issues/10184#issuecomment-139858153 .

jorendorff on 10 Dec 2016

We had a question about this in #rust-beginners today, someone ran into it in the wild.

steveklabnik on 25 Feb 2017

😄3

The book I'm writing with @jimblandy, _Programming Rust_, mentions this bug.

Several kinds of casts are permitted.

Numbers may be cast from any of the built-in numeric types to any other.

(...)

However, as of this writing, casting a large floating-point value to an integer type that is too small to represent it can lead to undefined behavior. This can cause crashes even in safe Rust. It is a bug in the compiler, github.com/rust-lang/rust/issues/10184.

Our deadline for this chapter is May 19. I'd love to delete that last paragraph, but I feel like we should at least have some kind of plan here first.

Apparently current JavaScriptCore uses an interesting hack on x86. They use the CVTTSD2SI instruction, then fall back on some hairy C++ if the value is out of range. Since out-of-range values currently explode, using that instruction (with no fallback!) would be an improvement on what we have now, albeit only for one architecture.

jorendorff on 3 May 2017

Honestly I think we should deprecate numeric casts with as and use From and TryFrom or something like the conv crate instead.

vks on 3 May 2017

👍6 👎4

Maybe so, but that seems orthogonal to me.

jorendorff on 3 May 2017

👍5

OK, I've just re-read this whole conversation. I think there is agreement that this operation should not panic (for general consistency with as). There are two leading contenders for what the behavior ought to be:

Some sort of defined result
- Pro: This I think is maximally consistent with our general philosophy thus far.
- Con: There doesn't seem to be a truly portable way to produce any particular defined result in this case. This implies that we would be using platform-specific intrinsics with some sort of fallback for out of range values (for example, falling back to saturation, this function that @oli-obk proposed, to Java's definition, or to whatever "hairy C++" JSC uses.
- At worst, we can just insert some ifs for the "out of range" cases.
An undefined value (not undefined behavior)
- Pro: This lets us just use the platform-specific intrinsics that are available on each platform.
- Con: It's a portability hazard. In general, I feel like we've not made use of undefined results very often, at least in the language (I'm sure we do in the libs in various places).

It's not clear to me if there is a clear precedent for what the result ought to be in the first case?

nikomatsakis on 4 May 2017

After having written that out, my preference would be to maintain a deterministic result. I feel like every place that we can hold the line on determinism is a win. I am not really sure what the result ought to be though.

I like saturation because I can understand it and it seems useful, but it seems somehow incongruent with the way that u64 as u32 does truncation. So perhaps some sort of result based on truncation makes sense, which I guess is probably what @oli-obk proposed -- I don't fully understand what that code is intended to do. =)

nikomatsakis on 4 May 2017

👍1

My code gives the correct value for things in the range 0..2^64 and deterministic but bogus values for everything else.

floats are represented by mantissa ^ exponent, e.g. 1.0 is (2 << 52) ^ -52 and since bitshifts and exponents are the same thing in binary, we can just reverse the shift (thus the negation of the exponent and the right shift).

oli-obk on 4 May 2017

+1 for determinism.

I see two semantics that make good sense for humans, and I think we should pick whichever one is faster for values that are in range, when the compiler can't optimize away any of the computation. (When the compiler knows that a value is in range, both options give the same results, so they are equally optimize-able.)

Saturation (out-of-range values become IntType::max_value()/min_value())
Modulo (out-of-range values are treated as if by converting to a bigint first, then truncating)

The table below is meant to specify both options fully. T is any machine integer type. Tmin and Tmax are T::min_value() and T::max_value(). RTZ(v) means take the mathematical value of v and Round Toward Zero to get a mathematical integer.

v | v as T (saturation) | v as T (modulo)
---- | ---- | ----
in range (Tmin <= v <= Tmax) | RTZ(v) | RTZ(v)
negative zero | 0 | 0
NaN | 0 | 0
Infinity | Tmax | 0
-Infinity | Tmin | 0
v > Tmax | Tmax | RTZ(v) truncated to fit T
v < Tmin | Tmin | RTZ(v) truncated to fit T

The ECMAScript standard specifies operations ToInt32, ToUint32, ToInt16, ToUint16, ToInt8, ToUint8, and my intent with the "modulo" option above is to match those operations in every case.

ECMAScript also specifies ToInt8Clamp which does not match either case above: it does "round half to even" rounding on fractional values rather than "round to zero".

@oli-obk's suggestion is a third way, worth considering if it's faster to compute, for values that are in range.

jorendorff on 4 May 2017

👍2 ❤1

@oli-obk What about signed integer types?

jorendorff on 4 May 2017

est31 on 7 May 2017

Throwing another proposal into the mix: Mark u128 casts to floats as unsafe and force folks to explicitly choose a way of handling it. u128 is pretty rare currently.

Manishearth on 8 May 2017

👍1

@Manishearth I’d hope for similar semantics integers → floats as floats → integers. Since both are UB-ful, and we cannot make float→integer unsafe anymore, we should probably avoid making integer→float unsafe as well.

For float→integer saturating will be faster AFAICT (resulting in a sequence of and, ~~test+jump~~ float comparison and jump, all for ~~0.66 or 0.5~~ 2-3 cycles on modern arches). I personally couldn’t care less for what exact behaviour we decide on as long as the in-range values are as fast as they possibly could be.

nagisa on 8 May 2017

Wouldn't it make sense to make it behave like overflow? So in a debug build it would panic if you do a cast with undefined behaviour. Then you could have methods for specifying the casting behaviour like 1.04E+17.saturating_cast::<u8>(), unsafe { 1.04E+17.unsafe_cast::<u8>() } and potentially others.

CryZe on 8 May 2017

Oh, I thought the issue was only for u128, and we can make that unsafe both ways.

@cryze UB should not exist even in release mode in safe code. The overflow stuff is still defined behavior.

That said, panic on debug, and on release would be great.

Manishearth on 8 May 2017

This affects:

f32 -> u8, u16, u32, u64, u128, usize (-1f32 as _ for all, f32::MAX as _ for all but u128)
f32 -> i8, i16, i32, i64, i128, isize (f32::MAX as _ for all)
f64 -> all ints (f64::MAX as _ for all)

Gankra on 8 May 2017

f32::INFINITY as u128 is also UB

nagisa on 8 May 2017

@CryZe

Wouldn't it make sense to make it behave like overflow? So in a debug build it would panic if you do a cast with undefined behaviour.

This is what I thought initially, but I was reminded that as conversions never panic at present (we don't do overflow checking with as, for better or worse). So the most analogous thing is for it to "do something defined".

nikomatsakis on 8 May 2017

FWIW, the "kill undef" thing would, in fact, provide a way to fix the memory unsafety, but leaving the result nondeterministic. One of the key components is:

3) Create a new instruction, '%y = freeze %x', that stops propagation of
poison. If the input is poison, then it returns an arbitrary, but fixed,
value. (like old undef, but each use gets the same value), otherwise it
just returns its input value.

The reason undefs can be used to violate memory safety today is that they can magically change values between uses: in particular, between a bounds check and subsequent pointer arithmetic. If rustc added a freeze after every dangerous cast, you would just get an unknown but otherwise well-behaved value. Performance-wise, freeze is basically free here, since of course the machine instruction corresponding to the cast produces a single value, not a fluctuating one; even if the optimizer feels like duplicating the cast instruction for some reason, it should be safe to do so because the result for out-of-range inputs is usually deterministic on a given architecture.

...But not deterministic across architectures, if anyone was wondering. x86 returns 0x80000000 for all bad inputs; ARM saturates for out-of-range inputs and (if I'm reading this pseudocode right) returns 0 for NaN. So if the goal is to produce a deterministic and platform-independent result, it's not enough to just use the platform's fp-to-int intrinsic; at least on ARM you also need to check the status register for an exception. This may have some overhead in itself, and certainly prevents autovectorization in the unlikely event that using the intrinsic didn't already. Alternately, I guess you could explicitly test for in-range values using regular compare operations and then use a regular float-to-int. That sounds a lot nicer on the optimizer…

comex on 8 May 2017

👍5

as conversions never panic at present

At some point we changed + to panic (on debug mode). I wouldn’t be shocked to see as panic in cases that were previously UB.

SimonSapin on 11 May 2017

👍6

If we care about checking (which we should), then we should either deprecate as (is there any use case where it's the only good option?) or at least advise against using it, and move people onto things like TryFrom and TryInto instead, which is what we said we were planning to do back when it was decided to leave as as-is. I don't feel that the cases under discussion are qualitatively different, in the abstract, from the cases where as is already defined not to do any checks. The difference is just that in practice the implementation for these cases is currently incomplete and has UB. A world where you can't rely on as doing checks (because for most types, it doesn't), and you can't rely on it not panicking (because for some types, it would), and it's not consistent, and we still haven't deprecated it seems like the worst of all of them to me.

glaebhoerl on 14 May 2017

👍3

So, I think at this point @jorendorff basically enumerated what seems to me to be the best plan:

as will have some deterministic behavior;
we'll pick a behavior based on a combination of how sensible it is, and how efficient it is

He enumerated three possibilities. I think the remaining work is to investigate those possibilities -- or at least investigate one of them. That is, actually implement it, and try to get some feeling for how "slow" or "fast" it is.

Is there anyone out there who feels motivated to take a stab at that? I'm going to tag this as E-help-wanted in hopes of attracting some a person. (@oli-obk?)

nikomatsakis on 17 May 2017

Uh, I'd rather not pay price for cross-platform consistency :/ It's garbage-in, I don't care what garbage goes out (however a debug assertion would be super helpful).

Currently all rounding/truncating functions in Rust are very slow (function calls with painstakingly precise implementations), so the as is my last resort hack for fast float rounding.

If you're going to make as anything more than bare cvttss2si, please also add a stable alternative that is just that.

kornelski on 9 Jun 2017

👍3

@pornel this isn't just UB of the theoretic kind where stuff is okay if you ignore that it is ub, it has real world implications. I've extracted #41799 from a real world code example.

est31 on 9 Jun 2017

👍3

@est31 I agree that leaving it as UB is wrong, but I've seen freeze proposed as a solution to UB. AFAIK that makes it a defined deterministic value, you just don't get to say which. That behavior is fine with me.

So I'd be fine if e.g. u128::MAX as f32 deterministically produced 17.5 on x86, and 999.0 on x86-64, and -555 on ARM.

kornelski on 9 Jun 2017

freeze would not produce a defined, deterministic, unspecified value. Its result is still "any bit pattern the compiler likes", and it is consistent only across uses of the same operation. This may sidestep the UB-producing examples people have collected above, but it would not give this:

u128::MAX as f32 deterministically produced 17.5 on x86, and 999.0 on x86-64, and -555 on ARM.

For example, if LLVM notices that u128::MAX as f32 overflows and replaces it with freeze poison, a valid lowering of fn foo() -> f32 { u128::MAX as f32 } on x86_64 might be this:

foo:
  ret

(that is, just return whatever was last stored in the return register)

hanna-kruppe on 9 Jun 2017

I see. That's still acceptable for my uses (for cases where I expect out of range values, I do clamping beforehand. Where I expect values in range, but they aren't, then I'm not going to get a correct result no matter what).

kornelski on 9 Jun 2017

👍2

I have no problem with out of range float casts returning arbitrary values as long as the values are frozen so they can't cause further undefined behavior.

retep998 on 9 Jun 2017

👍1

Is something like freeze available on LLVM? I thought that was a purely theoretical construct.

nikomatsakis on 9 Jun 2017

@nikomatsakis I've never seen it used like that (unlike poison) - it's a planned revamp of poison/undef.

eddyb on 9 Jun 2017

freeze does not exist at all in LLVM today. It's only been proposed (this PLDI paper is a self-contained version, but it's also been discussed a lot on the mailing list). The proposal seems to have considerable buy-in, but of course that is no guarantee it will be adopted, much less adopted in a timely manner. (Removing the pointee types from pointer types has been accepted for years and it's still not done.)

hanna-kruppe on 9 Jun 2017

Do we want to open up an RFC to get more widespread discussion on the changes being proposed here? IMO, anything that potentially impacts the performance of as is going to be contentious, but it will be doubly contentious if we don't give people the chance to make their voice heard.

bstrie on 14 Jun 2017

👍1

I'm a Julia developer, and I've been following this issue for a while, as we share the same LLVM backend and so have similar issues. In case it's of interest, here is what we have settled on (with approximate timings for a single function on my machine):

unsafe_trunc(Int64, x) maps directly to the corresponding LLVM intrinsic fptosi (1.5 ns)
trunc(Int64, x) throws an exception for out of range values (3 ns)
convert(Int64, x) throws an exception for out of range or non-integer values (6 ns)

Also, I have asked on the mailing list about making the undefined behaviour a little more defined, but did not receive a very promising response.

simonbyrne on 14 Jun 2017

❤16 👍6

@bstrie I'm fine with an RFC, but I think it'd definitely be useful to have data! @simonbyrne's comment is super helpful in that regard, however.

nikomatsakis on 14 Jun 2017

I've toyed around with JS semantics (the modulo @jorendorff mentioned) and the Java semantics which appear to be the "saturation" column. In case those links expire it's JS and Java.

I also whipped up a quick implementation of saturation in Rust which I think (?) is correct. And got some benchmark numbers as well. Interestingly I'm seeing that the saturating implementation is 2-3x slower than the intrinsic, which is different from what @simonbyrne found with only 2x slower.

I'm not entirely sure how to implement the "mod" semantics in Rust...

To me, though, it seems clear that we'll need a slew of f32::as_u32_unchecked() methods and such for those who need the performance.

alexcrichton on 2 Sep 2017

❤1

it seems clear that we'll need a slew of f32::as_u32_unchecked() methods and such for those who need the performance.

That's a bummer - or do you mean a safe but implementation-defined variant?

eddyb on 2 Sep 2017

Is there no option for an implementation defined fast default?

Manishearth on 2 Sep 2017

👍4

@eddyb I was thinking that we'd just have unsafe fn as_u32_unchecked(self) -> u32 on f32 and such which is a direct analog to what as is today.

I'm certainly not going to claim that the Rust implementation I wrote is optimal, but I was under the impression that when reading this thread determinism and safety were more important than speed in this context most of the time. The unsafe escape hatch is for those on the other side of the fence.

alexcrichton on 2 Sep 2017

👍1

So there's no cheap platform-dependent variant? I want something that's fast, gives an unspecified value when outside of bounds and is safe. I don't want UB for some inputs and I think that's too dangerous for common use, if we can do better.

eddyb on 2 Sep 2017

👍2

As far as I am aware, on most if not all platforms the canonical way to implement this conversion does something to out-of-range inputs that isn't UB. But LLVM does not seem to have any way to pick that option (whatever it may be) over UB. If we could convince LLVM developers to introduce an intrinsic that yields an "unspecified but not undef/poison" result on out-of-range inputs, we could use that.

But I'd estimate that someone in this thread would have to write a convincing RFC (on the llvm-dev list), get buy-in, and implement it (in backends we care about, and with a fall-back implementation for other targets). Probably easier than convincing llvm-dev to make the existing casts not-UB (because it side-steps questions like "will this make any C and C++ programs slower"), but still not very easy.

hanna-kruppe on 2 Sep 2017

Just in case you will be choosing between these:

Saturation (out-of-range values become IntType::max_value()/min_value())
Modulo (out-of-range values are treated as if by converting to a bigint first, then truncating)

IMO only saturation would make sense here, because absolute precision of floating point quickly drops as values get large, so at some point the modulo would be something useless like all zeroes.

sp-1234 on 22 Sep 2017

👍3

I marked this as E-needs-mentor and tagged it with WG-compiler-middle since it seems the impl period might be a great time to investigate this further! My existing notes on the plan of record are pretty sparse though, so it'd be great if someone from @rust-lang/compiler wanted to help elaborate a those a bit further!

nikomatsakis on 22 Sep 2017

@nikomatsakis

IIRC LLVM are planning to eventually implement freeze, which should allow us to deal with the UB by doing a freeze.

arielb1 on 24 Sep 2017

My results so far: https://gist.github.com/s3bk/4bdfbe2acca30fcf587006ebb4811744

The _array variants run a loop of 1024 values.
_cast: x as i32
_clip: x.min(MAX).max(MIN) as i32
_panic: panics if x is out of bounds
_zero: sets the result to zero if out of bounds

test bench_array_cast       ... bench:       1,840 ns/iter (+/- 37)
test bench_array_cast_clip  ... bench:       2,657 ns/iter (+/- 13)
test bench_array_cast_panic ... bench:       2,397 ns/iter (+/- 20)
test bench_array_cast_zero  ... bench:       2,671 ns/iter (+/- 19)
test bench_cast             ... bench:           2 ns/iter (+/- 0)
test bench_cast_clip        ... bench:           2 ns/iter (+/- 0)
test bench_cast_panic       ... bench:           2 ns/iter (+/- 0)
test bench_cast_zero        ... bench:           2 ns/iter (+/- 0)

ghost on 24 Sep 2017

Perhaps you need not round the results to integer for individual operations. Clearly there must be some difference behind these 2 ns/iter. Or is it really like this, _exactly_ 2 ns for all 4 variants?

sp-1234 on 25 Sep 2017

@sp-1234 I wonder if it's partially optimized out.

eddyb on 25 Sep 2017

@sp-1234 It is too fast to measure. The non-array benchmarks are basically useless.
If you force the single-value functions to be functions via #[inline(never)], you get 2ns vs 3ns.

ghost on 25 Sep 2017

@arielb1
I have some reservations regarding freeze. If I understand correctly, a frozen undef can still contain any arbitrary value, it just won't change between uses. In practice, the compiler will probably reuse a register or stack slot.

However this means that we can now read uninitialized memory from safe code. This could lead to secret data being leaked, somewhat like Heartbleed. It's debatable whether this is truly considered UB from the point of view of Rust, but it clearly seems undesirable.

Amanieu on 25 Sep 2017

👍5

I ran @s3bk's benchmark locally. I can confirm the scalar versions are optimized out completely, and the asm for the array variants also looks suspiciously well-optimized: for example, the loops are vectorized, which is nice but makes it hard to extrapolate performance to scalar code.

Unfortunately, spamming black_box doesn't seem to help. I do see the asm doing useful work, but running the benchmark still consistently gives 0ns for the scalar benchmarks (except cast_zero, which shows 1ns). I see that @alexcrichton performed the comparison 100 times in their benchmarks, so I adopted the same hack. I'm now seeing these numbers (source code):

test bench_cast             ... bench:          53 ns/iter (+/- 0)
test bench_cast_clip        ... bench:         164 ns/iter (+/- 1)
test bench_cast_panic       ... bench:         172 ns/iter (+/- 2)
test bench_cast_zero        ... bench:         100 ns/iter (+/- 0)

The array benchmarks vary too much for me to trust them. Well, truth to be told, I'm skeptical of the test benchmarking infrastructure anyway, especially after seeing the above numbers compared to the flat 0ns I got previously. Furthermore, even just 100 iterations of black_box(x); (as a baseline) takes 34ns, which makes it even harder to reliably interpret those numbers.

Two points worth noting:

Despite not handling NaN specifically (it returns -inf instead of 0?), the cast_clip implementation appears to be slower than @alexcrichton's saturating cast (note that their run and mine have roughly the same timing for as casts, 53-54ns).
Unlike @s3bk's array results, I'm seeing cast_panic being slower than the other checked casts. I also see an even greater slowdown on the array benchmarks. Maybe these things are just highly dependent on microarchitectural details and/or optimizer mood?

For the record, I've measured with rustc 1.21.0-nightly (d692a91fa 2017-08-04), -C opt-level=3, on an i7-6700K under light load.

In conclusion, I conclude that don't have reliable data so far and that getting more reliable data seems hard. Furthermore, I strongly doubt that any real application spends even 1% of its wall clock time on this operation. Therefore, I would suggest to move forward by implementing saturating as casts in rustc, behind a -Z flag, and then running some non-artificial benchmarks with and without this flag to determine the impact on realistic applications.

Edit: I would also recommend to run such benchmarks on a variety of architectures (e.g., including ARM) and microarchitectures, if at all possible.

hanna-kruppe on 26 Sep 2017

👍4

I admit I'm not that familiar with rust, but I think this line is subtly incorrect: std::i32::MAX (2^31-1) is not exactly representable as a Float32, so std::i32::MAX as f32 will be rounded to the nearest representable value (2^31). If this value is used as the argument x, the result is technically undefined. Replacing with a strict inequality should fix this case.

simonbyrne on 27 Sep 2017

Yeah, we had exactly that problem in Servo before. The final solution was to cast to f64 and then clamp.

There are other solutions but they're pretty tricky and rust doesn't expose nice APIs for dealing with this well.

Manishearth on 27 Sep 2017

using 0x7FFF_FF80i32 as upper limit and -0x8000_0000i32 should solve this without casting to f64.
edit: use the correct value.

ghost on 27 Sep 2017

I think you mean 0x7fff_ff80, but simply using a strict inequality would probably make the intention of the code clearer.

simonbyrne on 27 Sep 2017

as in x < 0x8000_0000u32 as f32 ? That would probably be a good idea.

ghost on 27 Sep 2017

👍1

I think of all the suggested deterministic options, clamping is geneally most useful one because I think it's done often anyway. If the conversion type would actually be documentated to be saturating, manual clamping would become unnecessary.

I am only a little worried about the suggested implementation because it doesn't properly translate to machine instructions and it relies heavily on branching. Branching makes the performance dependent on specific data patterns. In the test cases given above everything looks (comparatively) fast because the same branch is taken always and the processor has good branch prediction data from many previous loop iterations. The real world will probably not look like that. Additionally the branching hurts the ability of the compiler to vectorize the code. I disagree with the opinion of @rkruppe , that the operation shouldn't also be tested in combination with vectorization. Vectorization is important in high performance code and being able to vectorize simple casts on common architectures should be a crucial requirement.

For the reasons given above, I played around with an alternative branchless and data flow oriented version of @alexcrichton 's cast with saturation semantics and @simonbyrne 's fix. I implemented it for u16, i16 and i32 since they all have to cover slightly different cases which result in varying performance.

The results:

test i16_bench_array_cast       ... bench:          99 ns/iter (+/- 2)
test i16_bench_array_cast_clip  ... bench:         197 ns/iter (+/- 3)
test i16_bench_array_cast_clip2 ... bench:         113 ns/iter (+/- 3)
test i16_bench_cast             ... bench:          76 ns/iter (+/- 1)
test i16_bench_cast_clip        ... bench:         218 ns/iter (+/- 25)
test i16_bench_cast_clip2       ... bench:         148 ns/iter (+/- 4)
test i16_bench_rng_cast         ... bench:       1,181 ns/iter (+/- 17)
test i16_bench_rng_cast_clip    ... bench:       1,952 ns/iter (+/- 27)
test i16_bench_rng_cast_clip2   ... bench:       1,287 ns/iter (+/- 19)

test i32_bench_array_cast       ... bench:         114 ns/iter (+/- 1)
test i32_bench_array_cast_clip  ... bench:         200 ns/iter (+/- 3)
test i32_bench_array_cast_clip2 ... bench:         128 ns/iter (+/- 3)
test i32_bench_cast             ... bench:          74 ns/iter (+/- 1)
test i32_bench_cast_clip        ... bench:         168 ns/iter (+/- 3)
test i32_bench_cast_clip2       ... bench:         189 ns/iter (+/- 3)
test i32_bench_rng_cast         ... bench:       1,184 ns/iter (+/- 13)
test i32_bench_rng_cast_clip    ... bench:       2,398 ns/iter (+/- 41)
test i32_bench_rng_cast_clip2   ... bench:       1,349 ns/iter (+/- 19)

test u16_bench_array_cast       ... bench:          99 ns/iter (+/- 1)
test u16_bench_array_cast_clip  ... bench:         136 ns/iter (+/- 3)
test u16_bench_array_cast_clip2 ... bench:         105 ns/iter (+/- 3)
test u16_bench_cast             ... bench:          76 ns/iter (+/- 2)
test u16_bench_cast_clip        ... bench:         184 ns/iter (+/- 7)
test u16_bench_cast_clip2       ... bench:         110 ns/iter (+/- 0)
test u16_bench_rng_cast         ... bench:       1,178 ns/iter (+/- 22)
test u16_bench_rng_cast_clip    ... bench:       1,336 ns/iter (+/- 26)
test u16_bench_rng_cast_clip2   ... bench:       1,207 ns/iter (+/- 21)

The test was run on an Intel Haswell i5-4570 CPU and Rust 1.22.0-nightly.
clip2 is the new branchless implementation. It agrees with clip on all 2^32 possible f32 input values.

For the rng benchmarks, random input values are used that hit different cases often. This uncovers the _extreme_ performance cost (roughly 10 times the normal cost!!!) that occurs if branch prediction fails. I think it is _very_ important to consider this. It's not the average real world performance either, but it's still a possible case and some applications will hit this. People will expect a f32 cast to have consistent performance.

Assmbly comparison on x86: https://godbolt.org/g/AhdF71
The branchless version very nicely maps to the minss/maxss instructions.

Unfortunately I wasn't able to make godbolt generate ARM assembly from Rust, but here is a ARM comparison of the methods with Clang: https://godbolt.org/g/s7ronw
Without being able to test the code and knowing much of ARM: The code size seems smaller too and LLVM mostly generates vmax/vmin which looks promising. Maybe LLVM could be teached eventually to fold most of the code into a single instruction?

ActuallyaDeviloper on 28 Sep 2017

❤6

@ActuallyaDeviloper The asm and the benchmark results look very good! Furthermore, branchless code like yours is probably easier to generate in rustc than the nested conditionals of other solutions (for the record, I am assuming we want to generate inline IR instead of calling a lang item function). Thank you so much for writing this.

I have a question about u16_cast_clip2: it doesn't seem to handle NaN?! There is a comment talking about NaN, but I believe the function will pass NaN through unmodified and attempt to cast it to f32 (and even if it didn't, it would produce one of the boundary values rather than 0).

PS: To be clear, I was not trying to imply that it's unimportant whether the cast can be vectorized. It clearly is important if the surrounding code is otherwise vectorizable. But scalar performance is also important, as vectorization is often not applicable, and the benchmarks I was commenting on were not making any statement about scalar performance. Out of interest, have you checked the asm of the *array* benchmarks to see if they're still vectorized with your implementation?

hanna-kruppe on 28 Sep 2017

@rkruppe You are right, I accidently swapped the sides of the if and forgot about that. f32 as u16 happend to do the right thing by truncating the upper 0x8000 away, so the tests didn't catch it either. I fixed the problem now by swapping the branches again and testing all methods with if (y.is_nan()) { panic!("NaN"); } this time.

I updated my previous post. The x86 code did not change significantly at all but unfortunately the change stops LLVM from generating vmax in the u16 ARM case for some reason. I assume this has to do with some details about NaN handling of that ARM instruction or maybe it's a LLVM limitation.

For why it works, notice that the lower boundary value is actually 0 for unsigned values. So NaN and the lower bound can be catched at the same time.

The array versions are vectorized.
Godbolt: https://godbolt.org/g/HnmsSV

ActuallyaDeviloper on 29 Sep 2017

Re: the ARM asm, I believe the reason vmax is not used any more is that it returns NaN if either operand is NaN. The code is still branchless, though, it just uses predicated moves (vmovgt, referring to to the result of the earlier vcmp with 0).

For why it works, notice that the lower boundary value is actually 0 for unsigned values. So NaN and the lower bound can be catched at the same time.

Ohhh, right. Nice.

hanna-kruppe on 29 Sep 2017

I would suggest to move forward by implementing saturating as casts in rustc, behind a -Z flag

I have implemented this and will file a PR once I also fixed #41799 and have a lot more tests.

hanna-kruppe on 9 Oct 2017

🎉10

45134 has pointed out a code path that I missed (generation of LLVM constant expressions – this is separate from rustc's own constant evaluation). I'll roll a fix for that into the same PR, but it will take a little while longer.

hanna-kruppe on 9 Oct 2017

@rkruppe You should coordinate with @oli-obk so that miri ends up with the same changes.

eddyb on 10 Oct 2017

Pull request is up: #45205

hanna-kruppe on 11 Oct 2017

45205 has been merged, so anyone can now (well, starting with the next nightly) measure the performance impact of saturation by passing `-Z saturating-float-casts` via `RUSTFLAGS`. [1] Such measurements would be very valuable for deciding how to proceed with this issue.

[1] Strictly speaking, this won't affect the non-generic, non-#[inline] portions of the standard library, so to be 100% accurate you'd want to locally build std with Xargo. However, I don't expect that there will be a lot of code affected by this (the various conversion trait impls are #[inline], for example).

hanna-kruppe on 8 Nov 2017

@rkruppe I suggest starting an internals/users page to collect data, in the same vein as https://internals.rust-lang.org/t/help-us-benchmark-incremental-compilation/6153/ (we can then also link people to that, rather than some random comments in our issue tracker)

Gankra on 9 Nov 2017

@rkruppe you should create a tracking issue. This discussion is split up into two issues already. That's not good!

est31 on 9 Nov 2017

@Gankro Yeah I agree, but it may be a few days before I find the time to write that post properly, so I figured I'd solicit feedback from the people subscribed to this issue in the mean time.

@est31 Hmm. Although the -Z flag covers both cast directions (which may have been a mistake, in retrospect), it seems unlikely that we'll flip the switch on both at the same time, and there's little overlap between the two in terms of what must be discussed (e.g., this issue hinges on the performance of saturation, while in #41799 it's agreed upon what the right solution is).
It is a bit silly that benchmarks primarily targeted at this issue would also measure the impact of the fix to #41799, but that can at most lead to overreporting of performance regressions, so I'm sort of okay with that. (But if anyone is motivated to split the -Z flag into two, go ahead.)

I've considered a tracking issue for the task of removing the flag once it has outlived its usefulness, but I don't see the need to merging the discussions occuring here and in #41799.

hanna-kruppe on 9 Nov 2017

I have drafted up an internals post: https://gist.github.com/Gankro/feab9fb0c42881984caf93c7ad494ebd

Feel free to copy that, or just give me notes so I can post it. (note I'm a bit confused about the const fn behaviour)

Gankra on 9 Nov 2017

One additional tidbit is that the cost of float->int conversions is specific to the current implementation, rather than being fundamental. On x86, ~~cvtss2si~~cvttss2si returns 0x80000000 in the too-low, too-high, and nan cases, so one could implement -Zsaturating-float-casts with a ~~cvtss2si~~cvttss2si followed by special code in the 0x80000000 case, so it could be just a single compare-and-predictable-branch in the common case. On ARM, vcvt.s32.f32 has the -Zsaturating-float-casts semantics already. LLVM doesn't currently optimize the extra checks away in either case.

sunfishcode on 9 Nov 2017

@Gankro

Awesome, thanks a lot! I left a few notes on the gist. After reading this, I'd like to take a stab at separating u128->f32 casts from the -Z flag. Just for the sake of getting rid of the distracting caveat about the flag covering two orthogonal features.

hanna-kruppe on 9 Nov 2017

(I've filed #45900 to refocus the -Z flag so that it only covers the float->int issue)

hanna-kruppe on 10 Nov 2017

It would be nice if we could get platform-specific implementations a la @sunfishcode (at least for x86) before asking for mass benchmarking. It shouldn't be very difficult.

comex on 10 Nov 2017

The problem is that LLVM doesn't currently provide a way to do this, as far as I know, except maybe with inline asm which I wouldn't necessarily recommend for a release.

sunfishcode on 10 Nov 2017

I have updated the draft to reflect discussion (basically ripping out any inline mention of u128 -> f32 to an extra section at the end).

Gankra on 10 Nov 2017

@sunfishcode Are you sure? Isn't the llvm.x86.sse.cvttss2si intrinsic what you're looking for?

Here is a playground link that uses it:

https://play.rust-lang.org/?gist=33cf9e0871df2eb2475b845af4f1b574&version=nightly

In release mode, float_to_int_with_intrinsic and float_to_int_with_as both compile down to a single instruction. (In debug mode, float_to_int_with_intrinsic wastes a few instructions putting zero into the high, but it's not too bad.)

It even seems to do constant folding correctly. For example,

float_to_int_with_intrinsic(42.0)

becomes

movl    $42, %eax

But an out-of-range value,

float_to_int_with_intrinsic(42.0e33)

does not get folded:

cvttss2si   .LCPI2_0(%rip), %eax

(Ideally it would fold to constant 0x80000000, but that's no big deal. The important thing is that it doesn't produce undef.)

comex on 10 Nov 2017

Oh, cool. It looks like that would work!

sunfishcode on 10 Nov 2017

It's cool to know that we do, after all, have a way to build on cvttss2si. However, I do not agree that it's clearly better to change the implementation to use it before we call for benchmarks:

Most people will benchmark on x86, so if we special case x86, we'll get far less data on the general implementation, which will still be used on most other targets. Admittedly, it's already difficult to infer anything about other architectures, but a wholly different implementation makes it outright impossible.

Second, if we collect benchmarks now, with the "simple" solution, and find that there are no performance regressions in real code (and tbh that's what I expect), then we don't even need to go through the trouble of trying to optimize this code path further.

Finally, I'm not even sure building on cvttss2si will be faster than what we have now (though on ARM, just using the appropriate instruction is clearly better):

You need a compare to notice that the conversion returns 0x80000000, and if that's the case you still need another compare (of the input value) to know whether you should return int::MIN or int::MAX. And if it's a signed integer type, I don't see how to avoid a third compare to distinguish NaN. So in the worst case:
- you don't save in the number of compares/selects
- you're trading a float compare for an int compare, which might be nice for OoO cores (if you're bottlenecked on FUs that can do compares, which seems like a relatively big if), but that compare is also dependent on the float->int comparison, while the comparisons in the current implementation are all independent, so it's far from obvious that this is a win.
Vectorization probably becomes more difficult or impossible. I don't expect that the loop vectorizer handles this intrinsic at all.
It's also worth noting that (AFAIK) this strategy only applies to some integer types. f32 -> u8, for example, will need additional fixups of the result, which make this strategy pretty clearly unprofitable. I'm not quite sure which types are affected by this (e.g., I don't know if there's an instruction for f32 -> u32), but an application that uses only those types won't benefit at all.
You could do a branching solution with only one compare in the happy path (as opposed to two or three compares, and thus branches, as previous solutions did). However, as @ActuallyaDeviloper argued earlier, branching may not be desirable: Performance now becomes even more workload-dependent and branch-prediction-dependent.

hanna-kruppe on 10 Nov 2017

👍1

Is it safe to assume that we're going to need a slew of unsafe fn as_u32_unchecked(self) -> u32 and friends regardless of what the benchmarking shows? What other potential recourse would someone have if they did end up observing a slowdown?

bstrie on 11 Nov 2017

@bstrie I think it'd make more sense, in a case like that, to do something like extending the syntax to as <type> [unchecked] and requiring the unchecked only be present in unsafe contexts.

As I see it, a forest of _unchecked functions as variants of as casting would be a wart, both as far as intuitiveness goes, and when it comes to generating clean, usable documentation.

ssokolow on 11 Nov 2017

@ssokolow Adding syntax should always be a last resort, especially if all of this can be taken care of with just ten rote functions. Even having a generic foo.as_unchecked::<u32>() would be preferable to syntactic changes (and the concomitant interminable bikeshed), especially since we ought to be reducing, not increasing, the number of things that unsafe unlocks.

bstrie on 11 Nov 2017

👍1

Point. The turbofish slipped my mind when considering options and, in hindsight, I'm not exactly firing on all cylinders this evening either, so I should have been more cautious about commenting on design decisions.

That said, it feels wrong to bake the destination type into the function name... inelegant and a potential burden on future evolution of the language. The turbofish feels like a better option.

ssokolow on 11 Nov 2017

A generic method could be supported by a new set of UncheckedFrom / UncheckedInto traits with unsafe fn methods, joining the From / Into and TryFrom / TryInto collection.

SimonSapin on 11 Nov 2017

@bstrie One alternative solution for people whose code got slower could be to use an intrinsic (e.g., via stdsimd) to access the underlying hardware instruction. I argued earlier that this has downsides for the optimizer – auto-vectorization likely suffers, and LLVM can't exploit it returning undef on out-of-range inputs – but it does offer a way to do the cast without any extra work at run time. I can't decide if this is good enough, but it seems at least plausible that it might be.

hanna-kruppe on 11 Nov 2017

Some notes on conversions in the x86 instruction set:

SSE2 is actually relatively limited in which conversion operations it gives you. You have:

CVTTSS2SI family with 32-bit register: converts single float to i32
CVTTSS2SI family with 64-bit register: converts single float to i64 (x86-64 only)
CVTTPS2PI family: converts two floats to two i32s

Each of those has variants for f32 and f64 (as well as variants that round instead of truncating, but that's useless here).

But there is nothing for unsigned integers, nothing for sizes smaller than 32, and if you're on 32-bit x86, nothing for 64-bit. Later instruction set extensions add more functionality, but it seems barely anybody compiles for those.

As a result, the existing ('unsafe') behavior:

To convert to u32, compilers convert to i64 and truncate the resulting integer. (This produces odd behavior for out-of-range values, but that's UB so who cares.)
To convert to anything 16-bit or 8-bit, compilers convert to i64 or i32 and truncate the resulting integer.
To convert to u64, compilers generate a morass of instructions. For f32 to u64 GCC and LLVM generate an equivalent of:

fn f32_to_u64(f: f32) -> u64 {
    const CUTOFF: f32 = 0x8000000000000000 as f32; // 2^63 exactly
    if !(f >= CUTOFF) { // less, or NaN
        // just use the signed conversion
        f as i64 as u64
    } else {
        0x8000000000000000u64 + ((f - CUTOFF) as i64 as u64)
    }
}

Unrelated fun fact: "Convert-than-truncate" code generation is what causes the "parallel universes" glitch in Super Mario 64. The collision detection code first MIPS instruction to convert f32 coordinates to i32, then truncates to i16; thus, coordinates that fit in i16 but not i32 'wrap', e.g. going to coordinate 65536.0 gets you collision detection for 0.0.

Anyway, conclusions:

"Test for 0x80000000 and have special handler" only works for conversions to i32 and i64.
For conversions to u32, u/i16, and u/i8, however, "test if truncated/sign-extended output differs from original" is an equivalent. (This would scoop up both integers that were in range for the original conversion but out of range for the final type, and 0x8000000000000000, the indicator that the float was NaN or out of range for the original conversion.)
But the cost of a branch and a bunch of extra code for that case is probably overkill. It may be OK if branches can be avoided.

@ActuallyaDeviloper's minss/maxss based approach is not so bad! The minimal form,

minss %xmm2, %xmm1
maxss %xmm3, %xmm1
cvttss2si %rax, %xmm1

is only three instructions (which have decent code size and throughput/latency) and no branches.

However:

The pure-Rust version needs an extra test for NaN. For conversions to 32-bit or smaller, that can be avoided using intrinsics, by using 64-bit cvttss2si and truncating the result. If the input was not NaN, the min/max ensure that integer is unchanged by truncation. If the input was NaN, the integer is 0x8000000000000000 which truncates to 0.
I didn't include the cost of loading 2147483647.0 and -2148473648.0 into the registers, typically one mov from memory each.
For f32, 2147483647.0 cannot be represented exactly, so that doesn't actually work: you need another check. That makes things much worse. Ditto for f64 to u/i64, but f64 to u/i32 doesn't have this problem.

I suggest a compromise between the two approaches:

For f32/f64 to u/i16 and u/i8, and f64 to u/i32, go with min/max + truncation, as above, e.g.:

    let f = if f > 32767.0 { 32767.0 } else { f };
    let f = if f < -32768.0 { -32768.0 } else { f };
    cvttss2si(f) as i16

(For u/i16 and u/i8, the original conversion can be to i32; for f64 to u/i32, it needs to be to i64.)

For f32/64 to u32,

    let r = cvttss2si64(f) as u32;
    if f >= 4294967296.0 { 4294967295 } else { r }

is only a few instructions and no branches:

    cvttss2si   %xmm0, %rcx
    ucomiss .LCPI0_0(%rip), %xmm0
    movl    $-1, %eax
    cmovbl  %ecx, %eax

For f32/64 to i64, maybe

    let r = cvttss2si64(f);
    if f >= 9223372036854775808. {
        9223372036854775807 
    } else if f != f {
        0
    } else {
        r
    }

This produces a longer (still branchless) sequence:

    cvttss2si   %xmm0, %rax
    xorl    %ecx, %ecx
    ucomiss %xmm0, %xmm0
    cmovnpq %rax, %rcx
    ucomiss .LCPI0_0(%rip), %xmm0
    movabsq $9223372036854775807, %rax
    cmovbq  %rcx, %rax

…but at least we save one comparison compared to the naive approach, as if f is too small, 0x8000000000000000 is already the correct answer (i.e. i64::MIN).

For f32 to i32, not sure whether it would be preferable to do the same as previous, or just convert to f64 first and then do the shorter min/max thing.
u64 is a mess I don't feel like thinking about. :p

comex on 12 Nov 2017

❤9 👍2

Call for benchmarks is up: https://internals.rust-lang.org/t/help-us-benchmark-saturating-float-casts/6231

hanna-kruppe on 13 Nov 2017

❤3

In https://internals.rust-lang.org/t/help-us-benchmark-saturating-float-casts/6231/14 someone reported a measurable and significant slowdown on JPEG encoding with the image crate. I've minimized the program so that it's self-contained and mostly focused on the parts that are related to the slowdown: https://gist.github.com/rkruppe/4e7972a209f74654ebd872eb4bc57722 (this program shows ~ 15% slowdown for me with saturating casts).

Note that the casts are f32->u8 (rgb_to_ycbcr) and f32->i32 (encode_rgb, "Quantization" loop) in equal proportions. It also looks like the inputs are all in range, i.e., saturation never actually kicks in, but in the case of the f32->u8 this can only be verified by calculating the minimum and maximum of a polynominal and accounting for rounding error, which is a lot to ask. The f32->i32 casts are more obviously in range for i32, but only because the elements of self.tables are nonzero, which is (apparently?) not that easy for the optimizer to show, especially in the original program. tl;dr: The saturation checks are there to stay, the only hope is making them faster.

I've also poked at the LLVM IR some -- it appears literally the only difference are the comparisons and selects from the saturating casts. A quick look indicates the asm has corresponding instructions and of course a bunch more live values (which lead to more spills).

@comex Do you think f32->u8 and f32->i32 casts can be made measurably faster with CVTTSS2SI?

hanna-kruppe on 19 Nov 2017

👍3 ❤1

Minor update, as of rustc 1.28.0-nightly (952f344cd 2018-05-18), the -Zsaturating-float-casts flag still causes the code in https://github.com/rust-lang/rust/issues/10184#issuecomment-345479698 to be ~20% slower on x86_64. Which means LLVM 6 hasn't changed anything.

| Flags | Timing |
|-------|-------:|
| -Copt-level=3 -Ctarget-cpu=native | 325,699 ns/iter (+/- 7,607) |
| -Copt-level=3 -Ctarget-cpu=native -Zsaturating-float-casts | 386,962 ns/iter (+/- 11,601)
(19% slower) |
| -Copt-level=3 | 331,521 ns/iter (+/- 14,096) |
| -Copt-level=3 -Zsaturating-float-casts | 413,572 ns/iter (+/- 19,183)
(25% slower) |

kennytm on 19 May 2018

@kennytm Did we expect LLVM 6 to change something? Are they discussing a particular enhancement that would benefit this use case? If so, what's the ticket number?

bstrie on 22 Jun 2018

@insanitybit It... appears to still be open...?

shepmaster on 20 Jul 2018

Welp, no clue what I was looking at. Thanks!

insanitybit on 20 Jul 2018

@rkruppe didn't we ensure that float to int casts are no longer UB in LLVM
(by changing docs)?

On Jul 20, 2018 4:31 AM, "Colin" notifications@github.com wrote:

Welp, no clue what I was looking at.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/10184#issuecomment-406462053,
or mute
the thread
https://github.com/notifications/unsubscribe-auth/AApc0v3rJHhZMD7Kv7RC8xkGOiIhkGB1ks5uITMHgaJpZM4BJ45C
.

nagisa on 20 Jul 2018

@nagisa Maybe you’re thinking of f32::from_bits(v: u32) -> f32 (and similarly f64)? It used to do some normalization of NaNs but now is just transmute.

This issue is about as conversions which try to approximate the numerical value.

SimonSapin on 20 Jul 2018

@nagisa You might be thinking of float->float casts, see #15536 and https://github.com/rust-lang-nursery/nomicon/pull/65.

hanna-kruppe on 20 Jul 2018

Ah, yes, that was float to float.

On Fri, Jul 20, 2018, 12:24 Robin Kruppe notifications@github.com wrote:

@nagisa https://github.com/nagisa You might be thinking of float->float
casts, see #15536 https://github.com/rust-lang/rust/issues/15536 and
rust-lang-nursery/nomicon#65
https://github.com/rust-lang-nursery/nomicon/pull/65.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/10184#issuecomment-406542903,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AApc0gA24Hz8ndnYhRXCyacd3HdUSZjYks5uIaHegaJpZM4BJ45C
.

nagisa on 20 Jul 2018

LLVM 7 release notes mention something:

Optimization of floating-point casts is improved. This may cause surprising results for code that is relying on the undefined behavior of overflowing casts. The optimization can be disabled by specifying a function attribute: "strict-float-cast-overflow"="false". This attribute may be created by the clang option -fno-strict-float-cast-overflow. Code sanitizers can be used to detect affected patterns. The clang option for detecting this problem alone is -fsanitize=float-cast-overflow:

Does that have any bearing on this issue?

bstrie on 19 Sep 2018

We shouldn't care what LLVM does for overflowing casts, as long as it isn't unsafe undefined behavior. The result can be garbage as long as it can't cause unsound behavior.

retep998 on 19 Sep 2018

Does that have any bearing on this issue?

Not really. The UB did not change, LLVM got even more aggressive about exploiting it, which makes it easier to be affected by it in practice, but the soundness issue is unchanged. In particular, the new attribute does not remove the UB or affect any optimizations that existed before LLVM 7.

hanna-kruppe on 19 Sep 2018

@rkruppe out of curiosity, has this sort of fallen by the wayside? It seems like https://internals.rust-lang.org/t/help-us-benchmark-saturating-float-casts/6231/14 went well enough and the implementation hasn't had too many bugs. It seems that a slight performance regression was always expected, but to compile correctly seems like a worthwhile tradeoff.

Is this just waiting to be pushed across the finish line? Or are there other known blockers?

alexcrichton on 14 Oct 2018

Mostly I've been distracted / busy with other things, but a x0.82 regression in RBG JPEG encoding seems more than "slight", a rather bitter pill to swallow (although it's reassuring that other kinds of workload don't seem affected). It's not severe enough that I would object to turning saturation on by default, but enough that I'm hesitant to push for it myself before we've tried the "also provide a conversion function that's faster than saturation but may generate (safe) garbage" option discussed before. I haven't gotten to that, and apparently nobody else has either, so this has fallen by the wayside.

hanna-kruppe on 14 Oct 2018

Ok cool thanks for the update @rkruppe! I'm curious though if there's actually an implementation of the safe garbage option? I could imagine us easily providing something like unsafe fn i32::unchecked_from_f32(...) and such, but it sounds like you're thinking that should be a safe function. Is that possible with LLVM today?

alexcrichton on 17 Oct 2018

There's no freeze yet but it is possible to use inline assembly to access the target architecture's instruction for converting floats to integers (with a fallback to e.g. saturating as). While this can inhibit some optimizations, it may be good enough to mostly fix the regression in some benchmarks.

An unsafe function that keeps the UB that this issue is about (and is codegen'd in the same way as as is today) is another option, but a much less attractive one, I'd prefer a safe function if it can get the job done.

hanna-kruppe on 17 Oct 2018

There's also significant room for improvement in the safe saturating float-to-int sequence. LLVM today doesn't have anything specifically for this, but if inline asm solutions are on the table, it wouldn't be difficult to do something like this:

     cvttsd2si %xmm0, %eax   # x86's cvttsd2si returns 0x80000000 on overflow and invalid cases
     cmp $1, %eax            # a compact way to test whether %eax is equal to 0x80000000
     jno ok
     ...  # slow path: check for and handle overflow and invalid cases
ok:

which should be significantly faster than what rustc currently does.

sunfishcode on 17 Oct 2018

Ok I just wanted to be sure to clarify, thanks! I figured that inline asm solutions aren't workable as defaults as it'd inhibit other optimizations too much, but I haven't tried out myself. I'd personally prefer that we close this unsound hole by defining some reasonable behavior (like exactly today's saturating casts). If necessary we can always preserve today's fast/unsound implementation as an unsafe function, and in the limit of time given infinite resources we can even drastically improve the default and/or add other specialized conversion functions (like a safe conversion where out of bounds isn't UB but just a garbage bit pattern)

Would others be opposed to such a strategy? Do we think that this isn't important enough to fix in the meantime?

alexcrichton on 17 Oct 2018

I think inline assembly should be tolerable for cvttsd2si (or similar instructions) specifically because that inline asm would not access memory or have side effects, so it's just an opaque black box that can be removed if unused and doesn't inhibit optimizations around it very much, LLVM just can't reason about the internals and result value of the inline asm. That last bit is why I would be skeptical about e.g. using inline asm for the code sequence @sunfishcode suggests for saturation: the checks introduced for saturation can occasionally be removed today if they're redundant, but the branches in an inline asm block can't be simplified.

Would others be opposed to such a strategy? Do we think that this isn't important enough to fix in the meantime?

I do not object to flipping on saturating now and possibly adding alternatives later, I just don't want to be the one who has to drum up the consensus for it and justify it to users whose code got slower 😅

hanna-kruppe on 17 Oct 2018

👍4

I've started some work to implement intrinsics for saturating float to int casts in LLVM: https://reviews.llvm.org/D54749

If that goes anywhere, it will provide a relatively low-overhead way of getting the saturating semantics.

nikic on 28 Nov 2018

❤25

How does one reproduce this undefined behavior? I tried the example in the comment but the result was 255, which seems OK to me:

println!("{}", 1.04E+17 as u8);

AaronM04 on 1 Mar 2019

Undefined behavior can't be reliably observed that way, sometimes it gives you what you expect but in more complex situations breaks down.

In short, the code generation engine (LLVM) we use is allowed to assume that this doesn't happen, and thus it may generate bad code if it ever relies on this assumption.

Manishearth on 1 Mar 2019

👍3

@AaronM04 an example of reproducible undefined behavior was posted on reddit today:

fn main() {
    let a = 360.0f32;
    println!("{}", a as u8);

    let a = 360.0f32 as u8;
    println!("{}", a);

    println!("{}", 360.0f32 as u8);
}

(see playground)

lovasoa on 15 Mar 2019

👍19 ❤2 👀1

I assume that last comment was meant for @AaronM04, in reference to their previous comment.

shepmaster on 16 Mar 2019

😄3 👍2

"Oh, that's easy enough then."

@pcwalton, 2014

Sorry, I've read very carefully all this 6-year history of good intentions. But, seriously, 6 long years out of 10!!! Had it been a politician forum one would have anticipated some blazing sabotage around here.

So, please, can anybody explain, in simple words, what does it make the process of looking for a solution more interesting than the solution itself?

popovdenys on 26 Jun 2019

👎2 😕1

Because it's harder than it seemed initially and needs LLVM changes.

Manishearth on 26 Jun 2019

👍1

Ok, but it was not God who has made this LLVM on his second week, and moving in the same direction it might take another 15 years to resolve this fundamental problem.

Really, I have no attention to hurt somebody, and I'm new to Rust infrastructure to help suddenly but when I've learned about this case I was just stunned.

popovdenys on 26 Jun 2019

This issue tracker is for discussing how to solve this problem, and stating the obvious makes no progress in that direction. So if you want to help solve the problem or have some new information to contribute, please do, but otherwise your comments are not going to magically make the fix appear. :)

RalfJung on 26 Jun 2019

❤5 👎1

I think the assumption that this requires changes in LLVM is premature.

I think we can do it in the language with mimimal performance cost. Would it be a breaking change ** yea but it could be done, and should be done.

My go to solution would be to define float to int casts as unsafe then provide some helper functions in the, standard lib to provide results bound in Result Types.

It's an unsexy fix and it's a breaking change, but ultimately it is what every developer has to code themselves to work around the existing UB already. This is the correct rust approach.

IGBC on 27 Jun 2019

❤1

Thank you, @RalfJung, to make me understand it. I had no intention to insult anybody or scornfully intervene in the productive brainstorming process. Being new in rust, it is true, there is not so much I can do. Nevertheless, it helps me and maybe others, who try to enter into rust, learn more about its unsolved flaws and make the relevant output: is it worth to dig deeper or it is better to choose something else for now. But I'm already glad that removal of "my useless comments" will be much easier.

popovdenys on 27 Jun 2019

As noted earlier in the thread, this is slowly but surely being fixed the Right Way by fixing llvm to support the necessary semantics, as the relevant teams have long ago agreed.

Nothing more can really be added to this discussion.

https://reviews.llvm.org/D54749

Gankra on 27 Jun 2019

@nikic Seems like progress on LLVM's side has stalled, could you give a brief update if possible? Thanks.

Centril on 6 Sep 2019

Can the saturating cast be implemented as a library function that users could opt into, if they are willing to take some pref regression to get soundess? I’m reading the compiler’s implementation but it seems quite subtle:

https://github.com/rust-lang/rust/blob/625451e376bb2e5283fc4741caa0a3e8a2ca4d54/src/librustc_codegen_ssa/mir/rvalue.rs#L774-L901

SimonSapin on 28 Oct 2019

👍1

We could expose an intrinsic that generates the LLVM IR for saturation (whether that be the current open-coded IR or llvm.fpto[su]i.sat in the future) independent of the -Z flag. That is not difficult at all to do.

However, I'm concerned whether it's the best course of action. When (if?) saturation becomes the default semantics of as casts, such an API becomes redundant. It also seems ungreat to tell users that they should choose for themselves whether they want soundness or performance, even if it's only temporary.

At the same time, the current situation is clearly even worse. If we're thinking about adding library APIs, I'm warning up more and more to just enable saturation by default and offer an unsafe intrinsics that has UB on NaN and out-of-range numbers (and lowers to a plain fpto[su]i). That would still offer basically the same choice, but defaulting to soundness, and the new API would likely not become redundant in the future.

hanna-kruppe on 28 Oct 2019

👍5

Switching to sound by default sounds good. I think we can offer the intrinsic lazily upon request rather than from the get-go. Also, will const eval do the saturation as well in this case? (cc @RalfJung @eddyb @oli-obk)

Centril on 28 Oct 2019

Const eval us already doing saturation and has done so for ages, I think even before miri (I distinctly remember changing it in the old llvm::Constant-based evaluator).

hanna-kruppe on 28 Oct 2019

@rkruppe Awesome! Since you are familiar with the code in question, would you like to spearhead switching the defaults?

Centril on 28 Oct 2019

@rkruppe

We could expose an intrinsic that generates the LLVM IR for saturation

This may need to be 10 or 12 separate intrinsics, for each combination of source and destination type.

SimonSapin on 29 Oct 2019

@Centril

Switching to sound by default sounds good. I think we can offer the intrinsic lazily upon request rather than from the get-go.

I assume that unlike in other comments, “the intrinsic” in your comment means something that would have less pref regression when as does saturation.

I don’t think this is a good approach to deal with known significant regressions. For some users the performance loss might be a real problem, while their algorithm ensure the input is always in range. If they’re not subscribed to this thread they might only realize they are affected when the change reaches the Stable channel. At that point they might be stuck for 6 to 12 weeks even if we land an unsafe API immediately upon request.

I’d much rather we follow the pattern already established for deprecation warnings: only make the switch in Nightly after the alternative has been available in Stable for some time.

SimonSapin on 29 Oct 2019

This may need to be 10 or 12 separate intrinsics, for each combination of source and destination type.

Fine, you got me, but I don't see how that's relevant? Let it be 30 intrinsics, it's stilll trivial to add them. But in reality, it's even easier to have a single generic intrinsic used by N thin wrappers. The number also doesn't change if we choose the "make as sound and introduce an unsafe cast API" option.

I don’t think this is a good approach to deal with _known_ significant regressions. For some users the performance loss might be a real problem, while their algorithm ensure the input is always in range. If they’re not subscribed to this thread they might only realize they are affected when the change reaches the Stable channel. At that point they might be stuck for 6 to 12 weeks even if we land an unsafe API immediately upon request.

+1

I'm not sure whether the procedure for deprecation warnings (only deprecate on nightly once the replacement is stable) is necessary since it seems less important to remain perf-regression-free across all release channels than to remain warning-free across all release channels, but then again, waiting 12 more weeks is basically a rounding error with how long this issue has been around.

We can also leave the -Zsaturating-float-casts around (just changing the default) which means any nightly users can still opt out of the cange for a while.

hanna-kruppe on 29 Oct 2019

(Yes the number of intrinsics is just an implementation detail and was not meant as an argument for or against anything.)

SimonSapin on 29 Oct 2019

@rkruppe I cannot claim to have digested all of the comments here, but I am under the impression that LLVM now does have a freeze instruction, which was the item blocking the "shortest path" to eliminating UB here, right?

Though I guess freeze is so new that it may not be available in our own version of LLVM, right? Still, seems like something we should be exploring development on, perhaps during the first half of 2020?

Nominating for discussion at T-compiler meeting, to try to get rough consensus on our desired path at this point.

pnkfelix on 21 Nov 2019

Using freeze is still problematic though for all the reasons referenced here. I am not sure how realistic such concerns are with using freeze for these casts, but in principle they apply. Basically, expect freeze to return either random garbage or your secret key, whatever is worse. (I read this online somewhere and really like it has a summary. :D )

And anyway, even returning random garbage seems rather bad for an as cast. It makes sense to have faster operations for speed where needed, similar to unchecked_add, but making that the default seems quite strongly against Rust's spirit.

@SimonSapin you proposed the opposite approach first (default to unsound/"weird" semantics and provide an explicitly sound method); I cannot tell from your later comments whether you think defaulting to soundness (after a suitable transition period) is also reasonable / better?

RalfJung on 21 Nov 2019

👍2

@pnkfelix

I am under the impression that LLVM now does have a freeze instruction, which was the item blocking the "shortest path" to eliminating UB here, right?

There are some caveats. Most importantly, even if all we care about is getting rid of UB and we update our bundled LLVM to include freeze (which we could do at any time), we support several older versions (back to LLVM 6 at the moment) and we'd need some fallback implementation for those to actually get rid of the UB for all users.

Second, of course, is the question whether "just not UB" is all we care about while we're at it. In particular, I want to highlight again that a freeze(fptosi %x) behaves extremely counter-intuitively: it's non-deterministic and can return a different result (even one taken from sensitive memory as @RalfJung said) every time it's executed. I don't want to go into debating this again now but it's worth considering in the meeting if we'd rather do a little more work to make saturation the default and unchecked (either unsafe or freeze-using) conversions the non-default option.

hanna-kruppe on 21 Nov 2019

@RalfJung My position is that as is best avoided entirely regardless of this issue, because it can have wildly different semantics (truncating, saturating, rounding, …) depending on the input and output type, and those are not always obvious when reading code. (Even the latter can be inferred with foo as _.) So I have a draft pre-RFC for proposing various explicitly-named conversion methods that cover the cases that as does today (and maybe more).

I think as should definitely not have UB, since it can be used outside of unsafe. Returning garbage doesn’t sound good either. But we should probably have some kind of mitigation/transition/alternative for known cases of performance regression caused by saturating cast. I only asked about a library implementation of saturating cast in order to not block this draft RFC on that transition.

SimonSapin on 21 Nov 2019

👍1

@SimonSapin

My position is that as is best avoided entirely regardless of this issue, because it can have wildly different semantics (truncating, saturating, rounding, …)

Agreed. But that doesn't really help us with this issue.

(Also, I am happy that you are working towards making as not needed. Looking forward to that. :D )

I think as should definitely not have UB, since it can be used outside of unsafe. Returning garbage doesn’t sound good either. But we should probably have some kind of mitigation/transition/alternative for known cases of performance regression caused by saturating cast. I only asked about a library implementation of saturating cast in order to not block this draft RFC on that transition.

So we seem to agree that the final state should be that float-to-int as saturates? I am happy with any transition plan as long as that is the end goal we are moving towards.

RalfJung on 21 Nov 2019

That end goal sounds good to me.

SimonSapin on 21 Nov 2019

I don’t think this is a good approach to deal with _known_ significant regressions. For some users the performance loss might be a real problem, while their algorithm ensure the input is always in range. If they’re not subscribed to this thread they might only realize they are affected when the change reaches the Stable channel. At that point they might be stuck for 6 to 12 weeks even if we land an unsafe API immediately upon request.

In my view, it would not be the end of the world if those users wait with upgrading their rustc for those 6-12 weeks -- they might not need anything from the upcoming releases in either case, or their libraries may have MSRV constraints to uphold.

Meanwhile, users, who are also not subscribed to the thread might be getting miscompilations just as they might be getting performance losses. Which should we prioritize? We give guarantees about stability and we give guarantees about safety -- but to my knowledge, no such guarantees are given about performance (e.g. RFC 1122 doesn't mention perf at all).

I’d much rather we follow the pattern already established for deprecation warnings: only make the switch in Nightly after the alternative has been available in Stable for some time.

In the case of deprecation warnings, the consequence of waiting with the deprecation until there's a stable alternative is not, at least as far as I know, soundness holes during the waiting period. (Also, while intrinsics can be provided here, in the general case, we might not be able to reasonably offer alternatives when fixing soundness holes. So I don't think having alternatives on stable can be a hard requirement.)

Fine, you got me, but I don't see how that's relevant? Let it be 30 intrinsics, it's stilll trivial to add them. But in reality, it's even easier to have a single generic intrinsic used by N thin wrappers. The number also doesn't change if we choose the "make as sound and introduce an unsafe cast API" option.

Won't that single generic intrinsic requires separate implementations in the compiler for those 12/30 specific monomorphic instantiations?

It might be trivial to add intrinsics to the compiler, because LLVM has already done most of the work, but that is also far from the full cost. In addition, there's the implementation in Miri, Cranelift, as well as the eventual work needed in a specification. So I don't think we should add intrinsics on the off-chance someone needs them.

I am however not opposed to exposing more intrinsics, but if someone needs them, they should make a proposal (e.g. as a PR with some elaborate description), and justify the addition with some benchmarking numbers or some such. I don't think that should block fixing the soundness hole meanwhile.

We can also leave the -Zsaturating-float-casts around (just changing the default) which means any nightly users can still opt out of the cange for a while.

This seems fine to me, but I would suggest renaming the flag to -Zunsaturating-float-casts to avoid changing semantics towards unsoundness for those who already use this flag.

Centril on 21 Nov 2019

👍1

@Centril

Won't that single generic intrinsic requires separate implementations in the compiler for those 12/30 specific monomorphic instantiations?

No, most of the implementation can be and already is shared by parametrizing over the source and destination bit widths. Only a few bits need case distinctions. The same applies to the implementaton in miri and most likely also to other implementations and to specifications.

(Edit: to be clear, this sharing can happen even if there's N distinct intrinsics, but a single generic intrinsic cuts down on the boilerplate required per-intrinsic.)

So I don't think we should add intrinsics on the off-chance someone needs them.

I am however not opposed to exposing more intrinsics, but if someone needs them, they should make a proposal (e.g. as a PR with some elaborate description), and justify the addition with some benchmarking numbers or some such. I don't think that should block fixing the soundness hole meanwhile.

We already have some benchmarking numbers. We know from the call for benchmarks long ago that JPEG encoding gets significantly slower on x86_64 with saturating casts. Someone could re-run those but I feel confident predicting that this hasn't changed (although of course the specific numbers won't be identical) and see no reason why future changes to how saturation is implemented (like switching to inline asm or the LLVM intrinsics @nikic worked on) would fundamentally change that. While it's hard to be sure about the future, my educated guess is that the only plausible way to get that performance back is to use something that generates code without range checks, like an unsafe conversion or something using freeze.

hanna-kruppe on 21 Nov 2019

👍1

OK so from the existing benchmarking numbers, it seems like there is an active desire for said intrinsics. If so, I would propose the following action plan:

Concurrently:
- Introduce the intrinsics exposed on nightly through #[unstable(...)] functions.
- Remove -Zsaturating-float-casts and introduce -Zunsaturating-float-casts.
- Switch the default to what -Zsaturating-float-casts does.
We stabilize the intrinsics after some time; we can fast-track a bit.
Remove -Zunsaturating-float-casts after a while.

Centril on 21 Nov 2019

👍3

Sounds good. Except that intrinsics are implementation details of some public API, probably methods on f32 and f64. They could be either:

Methods of a generic trait (with a parameter for the conversion’s integer return type), optionally in the prelude
Inherent methods with a supporting trait (similar to str::parse and FromStr) in order to support different return types
Multiple non-generic inherent methods with the target type in the name

SimonSapin on 21 Nov 2019

👍2

Yea, I meant exposing the intrinsics via methods or some such.

Multiple non-generic inherent methods with the target type in the name

This feels like the usual thing we do - any objections to this option?

Centril on 21 Nov 2019

👍1

Is it though? I feel that when have the name of a type (of the signature) as part of the name of a method it’s ad-hoc “one of a kind” conversions (like Vec::as_slice and [T]::to_vec), or a series of conversions where the difference is not a type (like to_ne_bytes, to_be_bytes, to_le_bytes). But part of the motivation for std::convert’s traits was to avoid dozens of separate methods like u8::to_u16, u8::to_u32, u8::to_u64, etc.

SimonSapin on 21 Nov 2019

👍1

My wonder is whether this would be naturally generalizable to a trait given that the methods need to be unsafe fn. If we do add inherent methods, then you can always delegate to those in trait implementations and whatnot.

Centril on 21 Nov 2019

It does seem odd to me to add traits for unsafe conversions, but I guess probably Simon is thinking about the fact that we'd possibly need a different method for each combination of floating point and integer type (e.g. f32::to_u8_unsaturated, f32::to_u16_unsaturated, etc).

Not to weigh in on a long thread I haven't read in total ignorance, but is that desired or is it sufficient to have e.g. f32::to_integer_unsaturated which converts to u32or something? Is there an obvious choice for the target type for the unsafe conversion?

withoutboats on 21 Nov 2019

Providing unsafe conversions only to i32/u32 (for example) completely excludes all integer types whose value range isn't strictly smaller, and that's definitely sometimes needed. Going smaller (down to u8, as in JPEG encoding) is also often needed but can be emulated by converting to a wider integer type and truncating with as (which is cheap though not usually free).

But we can't very well only provide conversion to the largest integer size. Those are not always natively supported (hence, slow) and optimizations can't fix that: it's unsound to optimize "convert to large int, then truncate" into "convert to smaller int directly" because the latter has UB (in LLVM IR) / different results (on machine code level, on most architectures) in the cases where the original conversion result would have wrapped around when truncating.

Note that even pragmatically excluding 128 bit integers and focusing on 64 bit integers will still be bad for common 32 bit targets.

hanna-kruppe on 21 Nov 2019

👍1

I'm new to this conversation but not to programming. I'm curious as to why people think saturating conversions and converting NaN to zero are reasonable default behaviors. I understand that Java does this (although wrapping around seems much more common), but there is no integer value for which NaN can really be said to be a correct conversion. Similarly, converting 1000000.0 into 65535 (u16), for example, seems wrong. There is simply no u16 that's clearly the right answer. At least, I don't see it as being any better than the current behavior of converting it to 16960, which is at least a behavior shared with C/C++, C#, go, and others, and thus at least somewhat unsurprising.

Various people have commented on the similarity with overflow checking, and I agree with them. It's also similar to integer division by zero. I think invalid conversions should panic just like invalid arithmetic. Relying on NaN -> 0 and 1000000.0 -> 65535 (or 16960) seems just as error-prone as relying on integer overflow or a hypothetical n / 0 == 0. It's the kind of thing that should produce an error by default. (In release builds, rust can elide the error checking, just as it does with integer arithmetic.) And in the rare cases when you _want_ to convert NaN to zero or have floating point saturation, you should have to opt into it, just like you have to opt into integer overflow.

As for performance, it seems like the highest general performance would come from doing a plain conversion and relying on hardware faults. Both x86 and ARM, for example, raise hardware exceptions when a floating-point-to-integer conversion cannot be represented correctly (including both NaN and out-of-range cases). This solution is zero-cost except for invalid conversions, except when converting directly from floating-point to small integer types in debug builds - a rare case - where it should still be comparatively cheap. (On theoretical hardware that doesn't support these exceptions, then it can be emulated in software, but again only in debug builds.) I imagine that hardware exceptions are exactly how detecting integer division by zero is implemented today. I saw a lot of talk of LLVM, so maybe you're constrained here, but it'd be unfortunate to have software emulation in every floating point conversion even in release builds in order to provide dubious alternative behaviors for inherently invalid conversions.

admilazz on 28 Nov 2019

👍2

@admilazz We are constrained by what LLVM can do, and currently LLVM does not expose a method to efficiently convert floats to integers without the risk of undefined behavior.

The saturation is because the language defines as casts to always succeed, and so we can't change the operator to panic instead.

lambda-fairy on 28 Nov 2019

👍2

Similarly, converting 1000000.0 into 65535 (u16), for example, seems wrong. There is simply no u16 that's clearly the right answer. At least, I don't see it as being any better than the current behavior of converting it to 16960,

It wasn’t obvious to me so I think it’s worth pointing out: 16960 is the result of converting 1000000.0 to a sufficently-wide integer, then truncating to keep the 16 lower bits.

This is ~not an option that has been suggested before in this thread, and it’s~ (Edit: I was wrong here, sorry I didn’t find it) not the current behavior either. The current behavior in Rust is that out-of-range float-to-integer conversion is Undefined Behavior. In practice this often leads to a garbage value, on principle it could cause miscompilations and vulnerabilities. This thread is all about fixing that. When I run the program below in Rust 1.39.0 I get a different value every time:

fn main() {
    dbg!(1000000.0 as u16);
}

Playground. Example output:

[src/main.rs:2] 1000000.0 as u16 = 49072

I personally think integer-like truncation is no better or worse than saturation, they’re both numerically wrong for out-of-range values. An infallible conversion has its place, as long as it’s deterministic and not UB. You might already know from your algorithm that values are in range, or might not care about such cases.

I think we should also add fallible conversion APIs that return a Result, but I still need to finish writing that draft pre-RFC :)

SimonSapin on 28 Nov 2019

👍1

The "convert to mathematical integer, then truncate to target width" or "wraparound" semantics have been suggested before in this thread (https://github.com/rust-lang/rust/issues/10184#issuecomment-299229143). I don't particularly like it:

I think it's slightly less sensible than saturation. Saturation doesn't generally gives sensible results for numbers far out of range, but:
- it behaves more sensibly than wraparound when numbers are slightly out of range (e.g. due to accumulated rounding error). In contrast, a cast that wraps around can amplify a mild rounding error in the float computation to the maximal possible error in the integer domain.
- it's somewhat commonly used in digital signal processing, so there are at least some applications where it is actually desired. In contrast, I do not know of a single algorithm that benefits from wraparound semantics.
AFAIK the only reason to prefer wraparound semantics is the efficency of software emulation, but this seems like an unproven assumption to me. I'd be happy to be proven wrong but at a cursory glance wraparound seem to require such a long chain of ALU instructions (plus branches to handle infinities and NaNs separately) that I don't feel like it's clear one will be clearly better for performance than the other.
While the question of what to do for NaN is an ugly problem for any conversion to integer, saturation at least does not require any special casing (neither in semantics nor in most implementations) for infinity. But for wraparound, what's the integer equivalent is +/- infinity supposed to be? JavaScript says it's 0, and I suppose if we made as panic on NaN then it could also panic on infinity, but either way this seems like it'll make wraparound harder to make fast than looking at normal and denormal numbers alone would suggest.

hanna-kruppe on 28 Nov 2019

👍2

I suspect that most of the code regressed by the saturation semantics for conversion would be better off using SIMD. So while unfortunate, this change won't prevent high-performance code from being written (especially if intrinsic with different semantics are provided), and might even nudge some projects towards a faster (if less portable) implementation.

If so, some slight performance regressions shouldn't be used as justification to avoid closing a soundness hole.

lnicola on 28 Nov 2019

https://github.com/rust-lang/rust/pull/66841 adds unsafe fn methods that convert with LLVM’s fptoui and fptosi, for those cases where values are known to be in range and saturating is a measurable performance regression.

After that lands I think it’s fine to switch the default for as (and perhaps add another -Z flag to opt out?), although that should probably be a formal Lang team decision.

SimonSapin on 28 Nov 2019

After that lands I think it’s fine to switch the default for as (and perhaps add another -Z flag to opt out?), although that should probably be a formal Lang team decision.

So we (language team, with the people that were there at least) discussed this in https://github.com/rust-lang/lang-team/blob/master/minutes/2019-11-21.md and we thought adding new intrinsics + adding -Zunsaturated-float-casts would be good first steps.

I think it would be good to switch the default as part of that or shortly after, possibly with FCP if necessary.

Centril on 28 Nov 2019

👍2

I assume that by new intrinsics you mean something like https://github.com/rust-lang/rust/pull/66841

What does it mean to add -Z unsaturated-float-casts without changing the default? Accept it as no-op rather than emitting "error: unknown debugging option"?

SimonSapin on 28 Nov 2019

I assume that by new intrinsics you mean something like #66841

Yep 👍 -- thanks for spearheading that.

What does it mean to add -Z unsaturated-float-casts without changing the default? Accept it as no-op rather than emitting "error: unknown debugging option"?

Yeah basically. Alternatively, we remove -Z saturated-float-casts in favor of -Z unsaturated-float-casts and switch the default directly, but it should lead to the same result over fewer PRs.

Centril on 28 Nov 2019

I really don't understand the "unsaturated" suggestion. If the goal is just to provide a knob to opt out of the new default, it's easier to just change the default of the existing flag and do nothing more. If the goal is to pick a new name that is more clear about the trade-off (unsoundness), then "unsaturating" is awful at that — I'd instead suggest a name that includes "unsafe" or "UB" or a similarly scary word, for example -Z fix-float-cast-ub.

hanna-kruppe on 28 Nov 2019

unchecked is the term with some precedent in API names.

SimonSapin on 28 Nov 2019

@admilazz We are constrained by what LLVM can do, and currently LLVM does not expose a method to efficiently convert floats to integers without the risk of undefined behavior.

But presumably you can add runtime checks only in debug builds, as you do for integer overflow.

AFAIK the only reason to prefer wraparound semantics is the efficency of software emulation

I don't think we should prefer either wraparound or saturation, since both are wrong, but wraparound at least has the benefit of being the method used by many languages similar to rust: C/C++, C#, go, probably D, and surely more, and of also being rust's current behavior (at least sometimes). That said, I think "panic on invalid conversions (possibly in debug builds only)" is ideal, just like we do for integer overflow and invalid arithmetic like division by zero.

(Interestingly, I did get 16960 in the playground. But I see from other examples posted that sometimes rust does it differently...)

The saturation is because the language defines as casts to always succeed, and so we can't change the operator to panic instead.

Changing what the operation evaluates to is already a breaking change, insofar as we care about the results of people already doing this. This no-panic behavior could change too.

I suppose if we made as panic on NaN then it could also panic on infinity, but either way this seems like it'll make wraparound harder to make fast

If it's only checked in debug builds, as integer overflow is, then I think we can get the best of both worlds: conversions are guaranteed to be correct (in debug builds), user errors are more likely to be caught, you can opt in to weird behaviors like wraparound and/or saturation if you like, and performance is as good as it can be.

Also, it seems strange to control this stuff via a command-line switch. That's a big hammer. Surely the desired behavior of an out-of-range conversion depends on the specifics of the algorithm, so it's something that should be controlled on a per-conversion basis. I'd suggest f.to_u16_sat() and f.to_u16_wrap() or similar as the opt-ins, and not having any command-line option that changes the semantics of code. That would make it hard to mix and match different pieces of code, and you can't understand what something does by reading it...

And, if it's truly unacceptable to make "panic if invalid" the default behavior, it would be nice to have an intrinsic method that implements it but only performs the validity check in debug builds so we can ensure our conversions are correct in the (vast majority of?) cases when we expect to get the same number after the conversion, but without paying any penalty in release builds.

admilazz on 28 Nov 2019

Interestingly, I did get 16960 in the playground.

This is how Undefined Behavior works: depending on the exact formulation of the program and the exact compiler version and the exact compilation flags, you might get deterministic behavior, or a garbage value that changes on every run, or miscompilations. The compiler is allowed to do anything.

wraparound at least has the benefit of being the method used by many languages similar to rust: C/C++, C#, go, probably D, and surely more,

Does it really? At least not in C and C++, they have the same Undefined Behavior as Rust. This is not a coincidence, we use LLVM which is primarily built for clang implementing C and C++. Are you sure about C# and go?

C11 standard https://port70.net/~nsz/c/c11/n1570.html#6.3.1.4

When a finite value of real floating type is converted to an integer type other than _Bool, the fractional part is discarded (i.e., the value is truncated toward zero). If the value of the integral part cannot be represented by the integer type, the behavior is undefined.

The remaindering operation performed when a value of integer type is converted to unsigned type need not be performed when a value of real floating type is converted to unsigned type. Thus, the range of portable real floating values is (-1, Utype_MAX+1).

C++17 standard http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4659.pdf#section.7.10

A prvalue of a floating-point type can be converted to a prvalue of an integer type. The conversion truncates;that is, the fractional part is discarded. The behavior is undefined if the truncated value cannot be represented in the destination type.

SimonSapin on 28 Nov 2019

👍2

C# reference https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/numeric-conversions

When you convert a double or float value to an integral type, this value is rounded towards zero to the nearest integral value. If the resulting integral value is outside the range of the destination type, the result depends on the overflow checking context. In a checked context, an OverflowException is thrown, while in an unchecked context, the result is an unspecified value of the destination type.

So it's not UB, just an "unspecified value".

Amanieu on 28 Nov 2019

@admilazz There's a huge difference between this and integer overflow: integer overflow is undesirable but well defined. Floating point casts are undefined behavior.

What you are asking for is similar to turning off Vec bounds checking in release mode, but that would be wrong because that would allow for undefined behavior.

Allowing undefined behavior in safe code is not acceptable, even if it only happens in release mode. So any fix must apply to both release and debug mode.

Of course it's possible to have a more restrictive fix in debug mode, but the fix for release mode must still be well defined.

Pauan on 29 Nov 2019

👍2

@admilazz There's a huge difference between this and integer overflow: integer overflow is undesirable but well defined. Floating point casts are undefined behavior.

Sure, but this thread is about defining the behavior. If it was defined as producing "an unspecified value of the destination type", as in the C# spec that Amanieu helpfully referenced above, then it would no longer be undefined (in any dangerous way). You can't easily make use of the well-defined nature of integer overflow in practical programs because it will still panic in debug builds. Similarly, the value produced by an invalid cast in release builds needn't be predictable or particularly useful because programs couldn't practically make use of it anyway if it panicked in debug builds. This actually gives maximum scope to the compiler for optimizations, whereas picking a behavior like saturation constrains the compiler and could be significantly slower on hardware without native saturating conversion instructions. (And it's not like saturation is clearly correct.)

What you are asking for is similar to turning off Vec bounds checking in release mode, but that would be wrong because that would allow for undefined behavior. Allowing undefined behavior in safe code is not acceptable...

Not all undefined behavior is alike. Undefined behavior just means it's up to the compiler implementer to decide what happens. As long as there is no way to violate rust's safety guarantees by casting a float to an int, then I don't think it's similar to allowing people to write to arbitrary memory locations. Nonetheless, of course I agree that it should be defined in the sense of being guaranteed safe even if not necessarily predictable.

Does it really? At least not in C and C++, they have the same Undefined Behavior as Rust... Are you sure about C# and go?

Fair enough. I did not read all their specs; I just tested various compilers. You're right that saying "all the compilers I tried do it this way" is different from saying "the language specifications define it to be this way". But I'm not arguing in favor of overflow anyway, only pointing out that it seems to be the most common. I'm really arguing in favor of having a conversion that 1) protects against "wrong" results like 1000000.0 becoming 65535 or 16960 for the same reason that we protect against integer overflow - it's most likely a bug so users should have to opt into it, and 2) allows maximum performance in release builds.

admilazz on 29 Nov 2019

Not all undefined behavior is alike. Undefined behavior just means it's up to the compiler implementer to decide what happens. As long as there is no way to violate rust's safety guarantees by casting a float to an int, then I don't think it's similar to allowing people to write to arbitrary memory locations. Nonetheless, of course I agree that it should be defined: defined, but not necessarily predictable.

Undefined behaviour means the optimizers (which are provided by LLVM developers focused on C and C++) are free to assume it can never happen and transform the code based on that assumption, including deleting chunks of code which are only reachable by passing through the undefined cast or, as this example shows, assuming that an assignment must have been called, even though it actually wasn't, because invoking code that is called without first calling it would be undefined behaviour.

Even if it were reasonable to prove that composing the different optimization passes doesn't produce dangerous emergent behaviours, the LLVM developers won't make any conscious effort to preserve that.

I'd argue that all undefined behaviour is alike on that basis.

ssokolow on 29 Nov 2019

👍2

Even if it were reasonable to prove that composing the different optimization passes doesn't produce dangerous emergent behaviours, the LLVM developers won't make any conscious effort to preserve that.

Well, it's unfortunate that LLVM impinges on rust's design in this way, but I did just read some of the LLVM instruction reference and it mentions the "freeze" operation mentioned above ("... another is to wait for LLVM to add a freeze concept …") which would prevent undefined behavior at the LLVM level. Is rust tied to an old version of LLVM? If not, we could use it. Their documentation is unclear about the exact behavior, though.

If the argument is undef or poison, ‘freeze’ returns an arbitrary, but fixed, value of type ‘ty’. Otherwise, this instruction is a no-op and returns the input argument. All uses of a value returned by the same ‘freeze’ instruction are guaranteed to always observe the same value, while different ‘freeze’ instructions may yield different values.

I don't know what they mean by "fixed value" or "the same 'freeze' instruction". I think ideally it would compile to a no-op and give an unpredictable integer, but it sounds like it might possibly do something expensive. Has anyone tried this freeze operation?

admilazz on 29 Nov 2019

Well, it's unfortunate that LLVM impinges on rust's design in this way

It's not just that LLVM developers write the optimizers. It's that, even if the rustc developers wrote the optimizers, flirting with undefinedness is inherently a huge footgun because of the emergent properties of chaining up optimizers. The human brain simply didn't evolve to "intuit the potential magnitude of the rounding error" when the rounding in question is emergent behaviour built up by chaining optimization passes.

ssokolow on 29 Nov 2019

I'm not gonna disagree with you there. :-) I do hope this LLVM "freeze" instruction provides a zero-cost way to avoid this undefined behavior.

admilazz on 29 Nov 2019

That was discussed above and the conclusion was that while casting-then-freezing is defined behavior, it is not at all reasonable behavior. In release mode, such casts would return arbitrary results for out-of-bound inputs (in entirely safe code). That's not a good semantics for something as innocent-looking as as.

IMO such a semantics would be bad language design that we would rather avoid.

RalfJung on 29 Nov 2019

👍4

My position is that as is best avoided entirely regardless of this issue, because it can have wildly different semantics (truncating, saturating, rounding, …) depending on the input and output type, and those are not always obvious when reading code. (Even the latter can be inferred with foo as _.) So I have a draft pre-RFC for proposing various explicitly-named conversion methods that cover the cases that as does today (and maybe more).

I finished that draft! https://internals.rust-lang.org/t/pre-rfc-add-explicitly-named-numeric-conversion-apis/11395

Any feedback is very welcome, but please give it in the internals threads rather than here.

SimonSapin on 29 Nov 2019

In release mode, such casts would return arbitrary results for out-of-bound inputs (in entirely safe code). That's not a good semantics for something as innocent-looking as as.

Sorry to repeat myself, but I think this same argument applies to integer overflow. If you multiply some numbers and the result overflows, you're going to get a wildly wrong result that will almost certainly invalidate the calculation you were trying to perform, but it panics in debug builds and therefore the bug is likely to be caught. I would say that a numeric conversion that gives wildly wrong results should also panic because there's a very high chance it represents a bug in the user's code. (The case of typical floating-point inaccuracy is already handled. If a calculation produces 65535.3 it's already valid to convert that to u16. To get an out-of-bounds conversion you usually need a bug in your code, and if I have a bug I want to be notified so I can fix it.)

The ability for release builds to give arbitrary but defined results for invalid conversions also allows maximum performance, which is important, in my opinion, for something as fundamental as numeric conversions. Always saturating has a significant performance impact, hides bugs, and rarely makes a calculation that unexpectedly encounters it give the right result.

admilazz on 29 Nov 2019

Sorry to repeat myself, but I think this same argument applies to integer overflow. If you multiply some numbers and the result overflows, you're going to get a wildly wrong result that will almost certainly invalidate the calculation you were trying to perform

We are not talking about multiplication though, we are talking about casts. And yes, the same applies to integer overflow: int-to-int casts never panic, even when they overflow. This is because as, by design, never panics, not even in debug builds. Deviating from this for floating point casts is surprising at best and dangerous at worst, as the correctness and safety on unsafe code can depend on certain operations not panicking.

If you want to argue that the design of as is flawed because it provides infallible conversion between types where proper conversion is not always possible, I think most of us will agree. But that is entirely out of scope for this thread, which is about fixing float-to-int conversions inside the existing framework of as casts. These have to be infallible, they must not panic, not even in debug builds. So please either propose some reasonable (not involving freeze), non-panicking semantics for float-to-int casts, or else try to start a new discussion about re-designing as to permit panicking when the cast is lossy (and do so consistently for int-to-int and float-to-int casts) -- but the latter is off-topic in this issue, so please open a new thread (pre-RFC-style) for that.

RalfJung on 29 Nov 2019

👍4

How about we start by just implementing freeze semantics now to fix the UB, and then we can have all the time in the world to agree on what semantics we actually want since any semantics we choose will be backwards compatible with freeze semantics.

retep998 on 29 Nov 2019

👍3

How about we start by just implementing freeze semantics _now_ to fix the UB, and then we can have all the time in the world to agree on what semantics we actually want since any semantics we choose will be backwards compatible with freeze semantics.

Panicking is not backwards compatible with freezing, so we'd need to reject at least all proposals that involve panicking. Moving from UB to panicking is less obviously incompatible, though as discussed above there are some other reasons to not make as panic.
As I wrote before,
> we support several older versions (back to LLVM 6 at the moment) and we'd need some fallback implementation for those to actually get rid of the UB for all users.

I agree with @RalfJung that making only some as casts panicking is highly undesirable, but that aside I don't think this point @admilazz made is obviously correct:

(The case of typical floating-point inaccuracy is already handled. If a calculation produces 65535.3 it's already valid to convert that to u16. To get an out-of-bounds conversion you usually need a bug in your code, and if I have a bug I want to be notified so I can fix it.)

For f32->u16 it may be true that you need extraordinarily large rounding error to drop out of the u16 range just from rounding error, but for conversions from f32 to 32 bit integers that is not so obviously true. i32::MAX is not representable exactly in f32, the closest representable number is 47 off from i32::MAX. So if you have a calculation that should mathematically result in a number up to i32::MAX, any error >= 1 ULP away from zero will put you out of bounds. And it gets much worse once we consider lower precision floats (IEEE 754 binary16, or the non-standard bfloat16).

hanna-kruppe on 30 Nov 2019

We are not talking about multiplication though, we are talking about casts

Well, floating-point to integer conversions are used almost exclusively in the same context as multiplication: numerical calculations, and I do think there's a useful parallel with the behavior of integer overflow.

And yes, the same applies to integer overflow: int-to-int casts never panic, even when they overflow... Deviating from this for floating point casts is surprising at best and dangerous at worst, as the correctness and safety on unsafe code can depend on certain operations not panicking.

I would argue that the inconsistency here is justified by common practice and wouldn't be so surprising. Truncating and chopping up integers with shifts, masks, and casts - effectively using casts as a form of bitwise AND plus a size change - is very common and has a long history in systems programming. It's something I do several times a week at least. But over the past 30+ years, I can't recall ever expecting to get a reasonable result out of converting NaN, Infinity, or an out-of-range floating-point value to integer. (Every instance of that I can recall has been a bug in the calculation that produced the value.) So I don't think the case of integer -> integer casts and floating-point -> integer casts must be treated identically. That said, I can understand that some decisions have already been set in stone.

please … propose some reasonable (not involving freeze), non-panicking semantics for float-to-int casts

Well, my proposal is:

Don't use global compilation switches that effect significant changes to semantics. (I assume -Zsaturating-float-casts is a command-line parameter or similar.) Code that depends on saturation behavior, say, would be broken if compiled without it. Presumably code with different expectations couldn't be mixed together in the same project. There should be some local way for a calculation to specify the desired semantics, probably something like this pre-RFC.
Make as casts have maximum performance by default, as would be expected from a cast.
- I think this should be done via freeze on LLVM versions that support it and any other conversion semantics on LLVM versions that don't (e.g. truncation, saturation, etc). I expect the 'freeze could leak values from sensitive memory' claim is purely hypothetical. (Or, if y = freeze(fptosi(x)) merely leaves y unchanged, thus leaking uninitialized memory, that could be fixed by clearing y first.)
- If as will be relatively slow by default (e.g. because it saturates), provide some way to obtain maximum performance (e.g. a method - unsafe if necessary - that uses freeze).

admilazz on 3 Dec 2019

❤1

Don't use global compilation switches that effect significant changes to semantics. (I assume -Zsaturating-float-casts is a command-line parameter or similar.)

To be clear, I don't think anyone disagrees. This flag was only ever proposed as a short-term tool to more easily measure and work around performance regressions while libraries are getting updated to fix those regressions.

hanna-kruppe on 3 Dec 2019

For f32->u16 it may be true that you need extraordinarily large rounding error to drop out of the u16 range just from rounding error, but for conversions from f32 to 32 bit integers that is not so obviously true. i32::MAX is not representable exactly in f32, the closest representable number is 47 off from i32::MAX. So if you have a calculation that should mathematically result in a number up to i32::MAX, any error >= 1 ULP away from zero will put you out of bounds

This is getting a bit off-topic, but let's say you have this hypothetical algorithm that's supposed to mathematically produce f32s up to 2^31-1 (but should _not_ produce 2^31 or higher, except possibly due to rounding error). It seems to be flawed already.

I think the closest representable i32 is actually 127 below i32::MAX, so even in a perfect world with no floating-point imprecision the algorithm which you expect to be producing values up to 2^31-1 can in fact only produce (legal) values up to 2^31-128. Perhaps that's a bug already. I'm not sure it makes sense to talk about error measured from 2^31-1 when that number is not possible to represent. You'd have to be off by 64 from the nearest representable number (considering rounding) to get out of bounds. Granted, that's not much percentage-wise when you're near 2^32.
You shouldn't expect discrimination of values that are 1 apart (i.e. 2^31-1 but not 2^31) when the closest representable values are 128 apart. Furthermore, only 3.5% of i32s are representable as f32 (and <2% of u32s). You can't get that kind of range while also having that kind of precision with an f32. The algorithm sounds like it's using the wrong tool for the job.

I suppose any practical algorithm that does what you describe will be intimately tied to integers somehow. For example, if you convert a random i32 to f32 and back, it can fail if it's above i32::MAX-64. But that degrades your precision badly and I don't know why you'd do such a thing. Pretty much any i32 -> f32 -> i32 calculation that outputs the full i32 range can be expressed faster and more accurately with integer math, and if not there's f64.

Anyway, while I'm sure it's possible to find some cases where algorithms that perform out-of-bounds conversions would be fixed by saturation, I think they're rare - rare enough that we shouldn't slow down _all_ conversions to accommodate them. And, I'd argue that such algorithms are probably still flawed and should be fixed. And if an algorithm can't be fixed, it can always do a bounds check before the possibly-out-of-bounds conversion (or call a saturating conversion function). That way the expense of bounding the result is paid only where needed.

P.S. Belated happy Thanksgiving to everyone.

admilazz on 3 Dec 2019

To be clear, I don't think anyone disagrees. This flag was only ever proposed as a short-term tool...

I was primarily referring to the proposal to replace -Zsaturated-float-casts with -Zunsaturated-float-casts. Even if saturation becomes the default, flags like -Zunsaturated-float-casts seem bad for compatibility, but if it's also intended to be temporary, then okay, never mind. :-)

Anyway, I'm sure everyone hopes I've said enough on this issue - myself included. I know the Rust team has traditionally sought to provide multiple ways to do things so people can make their own choices between performance and safety. I've shared my perspective and trust that you guys will come up with a good solution in the end. Take care!

admilazz on 3 Dec 2019

I assumed that -Zunsaturated-float-casts would only exist temporarily, and would be removed at some point. That it’s a -Z option (only available on Nightly) rather than -C suggests it, at least.

SimonSapin on 3 Dec 2019

For what it's worth, saturation and UB are not the only choices. Another possibility is to change LLVM to add a variant of fptosi that uses the CPU's native overflow behavior – i.e. the behavior on overflow wouldn't be portable across architectures, but it would be well-defined on any given architecture (e.g. returning 0x80000000 on x86), and it would never return poison or uninitialized memory. Even if the default becomes saturating, it would be nice to have that as an option. After all, whereas saturating casts have inherent overhead on architectures where they're not the default behavior, "do what the CPU does" only has overhead if it inhibits some specific compiler optimization. I'm not sure, but I suspect that any optimizations enabled by treating float-to-int overflow as UB are niche and not applicable to most code.

That said, one issue might be if an architecture has multiple float-to-int instructions that return different values on overflow. In this case, the compiler choosing one or the other would affect observable behavior, which isn't a problem by itself, but might become one if a single fptosi is duplicated and the two copies end up behaving differently. But I'm not sure whether this kind of divergence actually exists on any popular architectures. And the same issue applies to other floating-point optimizations, including floating-point contraction...

comex on 4 Dec 2019

👍2

const fn (miri) has already chosen the saturated-cast behavior since Rust 1.26 though (assuming we want CTFE and RTFE result to be consistent) (before 1.26 the overflowing compile-time cast returns 0)

const fn g(a: f32) -> i32 {
    a as i32
}

const Q: i32 = g(1e+12);

fn main() {
    println!("{}", Q); // always 2147483647
    println!("{}", g(1e+12)); // unspecified value, but always 2147483647 in miri
}

kennytm on 4 Dec 2019

Miri/CTFE uses apfloat's to_u128/to_i128 methods to do the conversion. But I am not sure if this is a stable guarantee -- given in particular that it seems to have changed before (which we were not aware of when implementing that stuff in Miri).

I think we could adjust this to whatever codegen ends up picking. But the fact that LLVM's apfloat (of which the Rust version is a direct port) uses saturation is a good indicator that this is some kind of "reasonable default".

RalfJung on 4 Dec 2019

One solution to the observable behavior could be to randomly choose one of the available methods at build time of the compiler or the resulting binary.
Then have functions like a.saturating_cast::<i32>() for users that require a specific behavior.

dns2utf8 on 4 Dec 2019

@dns2utf8

The word "randomly" would run counter to the effort to get reproducible builds and, if it's predictable within a compiler version, you know someone will decide depend on it not changing.

ssokolow on 4 Dec 2019

👍3

IMO what @comex described (not novel to this thread IIRC, everything old is new again) this is the next best option if we don't want saturation. Note that we don't even need any LLVM changes to test that out, we can use inline asm (on architectures where such instructions exist).

That said, one issue might be if an architecture has multiple float-to-int instructions that return different values on overflow. In this case, the compiler choosing one or the other would affect observable behavior, which isn't a problem by itself, but might become one if a single fptosi is duplicated and the two copies end up behaving differently.

IMO such non-determinism would give up almost all practical advantages compared to freeze. If we do this, we should pick one instruction per architecture and stick to it, both for determinism and so that programs can actually rely on the behavior of the instruction when it makes sense for them. If this is not possible on some architecture, then we could fall back to a software implementation (but as you say this is entirely hypothetical).

This is easiest if we don't delegate this decision to LLVM but implement the operation with inline asm instead. Which incidentially would also be much easier than changing LLVM to add new intrinsics and lowering for them in every backend.

hanna-kruppe on 4 Dec 2019

@rkruppe

[...] Which incidentially would also be much easier than changing LLVM to add new intrinsics and lowering for them in every backend.

Additionally, LLVM isn't exactly happy about intrinsics with target-dependent semantics:

However, if you want the casts to be well defined, then you should define their behavior. "Do some fast thing" is not really a definition, and I don't believe that we should give target-independent constructs target-dependent behavior.

https://groups.google.com/forum/m/#!msg/llvm-dev/cgDFaBmCnDQ/CZAIMj4IBAA

aykevl on 4 Dec 2019

I'm going retag #10184 as solely T-lang: I think the issues to be resolved there are semantic choices about what float as int means

(i.e. whether we are willing to let it have a panicking semantics or not, whether we are willing to let it have a freeze-based underspecification or not, etc)

these are questions better aimed at the T-lang team, not T-compiler, at least for the initial discussion, IMO

pnkfelix on 5 Dec 2019

Just ran into this issue producing results that are _irreproducible between runs_ even without recompiling. The as operator seems to fetch some garbage from memory in such cases.

I suggest just to completely disallow using as for "float as int" and rely on specific rounding methods instead. Reasoning: as is not lossy for other types.

agalakhov on 13 Dec 2019

👍2 😕1

Reasoning: as is not lossy for other types.

Is it?

Based on Rust Book I may assume that it's lossless only in certain cases (namely in cases where the From<X> is defined for a type Y), i.e. you can cast u8 to u32 using From, but not the other way around.

application-developer-DA on 13 Dec 2019

By "not lossy" I mean casting of values that are small enough to fit. Example: 1_u64 as u8 is not lossy, thus u8 as u64 as u8 is not lossy. For floats there is no simple definition of "fits" since 20000000000000000000000000000_u128 as f32 is not lossy while 20000001_u32 as f32 is, so neither float as int nor int as float are lossless.

agalakhov on 13 Dec 2019

256u64 as u8 is lossy though.

RalfJung on 13 Dec 2019

But <anything>_u8 as u64 as u8 is not.

agalakhov on 13 Dec 2019

I think lossiness is normal and expected with casts, and not a problem. Truncating integers with casts (e.g. u32 as u8) is a common operation with a well-understood meaning that's consistent across all C-like languages that I'm aware of (at least on architectures that use two's complement integer representations, which is basically all of them these days). Valid floating-point conversions (i.e. where the integral part fits in the destination) also have well-understood and agreed-upon semantics. 1.6 as u32 is lossy, but all C-like languages that I know of agree that the result should be 1. Both of those cases flow out of the consensus among hardware manufacturers on how those conversions should work and the convention in C-like languages that casts should be high-performance, "I know what I'm doing" kinds of operators.

So I don't think we should consider those problematic in the same way that invalid floating-point conversions are, since those don't have any agreed-upon semantics in C-like languages or in hardware (but they typically result in error states or hardware exceptions) and almost always indicate bugs (in my experience) and are therefore usually nonexistent in correct code.

Just ran into this issue producing results that are irreproducible between runs even without recompiling. The as operator seems to fetch some garbage from memory in such cases.

Personally I think that's fine as long as it only happens when the conversion is invalid and it doesn't have any side effects besides producing a garbage value. If you really need an otherwise invalid conversion in a piece of code you can handle the invalid case yourself with whatever semantics you think it should have.

admilazz on 14 Dec 2019

👍1

and it doesn't have any side effects besides producing a garbage value

The side effect is, garbage value originates somewhere in memory and reveals some (possibly sensitive) data. Returning "random" value computed solely from float itself would be fine, but the current behaviour is not.

Valid floating-point conversions (i.e. where the integral part fits in the destination) also have well-understood and agreed-upon semantics.

Are there any use cases of float-to-int conversions not accompanied by explicit trunc(), round(), floor() or ceil()? The current rounding strategy of as is "undefined", making as barely usable for non-rounded numbers. I believe that in most cases the one who writes x as u32 actually wants x.round() as u32.

I think lossiness is normal and expected with casts, and not a problem.

I agree, but only if lossiness is easily predictable. For integers, the conditions of lossy conversion are obvious. For floats they are obscure. They are lossless for some very big numbers but lossy for some smaller ones, even if they are round. My personal preference is having two different operators for lossy and lossless conversions to avoid introducing lossy conversion by mistake, but I'm also fine with only one operator provided that I can tell if it is lossy or not.

agalakhov on 14 Dec 2019

The side effect is, garbage value originates somewhere in memory and reveals some (possibly sensitive) data.

I would expect it to just leave the destination unchanged or whatever, but if that's really a problem it could be zeroed first.

Are there any use cases of float-to-int conversions not accompanied by explicit trunc(), round(), floor() or ceil()? The current rounding strategy of as is "undefined", making as barely usable for non-rounded numbers.

If the rounding strategy is really undefined then that'd be a surprise to me, and I'd agree that the operator is barely useful unless you're giving it an integer already. I'd expect it to truncate toward zero.

I believe that in most cases the one who writes x as u32 actually wants x.round() as u32.

I guess it depends on the domain, but I expect x.trunc() as u32 is also quite commonly desired.

I agree, but only if lossiness is easily predictable.

I definitely agree. Whether 1.6 as u32 becomes 1 or 2 should not be undefined, for instance.

admilazz on 14 Dec 2019

https://doc.rust-lang.org/nightly/reference/expressions/operator-expr.html#type-cast-expressions

Casting from a float to an integer will round the float towards zero
NOTE: currently this will cause Undefined Behavior if the rounded value cannot be represented by the target integer type. This includes Inf and NaN. This is a bug and will be fixed.

The note links here.

Rounding of values that “fit” is well-defined, that’s not what this issue is about. This thread is already long, it would be nice not to take it into speculating tangents about facts that are already established and documented. Thanks.

What remains to decide is how to define f as $Int in the following cases:

f.trunc() > $Int::MAX (including positive infinity)
f.trunc() < $Int::MIN (including negative infinity)
f.is_nan()

One option that’s already implemented and available in Nightly with the -Z saturating-casts compiler flag is to define them to return respectively: $Int::MAX, $Int::MIN, and zero. But it’s still possible pick some other behavior.

My view is that the behavior should definitely be deterministic and return some integer value (rather than panic for example), but the exact value is not too important and users who care about these cases should rather use conversion methods that I’m separately proposing we add: https://internals.rust-lang.org/t/pre-rfc-add-explicitly-named-numeric-conversion-apis/11395

SimonSapin on 14 Dec 2019

👍4

I guess it depends on the domain, but I expect x.trunc() as u32 is also quite commonly desired.

Correct. In general, x.anything() as u32, most likely round(), but could also be trunc(), floor(), ceil(). Just x as u32 without specifying concrete rounding procedure is most likely a mistake.

My view is that the behavior should definitely be deterministic and return some integer value (rather than panic for example), but the exact value is not too important

I personally am fine even with "undefined" value provided that it does not depend on anything but float itself and, most important, does not expose any unrelated register and memory contents.

agalakhov on 14 Dec 2019

👍2

One option that’s already implemented and available in Nightly with the -Z saturating-casts compiler flag is to define them to return respectively: $Int::MAX, $Int::MIN, and zero. But it’s still possible pick some other behavior.

The behavior I would expect to get for f.trunc() > $Int::MAX and f.trunc() < $Int::MIN is the same as when the floating point number imaginary gets converted to an infinite sized integer number and then the lowest significant bits of that are returned (as in the conversion of integer types). Technically this would be some bits of the significant shifted to the left depending on the exponent (for positive numbers, negative numbers need inversion according to the two's complement).

So for example I would expect really big numbers to convert to 0.

It seems to be harder/more arbitrary to define what infinity and NaN converts to.

mawis on 10 Apr 2020

WebAssembly just stabilized saturating casts with the following semantics:

https://github.com/WebAssembly/nontrapping-float-to-int-conversions/blob/master/proposals/nontrapping-float-to-int-conversion/Overview.md#design

CryZe on 10 Apr 2020

👍2 🚀1

@CryZe so if I read that correctly, that matches -Z saturating-casts (and what Miri already implements)?

RalfJung on 11 Apr 2020

@RalfJung That's correct.

sunfishcode on 11 Apr 2020

Awesome, I'll copy https://github.com/WebAssembly/testsuite/blob/master/conversions.wast (with traps replaced by the specified results) to Miri's test suite then. :)

RalfJung on 11 Apr 2020

@RalfJung Please update to the latest version of conversions.wast, which was just updated to include tests for the new saturating conversion operators. The new operators have "_sat" in their names, and they don't have trapping so you shouldn't need to replace anything.

sunfishcode on 11 Apr 2020

@sunfishcode thanks for updating! I have to translate the tests to Rust anyway so I still have to replace many things. ;)

Are the _sat tests any different in terms of the values being tested? (EDIT: there's a comment there saying the values are the same.) For Rust's saturating casts I took many of these values and added them in https://github.com/rust-lang/miri/pull/1321. I was too lazy to do it for all of them... but I think this means that there is nothing to change right now with the updated file.

For the UB intrinsic, the traps on the wasm side should then become compile-fail tests in Miri I think.

RalfJung on 11 Apr 2020

The input values are all the same, the only difference is that _sat operators have expected output values on inputs where the trapping operators have expected traps.

sunfishcode on 11 Apr 2020

Tests for Miri (and thus also the Rust CTFE engine) were added in https://github.com/rust-lang/miri/pull/1321. I locally checked that rustc -Zmir-opt-level=0 -Zsaturating-float-casts also passes the tests in that file.
I now also implemented the unchecked intrinsic in Miri, see https://github.com/rust-lang/miri/pull/1325.

RalfJung on 12 Apr 2020

🎉1

I've posted https://github.com/rust-lang/rust/pull/71269#issuecomment-615537137 which documents the current state as I understood it and that PR also moves to stabilize the behavior of the saturating -Z flag.

Given the length of this thread I think if folks feel that I've missed anything in that comment, I would direct commentary to the PR, or, if it's minor, feel free to ping me on Zulip or Discord (simulacrum) and I can fix things up to avoid unnecessary noise on the PR thread.

I expect that someone on the language team will likely start an FCP proposal on that PR soon, and merging it will automatically close this issue out :)

Mark-Simulacrum on 18 Apr 2020

❤2

Are there plans for checked conversions? Something like fn i32::checked_from(f64) -> Result<i32, DoesntFit>?

federicomenaquintero on 22 Apr 2020

You'll need to consider what should i32::checked_from(4.5) return.

kennytm on 22 Apr 2020

Rust: floating point to integer casts can cause undefined behaviour

Status as of 2020-04-18

Status as of 2018-11-05

Old status

Most helpful comment

All 234 comments

45134 has pointed out a code path that I missed (generation of LLVM constant expressions – this is separate from rustc's own constant evaluation). I'll roll a fix for that into the same PR, but it will take a little while longer.

45205 has been merged, so anyone can now (well, starting with the next nightly) measure the performance impact of saturation by passing `-Z saturating-float-casts` via `RUSTFLAGS`. [1] Such measurements would be very valuable for deciding how to proceed with this issue.

Related issues

Rust: floating point to integer casts can cause undefined behaviour

Status as of 2020-04-18

Status as of 2018-11-05

Old status

Most helpful comment

All 234 comments

45134 has pointed out a code path that I missed (generation of LLVM constant expressions – this is separate from rustc's own constant evaluation). I'll roll a fix for that into the same PR, but it will take a little while longer.

45205 has been merged, so anyone can now (well, starting with the next nightly) measure the performance impact of saturation by passing -Z saturating-float-casts via RUSTFLAGS. [1] Such measurements would be very valuable for deciding how to proceed with this issue.

Related issues

45205 has been merged, so anyone can now (well, starting with the next nightly) measure the performance impact of saturation by passing `-Z saturating-float-casts` via `RUSTFLAGS`. [1] Such measurements would be very valuable for deciding how to proceed with this issue.