Add `{f32,f64}::approx_unchecked_to<Int>` unsafe methods #66841

SimonSapin · 2019-11-28T14:45:33Z

As discussed in #10184

Currently, casting a floating point number to an integer with as is Undefined Behavior if the value is out of range. -Z saturating-float-casts fixes this soundness hole by making as “saturate” to the maximum or minimum value of the integer type (or zero for NaN), but has measurable negative performance impact in some benchmarks. There is some consensus in that thread for enabling saturation by default anyway, but provide an unsafe fn alternative for users who know through some other mean that their values are in range.

The “fit” wording is copied from https://llvm.org/docs/LangRef.html#fptoui-to-instruction, but I’m not certain what it means exactly. Presumably this is after rounding towards zero, and the doc-test with i8::MIN seems to confirm this. Clang presumably uses those LLVM intrinsics to implement C and C++ casts, whose respective standard specify that the value after truncating to keep its integral part must be representable in the target type.

rust-highfive · 2019-11-28T14:45:37Z

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

SimonSapin · 2019-11-28T14:47:28Z

Regarding naming: we already have f32::round(self) -> Self and f64::round(self) -> Self which round to the nearest integer.

These new methods differ from those in that they round towards zero. However they are already different enough in their return type that perhaps documentation is enough and this different rounding behavior doesn’t need to be called out in the method name?

Centril · 2019-11-28T15:43:06Z

r? @rkruppe

src/librustc_codegen_llvm/intrinsic.rs

Centril · 2019-11-28T15:46:18Z

src/libcore/num/f32.rs

+    ///
+    /// # Safety
+    ///
+    /// The value must be finite and fit in the return type.


Maybe this could be elaborated upon what finite and fitting entails?

Finite is defined in the docs of the is_finite method, but I can repeat in this comment.

As discussed in the PR description, “fit” is copied from https://llvm.org/docs/LangRef.html#fptoui-to-instruction and I couldn’t find anything more precise. I’d prefer to find out what LLVM does exactly rather than documenting suppositions as fact, but I’m not sure how.

Re. finite, maybe link to is_finite saying "as defined by is_finite"?

Hopefully @rkruppe is more knowledgeable re. "fit".

This bit of doc-comment is now “The value must be finite (not infinite or NaN) and fit in the return type.” and I’ve added a (non-doc-) comment with a link to relevant LLVM docs.

If it's not spelled out in LLVM's LangRef, then my working knowledge of how the instruction works probably isn't worth much. There's dozens of places in LLVM that could deviate from the expected semantics (various IR passes and utilities, as well as the lowerings in the backends) and I do not know all of them.

hanna-kruppe · 2019-11-28T18:27:50Z

The implementation looks good to me.

Regarding naming: I don't think "round" is a good name. Not only do we already have APIs that use "round" in a different sense for the same pairs of types (even if the signatures are not literally identical because these new functions are generic), I would also never think to suspect that something named just "round" defaults to RTZ mode, even with the context of a float -> int signature.

I don't love any of these but here are some alternatives that seem less misleading to me:

unchecked_cast_to (leaning on the general precedent of float -> int casts rounding to zero)
unchecked_to_int
unchecked_round_to_zero (wow this is ugly)

SimonSapin · 2019-11-28T20:12:23Z

The “fit” wording is copied from https://llvm.org/docs/LangRef.html#fptoui-to-instruction, but I’m not certain what it means exactly.

Presumably fptoui and fptosi exist to allow clang to implement C and C++. Emphasis mine:

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.

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.

So “fit” is indeed after rounding/truncation.

SimonSapin · 2019-11-28T20:15:05Z

@rkruppe What do you think of just unchecked_to? There is precedent with Into::into and TryInto::try_into of the method name not saying (in)to what, and although it’s optional the turbofish can answer that question: float_value.unchecked_to::<u16>()

hanna-kruppe · 2019-11-28T20:41:10Z

Seems fine to me. I assume we don't really have to worry about a future name collision with a hypothetical std::convert::UncheckedInto trait.

SimonSapin · 2019-11-28T21:00:50Z

I’m considering whether the trait should be something like std::convert::FloatToInt, such that in the future we could have other methods in it (also supporting inherent methods of f32 and f64) with various conversions semantics.

SimonSapin · 2019-11-29T13:33:41Z

These methods could fit among other conversion methods: https://internals.rust-lang.org/t/pre-rfc-add-explicitly-named-numeric-conversion-apis/11395

