Mark float intrinsics with no preconditions as safe #146683

clarfonthey · 2025-09-17T18:17:17Z

Note: for ease of reviewing, the list of safe intrinsics is sorted in the first commit, and then safe intrinsics are added in the second commit.

All recently added float intrinsics have been correctly marked as safe to call due to the fact that they have no preconditions. This adds the remaining float intrinsics which are safe to call to the safe intrinsic list, and removes the unsafe blocks around their calls.

Side note: this may want a try run before being added to the queue, since I'm not sure if there's any tier-2 code that uses these intrinsics that might not be tested on the usual PR flow. We've already uncovered a few places in subtrees that do this, and it's worth double-checking before clogging up the queue.

rustbot · 2025-09-17T18:17:19Z

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

cc @tgross35

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

rustbot · 2025-09-17T18:17:22Z

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

clarfonthey · 2025-09-17T18:24:45Z

library/compiler-builtins/libm/src/math/support/float_traits.rs

                    // fma is not yet available in `core`
                    if #[cfg(intrinsics_enabled)] {
-                        unsafe{ core::intrinsics::$fma_intrinsic(self, y, z) }
+                        core::intrinsics::$fma_intrinsic(self, y, z)


I don't think it's possible to make this change separately, unfortunately. The best we could do is allow the lint for an unnecessary unsafe block, but that also would presumably require modifying the code within this crate, so, 🤷🏻

Yeah there's no problem with this bit 👍

rustbot · 2025-09-17T19:34:50Z

stdarch is developed in its own repository. If possible, consider making this change to rust-lang/stdarch instead.

cc @Amanieu, @folkertdev, @sayantn

clarfonthey · 2025-09-17T19:35:39Z

library/stdarch/crates/stdarch-gen-arm/spec/neon/aarch64.spec.yml

      - [f16, 'h_']
    compose:
-      - FnCall: [roundf16, [a], [], true]
+      - FnCall: [roundf16, [a], []]


These also seem to be difficult/not possible to do separately from modifying the intrinsics, since they otherwise throw unnecessary unsafe blocks.

Yeah this kind of change is exactly why we use subtrees instead of submodules :)

compiler/rustc_hir_analysis/src/check/intrinsic.rs

RalfJung · 2025-09-17T20:30:28Z

library/core/src/intrinsics/mod.rs

+pub const fn roundf128(x: f128) -> f128;

 /// Float addition that allows optimizations based on algebraic rules.
 /// May assume inputs are finite.


This line should really say

/// Safety: all inputs the result must be finite.

We unfortunately haven't adopted a proper safety section convention for intrinsics since some of them became safe, but a lot of them mention UB directly as a keyword to search for, so, I reworded it to:

Requires that inputs are finite, and can cause UB otherwise.

Hopefully that's a bit clearer for now until someone wants to properly clean up all the intrinsic documentation.

Inputs and output must be finite, as noted in my comment. (Though I typod it so it was probably hard to understand.)

Also, this will cause UB otherwise. "can" sounds like maybe it does, maybe it doesn't.

How would the output potentially not be finite if the input is finite?

… I asked before realising the answer. Okay, fair. That definitely is worth pointing out.

Finally got to this, new comment is:

Requires that both input and output of the operation are finite, causing UB otherwise.

This also got me curious whether the remainder of division by zero was finite by some weird chance, and it's NaN, so, this should be sufficient for all cases. (If it were something weird like rem-0 = 0, then I would have had to specify for remainder specifically that the division also has to be finite, but it's okay here, I think.)

rustbot · 2025-09-18T02:13:34Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung

r=me on the intrinsic changes. I don't know stdarch well enough to comment on the diff there. @Amanieu could you take a look?

View changes since this review

RalfJung · 2025-09-20T11:09:32Z

library/core/src/intrinsics/mod.rs


 /// Float addition that allows optimizations based on algebraic rules.
-/// May assume inputs are finite.
+/// Requires that both input and output of the operation are finite, causing UB otherwise.


Should probably say "inputs" since there is more than one

Fair, now says "inputs and output" instead of "both input and output" so it's a bit clearer

Amanieu · 2025-09-20T11:16:54Z

