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

Do not turn on LLVM intrinsic dependency by default #206

Open
shamatar opened this issue Jun 14, 2021 · 3 comments
Open

Do not turn on LLVM intrinsic dependency by default #206

shamatar opened this issue Jun 14, 2021 · 3 comments

Comments

@shamatar
Copy link

shamatar commented Jun 14, 2021

At the moment build script turns on a feature that uses LLVM instrinsics for additions. Unfortunately cranelift backend does not support those instinsics (and potentially other) yet, so can it be moved to an explicit feature?

num-bigint/build.rs

Lines 27 to 30 in 125fbbd

let addcarry = format!("{}::arch::{}::_addcarry_{}", std, target_arch, digit);
if ac.probe_path(&addcarry) {
autocfg::emit("use_addcarry");
}

Also, one can avoid calling intrinsics at all. E.g. this PR to the Rust itself rust-lang/rust#85017 introduces a function that optimized to add/adc chain by LLVM and does not use any intrinsics at all (copying for completeness)

#[inline]
pub const fn carrying_add(self, rhs: u64, carry: bool) -> (u64, bool) {
    // note: longer-term this should be done via an intrinsic, but this has been shown
    //   to generate optimal code for now, and LLVM doesn't have an equivalent intrinsic
    let (a, b) = self.overflowing_add(rhs);
    let (c, d) = a.overflowing_add(carry as u64);
    (c, b | d)
}
@cuviper
Copy link
Member

cuviper commented Jun 14, 2021

If we can structure the code such that LLVM does a better job itself, that would be far more preferable than using intrinsics.

@cuviper
Copy link
Member

cuviper commented Jul 7, 2021

I just found that cargo miri doesn't like the intrinsic calls either:

error: unsupported operation: can't call foreign function: llvm.x86.addcarry.64
   --> [...]/core/src/../../stdarch/crates/core_arch/src/x86_64/adx.rs:21:18
    |
21  |     let (a, b) = llvm_addcarry_u64(c_in, a, b);
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't call foreign function: llvm.x86.addcarry.64
    |
    = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support

    = note: inside `core::arch::x86_64::_addcarry_u64` at [...]/core/src/../../stdarch/crates/core_arch/src/x86_64/adx.rs:21:18
note: inside `biguint::addition::adc` at src/biguint/addition.rs:24:14
   --> src/biguint/addition.rs:24:14
    |
24  |     unsafe { arch::_addcarry_u64(carry, a, b, out) }
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@clarfonthey
Copy link
Contributor

Fun story, me making that change was explicitly the result of looking at the implementation here in this crate and wanting the code to be more easily usable without relying on intrinsics. Hopefully at some point in the future we can lower the code to a reliable Rust intrinsic that works properly on all targets.

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

No branches or pull requests

3 participants