Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hint to value() #48

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Add hint to value() #48

merged 1 commit into from
Dec 2, 2024

Conversation

jordens
Copy link
Contributor

@jordens jordens commented Dec 2, 2024

The compiler is typically not smart enough to optimize away bounds checks if the arbitrary-int new()/value() API is not optimized away.

E.g. take this example:

#![no_std]

pub struct U<const N: usize>(u32);

impl<const N: usize> U<N> {
    const MAX: u32 = u32::MAX >> (32 - N);
    
    #[inline]
    pub const fn new(value: u32) -> Self {
        assert!(value <= Self::MAX);
        Self(value)
    }

    #[inline]
    pub const fn value(&self) -> u32 {
        // unsafe { core::hint::assert_unchecked(self.0 < 1 << N); }
        self.0
    }
}

#[inline(never)]
fn f(x: U<2>) -> i32 {
    const A: [i32; 4] = [1,8,9,0];
    A[x.value() as usize]
}

pub fn g(x: u32) -> i32 {
    f(U::new(x))
}

https://rust.godbolt.org/z/cYx6j666W

f() does a full bounds check even though U<2> is known to be an in-bounds index for a len-4 array.

Adding the soundness promise that the return of value() does not exceed MAX (matching the assert in new()) enables the compiler to optimize away the check regardless of the value of the argument.

This is currently behind a feature gate (hint) but IMO it could just be blanket-added since the point of the crate is precisely to make that promise.

@danlehmann
Copy link
Owner

Thanks a lot! I didn't know about this assert, but this is really great to know.

I'm testing the impact on binary size on a fairly large project with a lot of arbitrary-int usage. I'm sure there are some nice gains to be had with this.

One issue I see though: The most common place to access value() is I think through the const fn value() is line 133, which isn't covered by your patch. I'm trying to add that, but that does require a tiny breach of API contract (it's in a part where no bounds on T exist, so we can't compare). This might require a bump to a 2.0, eventhough I can't imagine anyone would want to use UInt with T not being a primitive int

@danlehmann
Copy link
Owner

I think we can use the hint cfg check as a sneaky way to get a small api change in. If it is present, const fn value can move to be a peer of const fn try_new(). I can upload that patch on top of yours

@danlehmann danlehmann self-requested a review December 2, 2024 19:12
@danlehmann danlehmann merged commit 680553d into danlehmann:main Dec 2, 2024
@danlehmann
Copy link
Owner

See #49 for the follow-up patch

@jordens
Copy link
Contributor Author

jordens commented Dec 3, 2024

Oh yes. I completely missed that. Thanks!

@jordens jordens deleted the hint branch December 3, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants