Add intrinsics for integer minimum and maximum - #161081
Open
scottmcm wants to merge 1 commit into
Open
Conversation
rustbot
added
A-LLVM
scottmcm
commented
Aug 14, 2026
Comment on lines
+1845
to
+1848
| #[miri::fallback_is_spec] | ||
| pub const fn integer_min<T: Copy + [const] PartialOrd>(a: T, b: T) -> T { | ||
| if a < b { a } else { b } | ||
| } |
Member
Author
There was a problem hiding this comment.
cc @RalfJung because I said miri::fallback_is_spec
Member
There was a problem hiding this comment.
Odd, why did the bot not ping me...
... ah, because it's a draft. :)
Member
Author
There was a problem hiding this comment.
Yeah, because I know my odds of getting a working PR first push are about 2% on a good day 🙃
EDIT: oh, and also I managed to spell it wrong 🤦
Member
There was a problem hiding this comment.
LGTM apart from the comment nit. :)
This comment has been minimized.
This comment has been minimized.
scottmcm
force-pushed
the
min-max-intrinsics
branch
from
August 14, 2026 08:07
0392eb2 to
2560466
Compare
This comment has been minimized.
This comment has been minimized.
scottmcm
force-pushed
the
min-max-intrinsics
branch
2 times, most recently
from
August 14, 2026 16:59
2a35cda to
56bf46d
Compare
scottmcm
marked this pull request as ready for review
August 14, 2026 18:25
rustbot
added
the
S-waiting-on-review
Collaborator
|
Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr
cc @bjorn3 Any special-casing of Miri in the standard library requires review. cc @rust-lang/miri
cc @rust-lang/miri |
rustbot
removed
the
S-waiting-on-author
Collaborator
|
r? @clarfonthey rustbot has assigned @clarfonthey. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Member
Author
|
Given that LLVM can collapse all the mess this probably won't show much of a difference, but might as well make sure it's at least not worse somehow |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
rust-bors Bot
pushed a commit
that referenced
this pull request
Aug 14, 2026
Add intrinsics for integer minimum and maximum
RalfJung
reviewed
Aug 14, 2026
| #[rustc_nounwind] | ||
| #[rustc_intrinsic] | ||
| #[miri::intrinsic_fallback_is_spec] | ||
| pub const fn integer_min<T: Copy + [const] PartialOrd>(a: T, b: T) -> T { |
Member
There was a problem hiding this comment.
The name says integer but the type signature does not. If there are constraints on the type that go beyond the signature, please spell them out in the doc comment.
Member
Author
There was a problem hiding this comment.
Do you have thoughts about doing it via a trait?
Like we have FloatPrimitive I could add IntegerPrimitive and bound it that way.
That exists for fallbacks more than for just type checking, though, so I don't know if it's worth doing here vs just documenting it.
Edit: Oh, actually, I think I might as well do that because then the Ord requirement can come from that trait instead of listing it out.
Member
Author
There was a problem hiding this comment.
Done, with both a bound and a doc-comment update.
Contributor
clarfonthey
reviewed
Aug 14, 2026
Contributor
|
I'm happy with this and would be fine merging once you make whatever fallback changes you were mentioning. I'm not on the compiler team, but just from the perspective of optimising the giant pile of operations we have in libstd, adding more intrinsics for "obvious" primitives like this is fine, even if they're technically redundant. For example, #161069 which also fell under my review recommended adding some potential additional reasoning for |
clarfonthey
reviewed
Aug 14, 2026
|
|
||
| other => { | ||
| tcx.dcx().emit_err(UnrecognizedIntrinsicFunction { span, name: other }); | ||
| tcx.dcx().emit_err(UnrecognizedIntrinsicFunction { span, name: other, file: file!() }); |
Contributor
There was a problem hiding this comment.
I can definitely tell that this was motivated by you forgetting which file this was when you added this intrinsic. :p
Member
There was a problem hiding this comment.
Yeah, this is where span_bug! might be better than emit_err. ;)
I don't know why we bother with "pretty" errors for intrinsic misuse anyway...
clarfonthey
reviewed
Aug 14, 2026
Member
Author
|
Curious, the build didn't get queued |
This comment has been minimized.
This comment has been minimized.
Contributor
|
@rust-timer build 8764a9b |
Member
Author
|
Oh, locally and in CI it uses a trimmed path, but that job got I'll just remove that part of this PR (and maybe change it to an ICE later). |
scottmcm
force-pushed
the
min-max-intrinsics
branch
from
August 30, 2026 08:07
564405d to
7519f42
Compare
Collaborator
|
This PR was rebased onto a different main 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. |
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Member
Author
|
@bors r=clarfonthey |
Contributor
rust-bors
Bot
added
S-waiting-on-bors
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Aug 30, 2026
Add intrinsics for integer minimum and maximum
I got inspired to do this when looking at `SliceOrd::compare` where I was reminded that `if a < b { a } else { b }` isn't great in MIR since it takes 4 BBs. Looking at the codegen side, it turns out we currently emit [42 lines of LLVM-IR including 4 `alloca`s](https://rust.godbolt.org/z/ha4h4nT3r) for `u16::max` (pre-optimization), which is also unnecessarily bad†.
But both LLVM and Cranelift have dedicated things for min & max:
- https://llvm.org/docs/LangRef.html#llvm-umax-intrinsic
- https://docs.rs/cranelift-codegen/latest/cranelift_codegen/ir/trait.InstBuilder.html#method.smin
so let's just use those directly!
This actually wouldn't have been worth doing originally, but a couple of things have happened to change that:
- Back in 1.0 there was only `cmp::min` & `cmp::max`, so there was no place to actually do this at all, but in 2017 they were added to `Ord` as overridable things #25663 (comment)
- LLVM originally used icmp+select for these, not a dedicated construct, but then added one and as of 2022 the intrinsic is fully usable https://www.npopov.com/2022/12/20/This-year-in-LLVM-2022.html#integer-minmax-intrinsics
- Before we had intrinsic fallback this would have been more annoying to support everywhere -- GCC, [128-bit numbers on cg_clif](bytecodealliance/wasmtime#13790), CTFE, anything out-of-tree -- but now that we can write the obvious fallback we don't need to worry about that.
- The intrinsic would have helped less when it forced extra BBs and `alloca`s in codegen anyway, but [now](rust-lang/compiler-team#970) we can keep the result in SSA without needing to make it a primitive.
† Admittedly we could clean up the gratuitous badness there without needing an intrinsic, but I like doing the intrinsic anyway because that's the only way to avoid it always being stuck in the non-SSA path from the multi-BB assignments. Even if we made it inlineable, GVN and such will still just give up on seeing the `x = if a < b { a } else { b }` because it's multiple assignments to the same Local, which is non-ideal for something primitive-like.
Member
Contributor
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #162016. |
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Aug 30, 2026
Add intrinsics for integer minimum and maximum
I got inspired to do this when looking at `SliceOrd::compare` where I was reminded that `if a < b { a } else { b }` isn't great in MIR since it takes 4 BBs. Looking at the codegen side, it turns out we currently emit [42 lines of LLVM-IR including 4 `alloca`s](https://rust.godbolt.org/z/ha4h4nT3r) for `u16::max` (pre-optimization), which is also unnecessarily bad†.
But both LLVM and Cranelift have dedicated things for min & max:
- https://llvm.org/docs/LangRef.html#llvm-umax-intrinsic
- https://docs.rs/cranelift-codegen/latest/cranelift_codegen/ir/trait.InstBuilder.html#method.smin
so let's just use those directly!
This actually wouldn't have been worth doing originally, but a couple of things have happened to change that:
- Back in 1.0 there was only `cmp::min` & `cmp::max`, so there was no place to actually do this at all, but in 2017 they were added to `Ord` as overridable things #25663 (comment)
- LLVM originally used icmp+select for these, not a dedicated construct, but then added one and as of 2022 the intrinsic is fully usable https://www.npopov.com/2022/12/20/This-year-in-LLVM-2022.html#integer-minmax-intrinsics
- Before we had intrinsic fallback this would have been more annoying to support everywhere -- GCC, [128-bit numbers on cg_clif](bytecodealliance/wasmtime#13790), CTFE, anything out-of-tree -- but now that we can write the obvious fallback we don't need to worry about that.
- The intrinsic would have helped less when it forced extra BBs and `alloca`s in codegen anyway, but [now](rust-lang/compiler-team#970) we can keep the result in SSA without needing to make it a primitive.
† Admittedly we could clean up the gratuitous badness there without needing an intrinsic, but I like doing the intrinsic anyway because that's the only way to avoid it always being stuck in the non-SSA path from the multi-BB assignments. Even if we made it inlineable, GVN and such will still just give up on seeing the `x = if a < b { a } else { b }` because it's multiple assignments to the same Local, which is non-ideal for something primitive-like.
Member
|
@bors yield |
Contributor
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #162028. |
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Aug 31, 2026
Add intrinsics for integer minimum and maximum
I got inspired to do this when looking at `SliceOrd::compare` where I was reminded that `if a < b { a } else { b }` isn't great in MIR since it takes 4 BBs. Looking at the codegen side, it turns out we currently emit [42 lines of LLVM-IR including 4 `alloca`s](https://rust.godbolt.org/z/ha4h4nT3r) for `u16::max` (pre-optimization), which is also unnecessarily bad†.
But both LLVM and Cranelift have dedicated things for min & max:
- https://llvm.org/docs/LangRef.html#llvm-umax-intrinsic
- https://docs.rs/cranelift-codegen/latest/cranelift_codegen/ir/trait.InstBuilder.html#method.smin
so let's just use those directly!
This actually wouldn't have been worth doing originally, but a couple of things have happened to change that:
- Back in 1.0 there was only `cmp::min` & `cmp::max`, so there was no place to actually do this at all, but in 2017 they were added to `Ord` as overridable things #25663 (comment)
- LLVM originally used icmp+select for these, not a dedicated construct, but then added one and as of 2022 the intrinsic is fully usable https://www.npopov.com/2022/12/20/This-year-in-LLVM-2022.html#integer-minmax-intrinsics
- Before we had intrinsic fallback this would have been more annoying to support everywhere -- GCC, [128-bit numbers on cg_clif](bytecodealliance/wasmtime#13790), CTFE, anything out-of-tree -- but now that we can write the obvious fallback we don't need to worry about that.
- The intrinsic would have helped less when it forced extra BBs and `alloca`s in codegen anyway, but [now](rust-lang/compiler-team#970) we can keep the result in SSA without needing to make it a primitive.
† Admittedly we could clean up the gratuitous badness there without needing an intrinsic, but I like doing the intrinsic anyway because that's the only way to avoid it always being stuck in the non-SSA path from the multi-BB assignments. Even if we made it inlineable, GVN and such will still just give up on seeing the `x = if a < b { a } else { b }` because it's multiple assignments to the same Local, which is non-ideal for something primitive-like.
rust-bors
Bot
added
S-waiting-on-review
Contributor
|
💔 Test for b59ba1f failed: CI. Failed job:
|
|
(As an observer trying to understand the Rust build system, is this kind of failure known/expected to happen sometimes? Segfaulting on installing sccache seems like a very strange way to fail) |
Collaborator
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
Contributor
|
There are lots of spurious issues with GitHub actions, unfortunately. Whenever they come up, we just retry the job and hope it doesn't happen again. |
Contributor
|
@bors try jobs=x86_64-msvc-1 |
This comment has been minimized.
This comment has been minimized.
rust-bors Bot
pushed a commit
that referenced
this pull request
Aug 31, 2026
Add intrinsics for integer minimum and maximum try-job: x86_64-msvc-1
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
12 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
I got inspired to do this when looking at
SliceOrd::comparewhere I was reminded thatif a < b { a } else { b }isn't great in MIR since it takes 4 BBs. Looking at the codegen side, it turns out we currently emit 42 lines of LLVM-IR including 4allocas foru16::max(pre-optimization), which is also unnecessarily bad†.But both LLVM and Cranelift have dedicated things for min & max:
so let's just use those directly!
This actually wouldn't have been worth doing originally, but a couple of things have happened to change that:
cmp::min&cmp::max, so there was no place to actually do this at all, but in 2017 they were added toOrdas overridable things Tracking issue for Ord::{min, max} #25663 (comment)allocas in codegen anyway, but now we can keep the result in SSA without needing to make it a primitive.† Admittedly we could clean up the gratuitous badness there without needing an intrinsic, but I like doing the intrinsic anyway because that's the only way to avoid it always being stuck in the non-SSA path from the multi-BB assignments. Even if we made it inlineable, GVN and such will still just give up on seeing the
x = if a < b { a } else { b }because it's multiple assignments to the same Local, which is non-ideal for something primitive-like.Done without LLMs.