SimonSapin · 2019-12-02T09:15:52Z

I’ve renamed the methods to approx_unchecked_to per conversation in that internals thread, and moved/renamed the trait to convert::FloatToInt in anticipation for additional methods with different semantics.

SimonSapin · 2019-12-05T16:51:49Z

I’ve filed tracking issue #67057 and #67058, and made the stability attribute point to them.

@rkruppe, anything else you’d like to see before we land this?

hanna-kruppe · 2019-12-05T19:26:27Z

I'm happy with the final diff but FWIW I also don't have any opinion on the API design aspects (e.g., the entirety of the FloatToInt trait) other than not making the name misleading.

The individual commits seem worth revising before merging, though:

Contrary to its commit message, 0145ed7edee9ec9de71c0b509841d7263eb16045 seems to just delete libstd/convert.rs. libstd/convert/mod.rs file only shows up in the following commit.
Having separate commits that rename the file and move the numeric From/TryFrom impls to covert/num.rs seems useful for history spelunking, but the other commits should be squashed into 20f091b8948a03072adbcc3e9207c3795399f88c (including updating the commit message for the new method names and dropping the last paragraph about LLVM docs being vague).

As discussed in rust-lang#10184 Currently, casting a floating point number to an integer with `as` is Undefined Behavior if the value is out of range. `-Z saturating-float-casts` fixes this soundness hole by making `as` “saturate” to the maximum or minimum value of the integer type (or zero for `NaN`), but has measurable negative performance impact in some benchmarks. There is some consensus in that thread for enabling saturation by default anyway, but provide an `unsafe fn` alternative for users who know through some other mean that their values are in range.

This makes `libcore/num/mod.rs` slightly smaller. It’s still 4911 lines and not easy to navigate. This doesn’t change any public API.

SimonSapin · 2019-12-06T13:05:13Z

Squashed.

Input from @rust-lang/libs about the API design is welcome here. This is all #[unstable] so we can always discuss in the tracking issues after this is merged and change it later.

hanna-kruppe · 2019-12-06T16:12:24Z

Yes, let's merge this and bikeshedding can happen on the tracking issues.

@bors r+

bors · 2019-12-06T16:12:26Z

📌 Commit a213ff8 has been approved by rkruppe

…, r=rkruppe Add `{f32,f64}::approx_unchecked_to<Int>` unsafe methods As discussed in rust-lang#10184 Currently, casting a floating point number to an integer with `as` is Undefined Behavior if the value is out of range. `-Z saturating-float-casts` fixes this soundness hole by making `as` “saturate” to the maximum or minimum value of the integer type (or zero for `NaN`), but has measurable negative performance impact in some benchmarks. There is some consensus in that thread for enabling saturation by default anyway, but provide an `unsafe fn` alternative for users who know through some other mean that their values are in range. <del>The “fit” wording is copied from https://llvm.org/docs/LangRef.html#fptoui-to-instruction, but I’m not certain what it means exactly. Presumably this is after rounding towards zero, and the doc-test with `i8::MIN` seems to confirm this.</del> Clang presumably uses those LLVM intrinsics to implement C and C++ casts, whose respective standard specify that the value *after truncating to keep its integral part* must be representable in the target type.

@ghost

Rollup of 10 pull requests Successful merges: - #66606 (Add feature gate for mut refs in const fn) - #66841 (Add `{f32,f64}::approx_unchecked_to<Int>` unsafe methods) - #67009 (Emit coercion suggestions in more places) - #67052 (Ditch `parse_in_attr`) - #67071 (Do not ICE on closure typeck) - #67078 (accept union inside enum if not followed by identifier) - #67090 (Change "either" to "any" in Layout::from_size_align's docs) - #67092 (Fix comment typos in src/libcore/alloc.rs) - #67094 (get rid of __ in field names) - #67102 (Add note to src/ci/docker/README.md about multiple docker images) Failed merges: - #67101 (use `#[allow(unused_attributes)]` to paper over incr.comp problem) r? @ghost

rust-highfive assigned KodrAus Nov 28, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 28, 2019

SimonSapin mentioned this pull request Nov 28, 2019

floating point to integer casts can cause undefined behaviour #10184

Closed

SimonSapin force-pushed the float_round_unchecked_to branch 2 times, most recently from 28486c3 to 296dc11 Compare November 28, 2019 14:56

rust-highfive assigned hanna-kruppe and unassigned KodrAus Nov 28, 2019

Centril reviewed Nov 28, 2019

View reviewed changes

SimonSapin changed the title ~~Add {f32,f64}::round_unchecked_to<Int> unsafe methods~~ Add {f32,f64}::approx_unchecked_to<Int> unsafe methods Dec 2, 2019

SimonSapin force-pushed the float_round_unchecked_to branch from 427ec1b to 16a4fc3 Compare December 5, 2019 16:31

This was referenced Dec 5, 2019

Tracking issue for the convert::FloatToInt trait #67057

Open

Tracking issue for {f32,f64}::to_int_unchecked methods #67058

Closed

SimonSapin added 3 commits December 6, 2019 13:56

Make core::convert a directory-module with mod.rs

f442797

Move numeric From and TryFrom impls to libcore/convert/num.rs

a213ff8

This makes `libcore/num/mod.rs` slightly smaller. It’s still 4911 lines and not easy to navigate. This doesn’t change any public API.

SimonSapin force-pushed the float_round_unchecked_to branch from 37595ce to a213ff8 Compare December 6, 2019 13:02

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2019

Centril mentioned this pull request Dec 6, 2019

Rollup of 10 pull requests #67104

Merged

bors merged commit a213ff8 into rust-lang:master Dec 7, 2019

RalfJung mentioned this pull request Mar 26, 2020

Support unsafe float-to-int casts rust-lang/miri#1264

Closed

SimonSapin deleted the float_round_unchecked_to branch March 29, 2020 15:25

lopopolo mentioned this pull request Aug 21, 2022

Switch artichoke-backend to use spinoso-time tzrs feature artichoke/artichoke#1956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `{f32,f64}::approx_unchecked_to<Int>` unsafe methods #66841

Add `{f32,f64}::approx_unchecked_to<Int>` unsafe methods #66841

SimonSapin commented Nov 28, 2019 •

edited

Loading

rust-highfive commented Nov 28, 2019

SimonSapin commented Nov 28, 2019

Centril commented Nov 28, 2019

Centril Nov 28, 2019

SimonSapin Nov 28, 2019 •

edited

Loading

Centril Nov 28, 2019

SimonSapin Nov 28, 2019

hanna-kruppe Nov 28, 2019

hanna-kruppe commented Nov 28, 2019

SimonSapin commented Nov 28, 2019

SimonSapin commented Nov 28, 2019

hanna-kruppe commented Nov 28, 2019

SimonSapin commented Nov 28, 2019

SimonSapin commented Nov 29, 2019

SimonSapin commented Dec 2, 2019

SimonSapin commented Dec 5, 2019

hanna-kruppe commented Dec 5, 2019

SimonSapin commented Dec 6, 2019

hanna-kruppe commented Dec 6, 2019

bors commented Dec 6, 2019

Add {f32,f64}::approx_unchecked_to<Int> unsafe methods #66841

Add {f32,f64}::approx_unchecked_to<Int> unsafe methods #66841

Conversation

SimonSapin commented Nov 28, 2019 • edited Loading

rust-highfive commented Nov 28, 2019

SimonSapin commented Nov 28, 2019

Centril commented Nov 28, 2019

Centril Nov 28, 2019

Choose a reason for hiding this comment

SimonSapin Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

Centril Nov 28, 2019

Choose a reason for hiding this comment

SimonSapin Nov 28, 2019

Choose a reason for hiding this comment

hanna-kruppe Nov 28, 2019

Choose a reason for hiding this comment

hanna-kruppe commented Nov 28, 2019

SimonSapin commented Nov 28, 2019

SimonSapin commented Nov 28, 2019

hanna-kruppe commented Nov 28, 2019

SimonSapin commented Nov 28, 2019

SimonSapin commented Nov 29, 2019

SimonSapin commented Dec 2, 2019

SimonSapin commented Dec 5, 2019

hanna-kruppe commented Dec 5, 2019

SimonSapin commented Dec 6, 2019

hanna-kruppe commented Dec 6, 2019

bors commented Dec 6, 2019

Add `{f32,f64}::approx_unchecked_to<Int>` unsafe methods #66841

Add `{f32,f64}::approx_unchecked_to<Int>` unsafe methods #66841

SimonSapin commented Nov 28, 2019 •

edited

Loading

SimonSapin Nov 28, 2019 •

edited

Loading