LGTM on the stdarch side, this is just removing some unnecessary unsafe blocks.

rust-log-analyzer · 2025-09-22T09:27:39Z

The job x86_64-msvc-ext2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : WebException
 

thread 'main' panicked at src\bootstrap\src\core\download.rs:988:5:
move_file(&tempfile, dest_path) failed with The system cannot find the file specified. (os error 2) ("failed to rename \"D:\\\\a\\\\rust\\\\rust\\\\build\\\\tmp\\\\rustfmt-nightly-x86_64-pc-windows-msvc.tar.xz\" to \"D:\\\\a\\\\rust\\\\rust\\\\build\\\\cache\\\\2025-09-05\\\\rustfmt-nightly-x86_64-pc-windows-msvc.tar.xz\"")
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/788da80fcfcef3f34c90def5baa32813e39a1a41/library\std\src\panicking.rs:697
   1: core::panicking::panic_fmt
             at /rustc/788da80fcfcef3f34c90def5baa32813e39a1a41/library\core\src\panicking.rs:75
   2: bootstrap::core::download::download_file<ref$<bootstrap::core::download::DownloadContext> >
             at .\src\bootstrap\src\utils\helpers.rs:63
   3: bootstrap::core::download::download_component<ref$<bootstrap::core::download::DownloadContext> >
             at .\src\bootstrap\src\core\download.rs:843
   4: bootstrap::core::download::maybe_download_rustfmt<ref$<bootstrap::core::download::DownloadContext> >
             at .\src\bootstrap\src\core\download.rs:518
   5: bootstrap::core::config::config::impl$0::parse_inner::closure$15<enum2$<core::result::Result<bootstrap::core::config::toml::TomlConfig,toml::de::Error> > (*)(ref$<std::path::Path>)>
             at .\src\bootstrap\src\core\config\config.rs:933
   6: core::option::Option::or_else<std::path::PathBuf,bootstrap::core::config::config::impl$0::parse_inner::closure_env$15<enum2$<core::result::Result<bootstrap::core::config::toml::TomlConfig,toml::de::Error> > (*)(ref$<std::path::Path>)> >
             at /rustc/788da80fcfcef3f34c90def5baa32813e39a1a41\library\core\src\option.rs:1647
   7: bootstrap::core::config::config::Config::parse_inner<enum2$<core::result::Result<bootstrap::core::config::toml::TomlConfig,toml::de::Error> > (*)(ref$<std::path::Path>)>
             at .\src\bootstrap\src\core\config\config.rs:933
   8: bootstrap::core::config::config::Config::parse
             at .\src\bootstrap\src\core\config\config.rs:345
   9: bootstrap::main
             at .\src\bootstrap\src\bin\main.rs:39
  10: core::ops::function::FnOnce::call_once<void (*)(),tuple$<> >
             at /rustc/788da80fcfcef3f34c90def5baa32813e39a1a41\library\core\src\ops\function.rs:253
  11: core::hint::black_box
             at /rustc/788da80fcfcef3f34c90def5baa32813e39a1a41\library\core\src\hint.rs:482
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Panic was initiated from src\bootstrap\src\core\download.rs:988:5
Build completed unsuccessfully in 0:00:08

RalfJung · 2025-09-22T09:47:09Z

@bors retry curl: (35) Recv failure: Connection was reset

bors · 2025-09-22T14:35:51Z

⌛ Testing commit 055e05a with merge ce4beeb...

tgross35 · 2025-09-22T17:27:29Z

Fwiw when this came up before, a concern was raised about whether linkers must raise an error when symbols aren't found. Discussion starting around #t-libs > Different Safety Levels of the Same API in Intrinsic and std @ 💬. I don't really think that's something we should be worried about here though.

bors · 2025-09-22T17:48:46Z

☀️ Test successful - checks-actions
Approved by: RalfJung,Amanieu
Pushing ce4beeb to master...

github-actions · 2025-09-22T17:51:53Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 29005cb (parent) -> ce4beeb (this PR)

Test differences

Show 182 test diffs

182 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard ce4beebecb77821734079cff47d8af08f9f27f11 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

