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

saturating_add doc is not clear enough #64940

Closed
tesuji opened this issue Oct 1, 2019 · 3 comments · Fixed by #64943
Closed

saturating_add doc is not clear enough #64940

tesuji opened this issue Oct 1, 2019 · 3 comments · Fixed by #64943
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@tesuji
Copy link
Contributor

tesuji commented Oct 1, 2019

Godbolt link: https://godbolt.org/z/9rm4jF

So I expect these two functions has the same assembly:

pub fn foo(a: i32, b: i32) -> i32 {
    a.saturating_add(b)
}

pub fn bar(a: i32, b: i32) -> i32 {
    match a.checked_add(b) {
        Some(v) => v,
        None => i32::max_value(),
    }
}

But the result is not:

example::foo:
        xor     eax, eax
        mov     ecx, edi
        add     ecx, esi
        setns   al
        add     eax, 2147483647
        add     edi, esi
        cmovno  eax, edi
        ret

example::bar:
        add     edi, esi
        mov     eax, 2147483647
        cmovno  eax, edi
        ret
@tesuji

This comment has been minimized.

@rustbot rustbot added C-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 1, 2019
@hman523
Copy link
Contributor

hman523 commented Oct 1, 2019

Hey I tried solving this but wasn't really sure how to. I believe the implementation to the LLVM generation is right here.

"saturating_add" | "saturating_sub" => {
let is_add = name == "saturating_add";
let lhs = args[0].immediate();
let rhs = args[1].immediate();
if llvm_util::get_major_version() >= 8 {
let llvm_name = &format!("llvm.{}{}.sat.i{}",
if signed { 's' } else { 'u' },
if is_add { "add" } else { "sub" },
width);
let llfn = self.get_intrinsic(llvm_name);
self.call(llfn, &[lhs, rhs], None)
} else {
let llvm_name = &format!("llvm.{}{}.with.overflow.i{}",
if signed { 's' } else { 'u' },
if is_add { "add" } else { "sub" },
width);
let llfn = self.get_intrinsic(llvm_name);
let pair = self.call(llfn, &[lhs, rhs], None);
let val = self.extract_value(pair, 0);
let overflow = self.extract_value(pair, 1);
let llty = self.type_ix(width);
let limit = if signed {
let limit_lo = self.const_uint_big(
llty, (i128::MIN >> (128 - width)) as u128);
let limit_hi = self.const_uint_big(
llty, (i128::MAX >> (128 - width)) as u128);
let neg = self.icmp(
IntPredicate::IntSLT, val, self.const_uint(llty, 0));
self.select(neg, limit_hi, limit_lo)
} else if is_add {
self.const_uint_big(llty, u128::MAX >> (128 - width))
} else {
self.const_uint(llty, 0)
};
self.select(overflow, limit, val)
}
},

Hope someone can solve this!

@tesuji
Copy link
Contributor Author

tesuji commented Oct 1, 2019

Turn out I was wrong! The saturating_add is working correctly (i.e. those two functions
are not equivalent): https://github.com/rust-lang/rust/pull/58003/files#diff-01076f91a26400b2db49663d787c2576R885-R891

I would add some doctests to show that and close this issue.

@tesuji tesuji changed the title saturating_add has overhead? saturating_add doc is not clear enough Oct 1, 2019
@rustbot rustbot removed C-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code. labels Oct 1, 2019
@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 1, 2019
Centril added a commit to Centril/rust that referenced this issue Oct 1, 2019
Add lower bound doctests for `saturating_{add,sub}` signed ints

Closes rust-lang#64940
@bors bors closed this as completed in 66148f6 Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants