Implement Integer funnel shifts #145690

sayantn · 2025-08-20T23:51:38Z

Parent: #145686
ACP: rust-lang/libs-team#642

This implements funnel shifts on primitive integer types. Implements this for cg_llvm, with a fallback impl for everything else

Thanks @folkertdev for the fixes and tests

cc @rust-lang/libs-api

rustbot · 2025-08-20T23:51:42Z

r? @tgross35

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

rustbot · 2025-08-20T23:51:45Z

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

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

library/core/src/intrinsics/mod.rs

compiler/rustc_codegen_llvm/src/intrinsic.rs

library/core/src/num/uint_macros.rs

library/core/src/intrinsics/mod.rs

compiler/rustc_const_eval/src/interpret/intrinsics.rs

tgross35 · 2025-08-21T19:38:17Z

I see there is a tracking issue, but not an ACP for this. Could you create one? It's an issue template at https://github.com/rust-lang/libs-team/issues.

library/core/src/intrinsics/mod.rs

rustbot · 2025-08-22T16:18:34Z

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.

rustbot · 2025-08-24T21:06:08Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

sayantn · 2025-08-28T02:09:13Z

@bjorn3 could you look into this ICE? The clif code looks fine enough, and I don't have enough expertise on clif to see what's going wrong.

bjorn3 · 2025-08-28T09:18:45Z

At the very least there is a bug in the Cranelift IR verifier causing it to panic at https://github.com/bytecodealliance/wasmtime/blob/ecda6e330a946821b5edc0a90bd62288cf7df943/cranelift/codegen/src/verifier/mod.rs#L1907

But the thing you are doing wrong is let zero = fx.bcx.ins().iconst(ty, 0); I think, which is not valid if ty is an i128. You can use the type_zero_value helper function from the common module instead.

Edit: I'm not sure how that even passed the typecheck pass of the verifier.

tgross35 · 2025-08-28T23:07:21Z

library/core/src/num/uint_macros.rs

Could you update the tracking issue to have signatures matching rust-lang/libs-team#642 (comment)? This should match then.

Done 👍🏽, could you check if it makes sense (I'm bad at writing docs etc)

It's easier than that: tracking issues should show function signatures in something like an impl uX { ... } block, no need to write docs :)

library/core/src/intrinsics/mod.rs

library/coretests/tests/num/uint_macros.rs

sayantn · 2025-08-30T00:52:27Z

Should I add more variants of the functions? (e.g. wrapping, overflowing, checked, unchecked etc, the default one is the traditionally strict variant)

tgross35 · 2025-08-30T01:10:24Z

I don't think there is any need in the initial PR here. They can be added as a followup

clarfonthey · 2025-08-30T01:13:20Z

Also worth mentioning that the hardest part here is getting the intrinsic and actual functionality working, since all the other variants, at least for shifts, just require a few explicit checks.

sayantn · 2025-08-30T02:23:17Z

In that case, I guess this PR is ready. @rustbot ready

RalfJung · 2025-08-30T11:04:13Z

library/core/src/intrinsics/mod.rs

+#[rustc_nounwind]
+#[rustc_const_unstable(feature = "funnel_shifts", issue = "145686")]
+#[unstable(feature = "funnel_shifts", issue = "145686")]
+#[miri::intrinsic_fallback_is_spec]


This attribute means the fallback implementation must check the size to be valid and do unreachable_unchecked (or equivalent) in that case.

Please add Miri tests to ensure this works correctly: all cases of UB must be detected.

Looking at the implementation, I don't think you are checking for UB here. Seems like unchecked_funnel_shl(0, 0, n) will just work for any n.

Please don't just blindly add attributes like this, make sure you understand them or ask what to do. Adding this attribute incorrectly introduces hard-to-find bugs into the very tools unsafe code authors trust to find bugs for them.

EDIT: Or, are you relying on the checks in unchecked_shr/unchecked_shl? That's super subtle, definitely needs a comment at least.

Also clarifying a bit: since the fallbacks work a bit differently than you'd expect, this kind of info should probably be listed in the SAFETY comment a few lines below. Something like "UB-checking is done inside the called method" and then adding more explicit comments inside the trait implementations.

I was not actually testing for UB here, I misunderstood what the attribute means. Should I add such UB checks?

I have added some assume calls. @RalfJung could you check if it's ok?

To be fair, I don't think that relying on the attribute missing is reasonable, since you can easily just copy intrinsic bodies here. It makes a lot more sense to just have a triage bot reply to any PR that includes the attribute to make sure people get notified.

Both you personally and miri as a group were pinged in various ways in #145690 (comment). Then the implementation evolved somewhat but it seems reasonable to assume that this PR already had your attention?

If not, should we add a ping rule for that attribute specifically?

I think there should be a ping rule for the attribute just to be safe, since there appears to be a desire to ping the miri group whenever it's used.

I don't think we have support for "ping on specific text", see rust-lang/triagebot#1851. If we did have that then yes we'd definitely use it here.

To be fair, I don't think that relying on the attribute missing is reasonable, since you can easily just copy intrinsic bodies here.

I don't think it is reasonable to copy an attribute you don't understand, and then just hope it works out. I expect contributors to flag things like this explicitly in the PR so it can be discussed.

We could rename the attribute to make it sound more scary, but it's not clear to me that that would help. It's also hard to find a reasonably concise name that captures every aspect of this attribute.

Both you personally and miri as a group were pinged in various ways

That's true, I forgot miri already got pinged because the intrinsic file changed.

I don't think it is reasonable to copy an attribute you don't understand, and then just hope it works out.

I agree, it was a mistake on my part. I didn't think it through, and just copied the similar implementations of intrinsics, partly due to my inexperience in implementing rust-intrinsics. Anyway, thanks you all for all the help in pointing out this mistake

library/core/src/intrinsics/fallback.rs

compiler/rustc_codegen_llvm/src/intrinsic.rs

library/core/src/intrinsics/fallback.rs

- Add a fallback implementation for the intrinsics Co-Authored-By: folkertdev <folkert@folkertdev.nl>

Co-Authored-By: folkertdev <folkert@folkertdev.nl>

rustbot · 2025-09-02T01:12:44Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung

The Miri checks look good apart from a small nit, thanks!

View changes since this review

RalfJung · 2025-09-02T08:45:10Z

library/core/src/intrinsics/fallback.rs

+            #[cfg_attr(miri, track_caller)]
+            #[inline]
+            unsafe fn unchecked_funnel_shl(self, rhs: Self, shift: u32) -> Self {
+                // SAFETY: this is guaranteed by the caller


Suggested change

// SAFETY: this is guaranteed by the caller

// This implementation is also used by Miri so we have to check the precondition.

// SAFETY: this is guaranteed by the caller

(same in unchecked_funnel_shr)

RalfJung · 2025-09-02T08:47:42Z

src/tools/miri/tests/fail/intrinsics/funnel_shl.rs

+        std::intrinsics::unchecked_funnel_shl(1_u32, 2, 5);
+        std::intrinsics::unchecked_funnel_shl(1_u32, 2, 31);


Instead of this, please add calls in src/tools/miri/tests/pass/intrinsics/integer.rs that check the result.

(Same for funnel_shr)

We are already testing the const-eval implementation in library/coretests/tests/num/uint_macros.rs, do I need to add these tests too?

Ah, fair. But then what's the point of these non-failing calls here?

rustbot assigned tgross35 Aug 20, 2025

This comment has been minimized.

Sign in to view

folkertdev reviewed Aug 21, 2025

View reviewed changes

RalfJung reviewed Aug 21, 2025

View reviewed changes

library/core/src/intrinsics/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 21, 2025

View reviewed changes

compiler/rustc_const_eval/src/interpret/intrinsics.rs Outdated Show resolved Hide resolved

compiler/rustc_const_eval/src/interpret/intrinsics.rs Outdated Show resolved Hide resolved

compiler/rustc_const_eval/src/interpret/intrinsics.rs Outdated Show resolved Hide resolved

folkertdev reviewed Aug 22, 2025

View reviewed changes

library/core/src/intrinsics/mod.rs Show resolved Hide resolved

folkertdev mentioned this pull request Aug 22, 2025

ACP: add funnel shifts rust-lang/libs-team#642

Open

sayantn force-pushed the integer-funnel-shift branch from 81da1f2 to b91850b Compare August 22, 2025 16:18

This comment has been minimized.

Sign in to view

sayantn force-pushed the integer-funnel-shift branch from b91850b to 400a29d Compare August 22, 2025 16:30

This comment has been minimized.

Sign in to view

sayantn force-pushed the integer-funnel-shift branch from 400a29d to 23601d2 Compare August 22, 2025 16:37

This comment has been minimized.

Sign in to view

tgross35 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2025

bjorn3 reviewed Aug 27, 2025

View reviewed changes

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

sayantn force-pushed the integer-funnel-shift branch from 5f4c412 to 63fde15 Compare August 28, 2025 01:39

This comment has been minimized.

Sign in to view

sayantn force-pushed the integer-funnel-shift branch from 63fde15 to 3d0b3fb Compare August 28, 2025 22:58

This comment has been minimized.

Sign in to view

tgross35 requested changes Aug 28, 2025

View reviewed changes

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 28, 2025

sayantn force-pushed the integer-funnel-shift branch from 3d0b3fb to d1f3861 Compare August 30, 2025 00:38

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 2025

RalfJung reviewed Aug 30, 2025

View reviewed changes

clarfonthey reviewed Aug 30, 2025

View reviewed changes

library/core/src/intrinsics/fallback.rs Outdated Show resolved Hide resolved

clarfonthey reviewed Aug 30, 2025

View reviewed changes

compiler/rustc_codegen_llvm/src/intrinsic.rs Outdated Show resolved Hide resolved

clarfonthey reviewed Aug 30, 2025

View reviewed changes

library/core/src/intrinsics/fallback.rs Show resolved Hide resolved

sayantn and others added 3 commits September 2, 2025 06:38

Add funnel_sh{l,r} functions and intrinsics

009ba2d

- Add a fallback implementation for the intrinsics Co-Authored-By: folkertdev <folkert@folkertdev.nl>

Add LLVM backend support for funnel shifts

badf1bf

Add tests

c9bab92

Co-Authored-By: folkertdev <folkert@folkertdev.nl>

sayantn force-pushed the integer-funnel-shift branch from d1f3861 to c9bab92 Compare September 2, 2025 01:12

RalfJung reviewed Sep 2, 2025

View reviewed changes

	// SAFETY: this is guaranteed by the caller
	// This implementation is also used by Miri so we have to check the precondition.
	// SAFETY: this is guaranteed by the caller

		std::intrinsics::unchecked_funnel_shl(1_u32, 2, 5);
		std::intrinsics::unchecked_funnel_shl(1_u32, 2, 31);

Implement Integer funnel shifts #145690

Are you sure you want to change the base?

Implement Integer funnel shifts #145690

Conversation

sayantn commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 20, 2025

Uh oh!

rustbot commented Aug 20, 2025

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgross35 commented Aug 21, 2025

Uh oh!

Uh oh!

rustbot commented Aug 22, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Aug 24, 2025

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

sayantn commented Aug 28, 2025

Uh oh!

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

Uh oh!

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

sayantn Aug 30, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sayantn commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgross35 commented Aug 30, 2025

Uh oh!

clarfonthey commented Aug 30, 2025

Uh oh!

sayantn commented Aug 30, 2025

Uh oh!

RalfJung Aug 30, 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!

RalfJung Aug 30, 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!

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

sayantn commented Aug 20, 2025 •

edited

Loading

bjorn3 commented Aug 28, 2025 •

edited

Loading

sayantn Aug 30, 2025 •

edited

Loading

sayantn commented Aug 30, 2025 •

edited

Loading

RalfJung Aug 30, 2025 •

edited

Loading

RalfJung Aug 30, 2025 •

edited

Loading

RalfJung Sep 2, 2025 •

edited

Loading

RalfJung left a comment •

edited by rustbot

Loading