Skip to content

Conversation

@EFanZh
Copy link
Contributor

@EFanZh EFanZh commented Nov 22, 2025

This PR adds hints that the return value of T::ilog10 will never exceed T::MAX.ilog10().

This works because ilog10 is a monotonically nondecreasing function, the maximum return value is reached at the max input value.

@rustbot 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 22, 2025

r? @Mark-Simulacrum

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

@Noratrieb
Copy link
Member

Is this motivated by concrete codegen improvements (the codegen tests are quite artificial)? assume/assert_unchecked always has the potential to go either way wrt performance, so it would be good to know whether you've confirmed that this improves real-world code

@jhorstmann
Copy link
Contributor

Interesting that llvm can already infer some bounds, but apparently not the tightest ones. Looking at the following functions in compiler explorer, there are no panics:

pub fn test_u8(i: std::num::NonZeroU8, a: &[u32; 3]) -> u32 {
    a[i.ilog10() as usize]
}

pub fn test_u16(i: std::num::NonZeroU16, a: &[u32; 8]) -> u32 {
    a[i.ilog10() as usize]
}

pub fn test_u32(i: std::num::NonZeroU32, a: &[u32; 13]) -> u32 {
    a[i.ilog10() as usize]
}

pub fn test_u64(i: std::num::NonZeroU64, a: &[u32; 23]) -> u32 {
    a[i.ilog10() as usize]
}

These functions with tighter bounds would make for nicer codegen test.

@EFanZh
Copy link
Contributor Author

EFanZh commented Nov 23, 2025

@Noratrieb: An example would be getting a slice of digits of an integer where adding an assertion can remove bound checking code:

pub fn get_digits(mut x: u32, buffer: &mut [u8; 10]) -> &mut [u8] {
    let digits = &mut buffer[..x.checked_ilog10().unwrap_or_default() as usize + 1];

    for slot in &mut *digits {
        *slot = (x % 10) as u8;
        x /= 10;
    }

    digits
}

You can compare the the codegen results here.

Also, isqrt already have a similar assertion like this:

pub const fn isqrt(self) -> Self {
let result = crate::num::int_sqrt::$ActualT(self as $ActualT) as $SelfT;
// Inform the optimizer what the range of outputs is. If testing
// `core` crashes with no panic message and a `num::int_sqrt::u*`
// test failed, it's because your edits caused these assertions or
// the assertions in `fn isqrt` of `nonzero.rs` to become false.
//
// SAFETY: Integer square root is a monotonically nondecreasing
// function, which means that increasing the input will never
// cause the output to decrease. Thus, since the input for unsigned
// integers is bounded by `[0, <$ActualT>::MAX]`, sqrt(n) will be
// bounded by `[sqrt(0), sqrt(<$ActualT>::MAX)]`.
unsafe {
const MAX_RESULT: $SelfT = crate::num::int_sqrt::$ActualT(<$ActualT>::MAX) as $SelfT;
crate::hint::assert_unchecked(result <= MAX_RESULT);
}
result
}

I think it is reasonable that we do the same on ilog10, otherwise it is difficult to explain why is there behavior difference between isqrt and ilog10.

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

rustbot commented Nov 26, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@EFanZh EFanZh force-pushed the add-ilog10-result-range-hints branch from 1ce044b to d0206df Compare November 28, 2025 18:37
@rustbot

This comment has been minimized.

@EFanZh EFanZh force-pushed the add-ilog10-result-range-hints branch 2 times, most recently from d4dd0ab to 7a03ee3 Compare November 29, 2025 01:25
@rustbot
Copy link
Collaborator

rustbot commented Nov 29, 2025

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.

@rust-log-analyzer

This comment has been minimized.

@EFanZh EFanZh force-pushed the add-ilog10-result-range-hints branch from 7a03ee3 to 4046385 Compare November 29, 2025 01:52
@EFanZh
Copy link
Contributor Author

EFanZh commented Nov 29, 2025

@rustbot ready.

@rustbot 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 Nov 29, 2025
@Mark-Simulacrum
Copy link
Member

Given the precedent with sqrt I'll go ahead and approve this. I checked and neither the compiler nor rustc-perf seems to have any meaningful use of ilog10 so our perf suite is probably not capable of determining negative compile-time impact (which is one of the tradeoffs here).

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 8, 2025

📌 Commit 4046385 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors 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 8, 2025
bors added a commit that referenced this pull request Dec 8, 2025
Rollup of 8 pull requests

Successful merges:

 - #148935 (Fix division syntax in doc comments)
 - #149207 (Add `ilog10` result range hints)
 - #149676 (Tidying up tests/ui/issues tests [3/N])
 - #149710 (Move ambient gdb discovery from compiletest to bootstrap)
 - #149714 (Check associated type where-clauses for lifetimes)
 - #149722 (contracts: fix lowering final declaration without trailing semicolon)
 - #149736 (contracts: clean up feature flag warning duplicated across tests)
 - #149739 (mailmap: add binarycat)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ac729a4 into rust-lang:main Dec 8, 2025
11 checks passed
@rustbot rustbot added this to the 1.94.0 milestone Dec 8, 2025
rust-timer added a commit that referenced this pull request Dec 8, 2025
Rollup merge of #149207 - EFanZh:add-ilog10-result-range-hints, r=Mark-Simulacrum

Add `ilog10` result range hints

This PR adds hints that the return value of `T::ilog10` will never exceed `T::MAX.ilog10()`.

This works because `ilog10` is a monotonically nondecreasing function, the maximum return value is reached at the max input value.
@EFanZh EFanZh deleted the add-ilog10-result-range-hints branch December 8, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants