use funnel shift as fallback impl for rotating shifts #148478

RalfJung · 2025-11-04T14:25:03Z

That lets us remove this gnarly implementation from Miri and const-eval.

However, rotate_left/rotate_right are stable as const fn, so to do this we have to rustc_allow_const_fn_unstable a bunch of const trait stuff. Is that a bad idea? Cc @oli-obk @fee1-dead

rustbot · 2025-11-04T14:25:06Z

⚠️ #[rustc_allow_const_fn_unstable] needs careful audit to avoid accidentally exposing unstable
implementation details on stable.

cc @rust-lang/wg-const-eval

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

⚠️ #[miri::intrinsic_fallback_is_spec] must only be used if the function actively checks for all UB cases,
and explores the possible non-determinism of the intrinsic.

cc @rust-lang/miri

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

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

rustbot · 2025-11-04T14:25:09Z

r? @jdonszelmann

rustbot has assigned @jdonszelmann.
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

RalfJung · 2025-11-04T14:26:35Z

library/core/src/intrinsics/mod.rs

+pub const fn rotate_right<T: [const] fallback::FunnelShift>(x: T, shift: u32) -> T {
+    // SAFETY: we modulo `shift` so that the result is definitely less than the size of
+    // `T` in bits.
+    unsafe { unchecked_funnel_shr(x, x, shift % (mem::size_of::<T>() as u32 * 8)) }


FWIW I am not sure if modulo is the correct behavior here -- the docs for the intrinsics and the public function both don't say what happens for shift >= BITS... but the old const-eval implementation did modulo so I guess that's what it has to be?

Yes, that's correct. Also see my other comment, but in short rotate(n) always behaves like rotate(1) iterated n times, and doing the modulo here implements that. After every BITS steps, you're back where you started.

quaternic · 2025-11-05T01:13:29Z

library/core/src/num/int_macros.rs

+        /// `n` itself is truncated to the size of `self` in bits, i.e. the actual shift amount
+        /// is `n % BITS`.
+        ///


This comment seems potentially confusing and redundant at best since the result is the same for any rotation amount modulo Self::BITS. Indeed, x = x.rotate_left(1000); is equivalent to both of

for _ in 0..1000 { x = x.rotate_left(1); }

x = x.rotate_left(1000 % BITS);

The added comment says that it's specifically doing 2, and the reader may be left wondering if that actually is the same as 1, which is what they (semantically) wanted to do with rotate_left(1000). I'd just drop the addition, but maybe the original could be reworded.

Indeed, x = x.rotate_left(1000); is equivalent to both of

Yes, but the docs currently don't say that, and they should. Do you have a proposal for how to say that?

Indeed, though it's a natural property of the operation. Then again, rotations of slices panic if the amount is greater than the length, so one could be checking the docs to see if that also applies to integer rotations.

Maybe like this? Replacing the previous description with

Performs a left circular shift of the bits, also knows as a bitwise rotation. A 1-bit left rotation moves the most significant bit to the least significant position while shifting all other bits left by one. For any `n`, `x.rotate_left(n)` gives the result of repeating that `n` times.

Possibly followed by

Since rotating by `_::BITS` brings each bit back where it started, `x.rotate_left(n)` is equivalent to `x.rotate_left(n % _::BITS)`

(the macros to produce the correct self type for _::BITS in the doc comments look like a pain tbh)

Hm, not sure I like defining the general case in terms of the 1-step case... like, in a formal model I'd definitely do that, but for user-facing docs it seems suboptimal.

What about

Shifts the bits to the left by a specified amount, n, wrapping the truncated bits to the end of the resulting integer.

Note that rotate_left(n) is equivalent to applying rotate_left(1) a total of n times. In particular, rotating self by its own size is a NOP and returns self.

Yeah, I wasn't perfectly happy with that either, but an issue I'm seeing there (in the original description) is that it's not super clear how "shifting left and wrapping the truncated bits to the end of the resulting integer" results in a rotation, especially if n is larger than BITS. That might be clearer with something like

Performs a bitwise rotation (or circular shift) by shifting left while populating new low-order bits with those shifted out.

Anyway, I don't think that actually matters much. (After all, this PR wasn't really about documentation but getting rid of that gnarly const-eval implementation.) Your suggestion looks alright, though for the last sentence rather

In particular, a rotation by the size of the integer type (in bits) returns the input value unchanged.

rustbot · 2025-11-05T07:35:54Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

RalfJung · 2025-11-05T07:56:36Z

library/core/src/intrinsics/mod.rs