pr-check-1: 1800.5s -> 1419.9s (-21.1%)
dist-arm-linux-musl: 5591.8s -> 6256.3s (11.9%)
i686-gnu-2: 6351.3s -> 5619.7s (-11.5%)
dist-aarch64-apple: 7386.9s -> 6552.7s (-11.3%)
aarch64-gnu-llvm-20-1: 3717.3s -> 3334.5s (-10.3%)
i686-gnu-1: 8359.1s -> 7557.7s (-9.6%)
x86_64-mingw-1: 11146.0s -> 10083.7s (-9.5%)
aarch64-gnu: 6902.5s -> 6249.2s (-9.5%)
x86_64-rust-for-linux: 2856.7s -> 2606.9s (-8.7%)
arm-android: 6245.2s -> 5713.5s (-8.5%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-09-22T18:59:43Z

Finished benchmarking commit (ce4beeb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.6%, secondary 0.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	4.7%	[4.7%, 4.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.2%]	1
All ❌✅ (primary)	0.6%	[0.6%, 0.6%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 472.389s -> 472.191s (-0.04%)
Artifact size: 389.97 MiB -> 389.95 MiB (-0.01%)

…alfJung Make missed precondition-free float intrinsics safe So, in my defence, these were both separated out from the other intrinsics in the file *and* had a different safety comment in the stable versions, so, I didn't notice them before. But, in my offence, the entire reason I did the previous PR was because I was using them for SIMD intrinsic fallbacks, and `fabs` is needed for those too, so, I don't really have an excuse. Extra follow-up to rust-lang#146683. r? `@RalfJung` who reviewed the previous one These don't appear to be used anywhere outside of the standard locations, at least.

…ung,Amanieu Mark float intrinsics with no preconditions as safe Note: for ease of reviewing, the list of safe intrinsics is sorted in the first commit, and then safe intrinsics are added in the second commit. All *recently added* float intrinsics have been correctly marked as safe to call due to the fact that they have no preconditions. This adds the remaining float intrinsics which are safe to call to the safe intrinsic list, and removes the unsafe blocks around their calls. --- Side note: this may want a try run before being added to the queue, since I'm not sure if there's any tier-2 code that uses these intrinsics that might not be tested on the usual PR flow. We've already uncovered a few places in subtrees that do this, and it's worth double-checking before clogging up the queue.

…alfJung Make missed precondition-free float intrinsics safe So, in my defence, these were both separated out from the other intrinsics in the file *and* had a different safety comment in the stable versions, so, I didn't notice them before. But, in my offence, the entire reason I did the previous PR was because I was using them for SIMD intrinsic fallbacks, and `fabs` is needed for those too, so, I don't really have an excuse. Extra follow-up to rust-lang#146683. r? ``@RalfJung`` who reviewed the previous one These don't appear to be used anywhere outside of the standard locations, at least.

…alfJung Make missed precondition-free float intrinsics safe So, in my defence, these were both separated out from the other intrinsics in the file *and* had a different safety comment in the stable versions, so, I didn't notice them before. But, in my offence, the entire reason I did the previous PR was because I was using them for SIMD intrinsic fallbacks, and `fabs` is needed for those too, so, I don't really have an excuse. Extra follow-up to rust-lang#146683. r? ```@RalfJung``` who reviewed the previous one These don't appear to be used anywhere outside of the standard locations, at least.

Rollup merge of #146915 - clarfonthey:safe-intrinsics-2, r=RalfJung Make missed precondition-free float intrinsics safe So, in my defence, these were both separated out from the other intrinsics in the file *and* had a different safety comment in the stable versions, so, I didn't notice them before. But, in my offence, the entire reason I did the previous PR was because I was using them for SIMD intrinsic fallbacks, and `fabs` is needed for those too, so, I don't really have an excuse. Extra follow-up to #146683. r? ```@RalfJung``` who reviewed the previous one These don't appear to be used anywhere outside of the standard locations, at least.

rustbot assigned ibraheemdev Sep 17, 2025

clarfonthey force-pushed the safe-intrinsics branch from 3cd0908 to 6e2a954 Compare September 17, 2025 18:22

clarfonthey commented Sep 17, 2025

View reviewed changes