Make saturating_add and saturating_sub const functions

[oli-obk]

These used to require if to work, but are forwarded to llvm intrinsics nowadays (#58003). We can now implement them as const eval intrinsics, too.

Impl instructions:

1. add to

   rust/src/librustc_mir/transform/qualify_min_const_fn.rs

   Line 371 in 0c0c585

   | "add_with_overflow" // ~> .overflowing_add

2. remove unsafe for calls to the intrinsic
3. add to

   rust/src/librustc_mir/transform/qualify_consts.rs

   Line 820 in 51cc3cd

   | "add_with_overflow"

4. make functions calling the intrinsic const fn
5. Implement in

   rust/src/librustc_mir/interpret/intrinsics.rs

   Line 102 in 79d8a0f

   | "overflowing_add"

   (implementation is basically the two read_immediate calls, a call to binary_op_imm with arguments similar to the other add/sub intrinsics, in case the overflow bool in the return value is false, write the actual value to the destination like in

   rust/src/librustc_mir/interpret/operator.rs

   Line 36 in 79d8a0f

   self.write_scalar(val, dest)

   . In case the overflow bool is true, compute the min or max value of the type like in

   rust/src/librustc_mir/interpret/operator.rs

   Line 217 in 79d8a0f

   let max = 1 << (size.bits() - 1);

   and write that value to dest.
6. Write some tests that use the saturating methods in constants

[pmccarter]

I'd really like to do this!

[oli-obk]

It's all yours! If anything is unclear, don't hesitate to ask.

[pmccarter]

Thanks! I have some questions, please let me know if I'm misunderstanding anything...

Is it correct that the point of this ticket is to avoid code generation (LLVM intrinsics) by evaluating during compilation (const eval intrinsics), where previously LLVM branching code was generated? Is there already compiler logic to detect non immediate arguments/terms so that the LLVM code is generated even after the functions are whitelisted as const?

I can't find any compiler code where calls to n.saturating_add(m) are wrapped in unsafe blocks, and why would they be? Also, are the implementation guidelines saying that all functions with calls to these two functions can be marked as const? Why would that be, irregardless of other statements in the function?

[nikic]

Is it correct that the point of this ticket is to avoid code generation (LLVM intrinsics) by evaluating during compilation (const eval intrinsics), where previously LLVM branching code was generated? Is there already compiler logic to detect non immediate arguments/terms so that the LLVM code is generated even after the functions are whitelisted as const?

Not quite. For normal code, the LLVM intrinsics will still be generated (at which point LLVM may evaluate them during optimization, but that's a different matter). Making the intrinsics const just means that they can be used in contexts where the result must be evaluated during compilation. It allows you to write something like const N = A.saturating_add(B);.

I can't find any compiler code where calls to n.saturating_add(m) are wrapped in unsafe blocks, and why would they be?

You need to separate two things here: The saturating_add methods on numeric types like u32, and the saturating_add intrinsic in core::intrinsics. By default all such intrinsics are unsafe, but for saturating_add in particular there is no reason for it to be. (I think this is already the case -- though maybe this has to be marked separate for const handling.)

Also, are the implementation guidelines saying that all functions with calls to these two functions can be marked as const? Why would that be, irregardless of other statements in the function?

This refers to the functions that call the intrinsics, which should just be the saturing_add/saturating_sub methods in core::num (such as https://github.com/rust-lang/rust/blob/master/src/libcore/num/mod.rs#L885). Those are the only functions that should be marked as const.

[RalfJung]

Is it correct that the point of this ticket is to avoid code generation (LLVM intrinsics) by evaluating during compilation (const eval intrinsics), where previously LLVM branching code was generated?

No. This is to make the CTFE engine in Rust support these intrinsics. When you write const FOO: T = bar; or static FOO: T = bar;, Rust has to evaluate bar at compile-time, that's called CTFE (compile-time function evaluation). And the point of this issue is to enable bar to use saturating_add, to make code like this work:

fn main() {
    const x: u8 = 13u8.saturating_sub(66);
}

CTFE is very different from const propagation, which is an optimization that opportunistically will try to evaluate code at compile-time. Const propagation is performed by LLVM, rustc does not have to do anything for it to happen.

[pmccarter]

Thanks for the help guys.

Currently the num module saturating_add() wrapper only uses the intrinsic for stage 1+, so the function can't just be marked as const because of the non const checked_add() call in the stage 0 compiler. Looks like other intrinsic calls don't need the stage guards so not sure what the issue is. Does this need to wait for the bootstrapping compile version to advance or something?

Also not sure about the special // ~> comments...

[nikic]

@pmccarter I believe the way to handle this would be to move the cfg to cover the whole function. You'd have a stage0 variant that is non-const and also uses the old checked_add based implementation, and you'd have a not(stage0) variant that is const and uses the saturating_add intrinsic based implementation.

So basically instead of

pub fn saturating_add(self, rhs: Self) -> Self {
    #[cfg(stage0)]
    match self.checked_add(rhs) {
        Some(x) => x,
        None => Self::max_value(),
    }
    #[cfg(not(stage0))]
    {
        intrinsics::saturating_add(self, rhs)
    }
}

you'd have

#[cfg(stage0)]
pub fn saturating_add(self, rhs: Self) -> Self {
    match self.checked_add(rhs) {
        Some(x) => x,
        None => Self::max_value(),
    }
}

#[cfg(not(stage0))]
pub const fn saturating_add(self, rhs: Self) -> Self {
    intrinsics::saturating_add(self, rhs)
}