+pub const fn rotate_left<T: [const] fallback::FunnelShift>(x: T, shift: u32) -> T {
+    // SAFETY: we modulo `shift` so that the result is definitely less than the size of
+    // `T` in bits.
+    unsafe { unchecked_funnel_shl(x, x, shift % (mem::size_of::<T>() as u32 * 8)) }


With this we could remove the LLVM backend implementation -- it does basically the same thing, lowering rotate to a funnel shift. (It just avoids the % since LLVM's funnel shifts already implicitly mask the shift amount.)

oli-obk · 2025-11-05T15:52:46Z

library/core/src/num/uint_macros.rs

        #[must_use = "this returns the result of the operation, \
                      without modifying the original"]
        #[inline(always)]
+        #[rustc_allow_const_fn_unstable(const_trait_impl)] // for the intrinsic fallback


this seems fine to me. Absolutely worst case we can always remove bounds from the intrinsic and have the compiler do all the work again, but with the setup we have now for libstd bootstrap I'm not worried about even major changes anymore

RalfJung · 2025-11-05T16:09:36Z

@antoyo @bjorn3 in case you'd like to drop the rotate_left/right intrinsic implementations from your backends, that's possible now as there is a library fallback implementation in terms of the funnel shift intrinsics (which in turn also have a fallback implementation).

However, at least cranelift seems to have native support for rotations so we probably want to keep the direct implementation in the backend? OTOH if we can remove it from all backends we can stop making this an intrinsic so that would also be nice. ;)

oli-obk · 2025-11-05T16:13:37Z

@bors try @rust-timer queue

rust-timer · 2025-11-05T16:13:40Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

use funnel shift as fallback impl for rotating shifts

bjorn3 · 2025-11-05T16:16:52Z

However, at least cranelift seems to have native support for rotations so we probably want to keep the direct implementation in the backend?

Indeed.

OTOH if we can remove it from all backends we can stop making this an intrinsic so that would also be nice. ;)

I haven't implemented funnel shift support yet and Cranelift doesn't have native support for this.

rust-log-analyzer · 2025-11-05T18:21:04Z

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

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

tests/pass/shims/x86/rounding-error.rs ... FAILED
tests/pass/shims/x86/intrinsics-x86-gfni.rs ... ok

FAILED TEST: tests/pass/shims/x86/rounding-error.rs
command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-0eWKHE" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/tmp/miri_ui/0/tests/pass/shims/x86" "tests/pass/shims/x86/rounding-error.rs" "--edition" "2021" "--target" "x86_64-pc-windows-gnu"

error: test got exit status: 101, but expected 0
 = note: the compiler panicked

error: no output was expected
Execute `./miri test --bless` to update `tests/pass/shims/x86/rounding-error.stderr` to the actual output
+++ <stderr output>

thread 'main' (1) panicked at tests/pass/shims/x86/rounding-error.rs:28:17:
got an error of 0.00024414063 = 2^-11.999999
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect


full stderr:

thread 'main' (1) panicked at tests/pass/shims/x86/rounding-error.rs:28:17:
got an error of 0.00024414063 = 2^-11.999999
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect

full stdout:


FAILURES:
---
Location:
   /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/ui_test-0.30.2/src/lib.rs:365

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
   1: <color_eyre[58727343e7610ce4]::config::EyreHook>::into_eyre_hook::{closure#0}<unknown>
      at <unknown source file>:<unknown line>
   2: eyre[64e7be959633ad1e]::private::format_err<unknown>
      at <unknown source file>:<unknown line>
   3: ui_test[dd47fcae1eb5cec8]::run_tests_generic::<ui_test[dd47fcae1eb5cec8]::default_file_filter, ui[a592a14cb7615234]::run_tests::{closure#1}, alloc[d2edb3dd3800c929]::boxed::Box<dyn ui_test[dd47fcae1eb5cec8]::status_emitter::StatusEmitter>><unknown>
      at <unknown source file>:<unknown line>
   4: ui[a592a14cb7615234]::ui<unknown>
      at <unknown source file>:<unknown line>
   5: ui[a592a14cb7615234]::main<unknown>
      at <unknown source file>:<unknown line>
   6: std[49432bd0f9f74035]::sys::backtrace::__rust_begin_short_backtrace::<fn() -> core[f77a06b84286b5dd]::result::Result<(), eyre[64e7be959633ad1e]::Report>, core[f77a06b84286b5dd]::result::Result<(), eyre[64e7be959633ad1e]::Report>><unknown>
      at <unknown source file>:<unknown line>
   7: std[49432bd0f9f74035]::rt::lang_start::<core[f77a06b84286b5dd]::result::Result<(), eyre[64e7be959633ad1e]::Report>>::{closure#0}<unknown>
      at <unknown source file>:<unknown line>
   8: std[49432bd0f9f74035]::rt::lang_start_internal<unknown>
      at <unknown source file>:<unknown line>
   9: main<unknown>
      at <unknown source file>:<unknown line>
  10: __libc_start_main<unknown>
      at <unknown source file>:<unknown line>
  11: _start<unknown>
      at <unknown source file>:<unknown line>

Run with COLORBT_SHOW_HIDDEN=1 environment variable to disable frame filtering.
Run with RUST_BACKTRACE=full to include source snippets.
error: test failed, to rerun pass `--test ui`

Caused by:
  process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/deps/ui-60d430e8ea50df4d pass` (exit status: 1)
Bootstrap failed while executing `test --stage 2 src/tools/miri --target x86_64-pc-windows-gnu --test-args pass`
Command `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo test --target x86_64-unknown-linux-gnu -Zbinary-dep-depinfo -j 4 -Zroot-dir=/checkout --locked --color always --release --manifest-path /checkout/src/tools/miri/Cargo.toml -- pass [workdir=/checkout]` failed with exit code 1
Created at: src/bootstrap/src/core/build_steps/tool.rs:191:21
Executed at: src/bootstrap/src/core/build_steps/test.rs:676:19

Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:02:05
  local time: Wed Nov  5 18:18:51 UTC 2025
  network time: Wed, 05 Nov 2025 18:18:51 GMT
##[error]Process completed with exit code 1.
##[group]Run echo "disk usage:"

rust-bors · 2025-11-05T18:24:31Z

☀️ Try build successful (CI)
Build commit: 4819af7 (4819af74720dbd5612b2c8eaab40bd983fb38646, parent: 53efb3d4f3b67d189a0c72eb475a52017d79d609)

rust-timer · 2025-11-05T18:24:34Z

Queued 4819af7 with parent 53efb3d, future comparison URL.
There are currently 2 preceding artifacts in the queue.
It will probably take at least ~2.8 hours until the benchmark run finishes.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 4, 2025

rustbot assigned jdonszelmann Nov 4, 2025

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Nov 4, 2025

RalfJung force-pushed the rotating-funnel branch from 4824240 to e47ba14 Compare November 4, 2025 14:25

RalfJung commented Nov 4, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the rotating-funnel branch from c27f09a to a180c65 Compare November 4, 2025 22:58

This comment has been minimized.

Sign in to view

quaternic reviewed Nov 5, 2025

View reviewed changes

RalfJung force-pushed the rotating-funnel branch from a180c65 to fb03e19 Compare November 5, 2025 07:35

RalfJung commented Nov 5, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

oli-obk reviewed Nov 5, 2025

View reviewed changes

rustbot added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Nov 5, 2025

RalfJung added 2 commits November 5, 2025 17:10

use funnel shift as fallback impl for rotating shifts

0e43b2e

document behavior of rotations for n >= BITS

e8833ad

RalfJung force-pushed the rotating-funnel branch from af730a5 to 6ce020d Compare November 5, 2025 16:10

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Nov 5, 2025

Auto merge of #148478 - RalfJung:rotating-funnel, r=<try>

4819af7

use funnel shift as fallback impl for rotating shifts

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 5, 2025

This comment has been minimized.

Sign in to view

antoyo mentioned this pull request Nov 5, 2025

Use GENERIC expressions for the rotate intrinsics rust-lang/rustc_codegen_gcc#798

Open

use fallback impl in LLVM backend

e347990

RalfJung force-pushed the rotating-funnel branch from 6ce020d to e347990 Compare November 5, 2025 17:07

use funnel shift as fallback impl for rotating shifts #148478

Are you sure you want to change the base?

use funnel shift as fallback impl for rotating shifts #148478

Conversation

RalfJung commented Nov 4, 2025

Uh oh!

rustbot commented Nov 4, 2025

Uh oh!

rustbot commented Nov 4, 2025

Uh oh!

RalfJung Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Nov 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Nov 5, 2025

Uh oh!

oli-obk commented Nov 5, 2025

Uh oh!

rust-timer commented Nov 5, 2025

Uh oh!

This comment has been minimized.

bjorn3 commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

rust-log-analyzer commented Nov 5, 2025

Uh oh!

rust-bors bot commented Nov 5, 2025

Uh oh!

rust-timer commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

RalfJung Nov 4, 2025 •

edited

Loading

bjorn3 commented Nov 5, 2025 •

edited

Loading