i128 and u128 support #37900

est31 · 2016-11-20T18:43:47Z

Adds support for 128-bit integers. Rebased version of #35954 , with additional improvements.

Thanks @nagisa for mentoring this PR!

Some of the problems that were resolved:

Confirm that intrinsics work on 32 bit platforms and fix them if needed
Wait for rustbuild: linker error during 32 bit compilation #37906 to get fixed, so that work on intrinsics can be completed (worked around by merging the PR on my local setup)
Investigate and fix linkcheck failure

[plugin-breaking-change]

cc #35118

rust-highfive · 2016-11-20T18:43:53Z

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

frewsxcv · 2016-11-20T22:09:23Z

src/libcore/num/i128.rs

+
+//! The 128-bit signed integer type.
+//!
+//! *[See also the `i128` primitive type](../../std/primitive.i128.html).*


This should maybe be:

[See also the `i128` primitive type](../primitive.i128.html)

@frewsxcv I think this wouldn't work. The other integer primitive types link via ../../std/primitive.$name.html as well, and I can't see generated doc files for them in libcore.

I think the problem is that the primitive.i128.html isn't being generated at all so changing this won't help.

@ollie27 yes, that is consistent with my observations, I can find primitive.i64.html at the linked path, but none for i128.

ollie27 · 2016-11-20T22:59:50Z

Investigate and fix linkcheck failure:

By the looks of it the files won't be generated. You'll need to add entries to src/libstd/primitive_docs.rs and update rustdoc a bit more. At least here and here will need to be updated.

bors · 2016-11-21T17:29:01Z

☔ The latest upstream changes (presumably #37824) made this pull request unmergeable. Please resolve the merge conflicts.

aturon · 2016-11-22T03:04:22Z

cc @rust-lang/compiler

est31 · 2016-11-22T05:35:02Z

Okay, running on 32 bit into a linker error of the form:

rust/src/librustc_const_math/int.rs:545: Nicht definierter Verweis auf `core::panicking::panic::hdb3be1030a0e450d'
rust/build/i686-unknown-linux-gnu/stage1-rustc/i686-unknown-linux-gnu/release/deps/rustc_const_math-fb45b0b7ff405aab.0.o: In Funktion `rustc_const_math::int::{{impl}}::rem':
rust/src/librustc_const_math/int.rs:577: Nicht definierter Verweis auf `core::panicking::panic::hdb3be1030a0e450d'
/tmp/rustc.8pXinEKP9SgF/libcompiler_builtins-83f89429e0eb597e.rlib(compiler_builtins-83f89429e0eb597e.0.o): In Funktion `compiler_builtins::reimpls::u128_div_mod':
rust/src/libcompiler_builtins/lib.rs:132: Nicht definierter Verweis auf `core::panicking::panic::hdb3be1030a0e450d'
collect2: error: ld returned 1 exit status

Apparently the __udivmodti4 intrinsic may panic. According to readelf, its the only one in the libcompiler_builtin library that calls panic. I've localized the error to the ashl and ashr macros inside the src/libcompiler_builtins/lib.rs file, and the error messages of the panics are "attempt to calculate the remainder with a divisor of zero" and "attempt to divide by zero" (found out with a very reliable and robust method involving a strings invocation and grepping for "rustc version" xD)...

I'm not sure whether or not I should panic here, and how to fix the linker issue if I should indeed panic.

Setting panic=abort for the libcompiler_builtin crate does not remove the panic invocations for some reason, nor the linker failure...

eddyb · 2016-11-22T11:44:05Z

@est31 -C panic=abort is a red herring, forget about it, it changes the runtime, not code generation. Sounds like all we have to do is either always compile libcompiler_builtins with -Z force-overflow-checks=off or explicitly use Wrapping and/or wrapping_add etc.

bors · 2016-11-23T03:58:20Z

☔ The latest upstream changes (presumably #37487) made this pull request unmergeable. Please resolve the merge conflicts.

mattico · 2016-11-23T17:10:37Z

The approach we've used so far in compiler-builtins is to use explicit wrapping operations everywhere. Given that we're mostly translating C code it may make sense to just use -Z force-overflow-checks=off.

Using the Wrapping type is annoying due to #23545 (see https://github.com/rust-lang/rust/blob/master/src/libcore/num/wrapping.rs#L103-L118), so if you want to use Wrapping everywhere you have to do things like x << y.0 as usize where x and y are both Wrapping(_).

cc @japaric

est31 · 2016-11-23T17:38:03Z

Progress update: I've been able to eliminate the linker errors by substituting the operations that could fail with unchecked_ versions of them. It makes sense as the C intrinsics do the same and we do the checking afterwards.

I've right now arrived at debugging the functionality of the actual intrinsics themselves... Right now the failure is with the i128 unit tests, when parsing the literals for very large integers, apparently it overflows which it shouldn't.

bors · 2016-11-24T09:38:10Z

☔ The latest upstream changes (presumably #37890) made this pull request unmergeable. Please resolve the merge conflicts.

nagisa · 2016-11-25T00:25:34Z

src/libcompiler_builtins/lib.rs

+        match a.signum() {
+            1 => u128_as_f64(a.abs() as u128_),
+            0 => 0.0,
+            -1 => -u128_as_f64(a.abs() as u128_),


a.abs() as u128_

This abs might be dangerous. Namely, the i128::min_value().abs() cannot be represented in i128 so it ought to panic, just like the i64 variation of the same does:

i64::min_value().abs() // thread 'main' panicked at 'attempt to negate with overflow', ../src/libcore/num/mod.rs:1207 // note: Run with `RUST_BACKTRACE=1` for a backtrace.

We might want some helper like abs : i128 -> u128 or some such for use here. Similar note may apply to other uses of abs as well.

Good catch, will do that!

It doesn't really work well for __muloti4 as you have to negate the value, but I've changed it everywhere else. For __muloti4 its no big deal either way as we already check it manually beforehand. To make sure that there is really no panic (LLVM may remove the call but it may not, which then will lead to a linker error later on) I've made a non panicking iabs method

est31 · 2016-11-25T02:36:59Z

Okay, the PR is ready for review. I've finished the work on intrinsics and wouldn't know of anything broken or which needs to be done. The tests succeed for both 64 bit and 32 bit platforms.

leonardo-m · 2016-11-25T18:19:15Z

I am quite happy for the future availability of 128 bit numbers in Rust. The "mul_mod" operation is used by "pow_mod" (modular pow), that is sufficiently common:

fn mul_mod(a: u64, b: u64, m: u64) -> (u64, u64);
fn mul_mod(a: u128, b: u128, m: u128) -> (u128, u128);

If you have 128 bits you can perform the 64 bit mul_mod using 128 bit values, but it's more efficient to use the "divq" instruction and work on 64 bit values. And you can probably perform the 128 bit mul_mod more efficiently without 256 bit numbers.

Are you going to support those mul_mod (or something similar), or should this enhancement request be moved elsewhere?

est31 · 2016-11-25T18:52:32Z

@leonardo-m I think providing modular pow is outside of the scope of this PR. Feel free to open an issue or RFC at https://github.com/rust-lang/rfcs . Also, you might be interested in https://github.com/rust-num/num

leonardo-m · 2016-11-25T19:54:59Z

My suggestion was about mul_mod (not about pow_mod), and I think it needs a low-level implementation with intrinsics or asm to be efficient. I'll probably open an issue here.

alexcrichton · 2016-11-25T23:30:25Z

r? @eddyb (feel free to transfer)

just as ceil(log10(2^64)) == 20

Current stage0 with version "1.14.0-beta.1 (d4aea2d 2016-11-15)" suffers from issue 37686. You can confirm this on master by running ./x.py test --stage 0 src/test/run-pass --test-args 37686 Add this workaround until there is a fix. No idea how it works, but it does work.

The check inside compiler-rt file int_types.h to #define CRT_HAS_128BIT looks like: #if (defined(__LP64__) || defined(__wasm__)) && \ !(defined(__mips__) && defined(__clang__)) #define CRT_HAS_128BIT #endif Windows uses LLP64 instead of LP64, so it doesn't ship with the C based intrinsics. Also, add libcompiler_builtins to the list of crates that may have platform specific checks (like the ones we just added).

* shift so that no panics are generated (otherwise results in linker error) * no_std as insurance to not get into issues with errors like "cannot satisfy dependencies so `rustc_i128` only shows up once" (pure guessing here, but it doesn't hurt...)

There was a linker error on 32 bit platforms with optimisations turned off, complaining that there was an undefined reference to "rust_eh_personality", when compiling the rustc_const_math as stage1 artifact. Apparently the compiler_builtins crate includes a call to "rust_eh_personality". If compiled for 64 bits, this call doesn't appear, which explains why the linker error only happens on 32 bit platforms, and optimisations will get it removed on 32 bit as well. There were two origins of the call: 1. A for loop where apparently the compiler wasn't sure whether next() could panic or not, and therefore generated a landing pad for the worst case. The minimal reproducible example is "for _ in 0..sr { }". 2. A default impl of uabs where the compiler apparently wasn't sure either whether iabs() could panic or not. Many thanks to nagisa for contributing the fix. This commit also puts extern "C" to the intrinsics, as this is generally a good thing to do.

Otherwise, we codegen panic calls which create problems with debug assertions turned on.

on suggestion by nagisa.

...this time with the float intrinsics.

est31 · 2016-12-15T18:43:44Z

Rebased. r? @eddyb

leonardo-m · 2016-12-15T18:47:22Z

Keep going! :-)

est31 · 2016-12-15T21:55:20Z

Release is close, and this may cause regressions. It'd be better to have it at the start of a release period, not that it gets reverted before release because of the regressions it caused. Gonna reopen once the release is over.

est31 · 2016-12-20T01:26:25Z

See #38482 for the continuation

DirkyJerky · 2016-12-20T05:24:55Z

And #37900 was such a rememberable number too 😢

i128 and u128 support Brings i128 and u128 support to nightly rust, behind a feature flag. The goal of this PR is to do the bulk of the work for 128 bit integer support. Smaller but just as tricky features needed for stabilisation like 128 bit enum discriminants are left for future PRs. Rebased version of #37900, which in turn was a rebase + improvement of #35954 . Sadly I couldn't reopen #37900 due to github. There goes my premium position in the homu queue... [plugin-breaking-change] cc #35118 (tracking issue)

rust-highfive assigned aturon Nov 20, 2016

frewsxcv reviewed Nov 20, 2016

View reviewed changes

est31 mentioned this pull request Nov 21, 2016

rustbuild: linker error during 32 bit compilation #37906

Closed

est31 force-pushed the i128 branch from c7737ee to 6d59d14 Compare November 21, 2016 02:00

est31 force-pushed the i128 branch from 4e8f6b1 to 976fcd8 Compare November 21, 2016 18:41

aturon removed their assignment Nov 22, 2016

est31 force-pushed the i128 branch from 976fcd8 to 9b377b4 Compare November 23, 2016 18:07

est31 force-pushed the i128 branch from 9b377b4 to 86208a5 Compare November 24, 2016 15:44

nagisa reviewed Nov 25, 2016

View reviewed changes

est31 force-pushed the i128 branch from 5f0c10d to 1b8b198 Compare November 25, 2016 01:33

est31 changed the title ~~[WIP] i128 and u128 support~~ i128 and u128 support Nov 25, 2016

rust-highfive assigned eddyb Nov 25, 2016

est31 force-pushed the i128 branch 2 times, most recently from 7115923 to 76784e1 Compare November 26, 2016 19:55

eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 27, 2016

est31 added 14 commits December 15, 2016 19:43

Move from RUSTC_CRATES to TARGET_CRATES

e08e34b

40 -> 39, as ceil(log10(2^128)) == 39

bcf7f93

just as ceil(log10(2^64)) == 20

Try to fix some things

1f2ed68

* shift so that no panics are generated (otherwise results in linker error) * no_std as insurance to not get into issues with errors like "cannot satisfy dependencies so `rustc_i128` only shows up once" (pure guessing here, but it doesn't hurt...)

Remove unimplemented() function

8958e7c

Port to wrapping_* and unchecked_* operations

c004f88

Otherwise, we codegen panic calls which create problems with debug assertions turned on.

Tidy

93c3584

Windows x64 ABI requires i128 params to be passed as reference

2175095

intrinsics: try to return everything via {u,i}128ret to match LLVM

93ee9f5

on suggestion by nagisa.

Fix another windows ABI mistake

dc57f52

...this time with the float intrinsics.

Tidy

a66b8b0

Fix rebase fallout

53925c4

est31 closed this Dec 15, 2016

est31 mentioned this pull request Dec 20, 2016

i128 and u128 support #38482

Merged

joewalker mentioned this pull request Jan 9, 2017

Add a basic EDN parser mozilla/mentat#149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i128 and u128 support #37900

i128 and u128 support #37900

est31 commented Nov 20, 2016 •

edited

Loading

rust-highfive commented Nov 20, 2016

frewsxcv Nov 20, 2016

est31 Nov 20, 2016 •

edited

Loading

ollie27 Nov 20, 2016

est31 Nov 20, 2016

ollie27 commented Nov 20, 2016

bors commented Nov 21, 2016

aturon commented Nov 22, 2016

est31 commented Nov 22, 2016

eddyb commented Nov 22, 2016

bors commented Nov 23, 2016

mattico commented Nov 23, 2016

est31 commented Nov 23, 2016

bors commented Nov 24, 2016

nagisa Nov 25, 2016 •

edited

Loading

est31 Nov 25, 2016

est31 Nov 25, 2016

est31 commented Nov 25, 2016 •

edited

Loading

leonardo-m commented Nov 25, 2016 •

edited

Loading

est31 commented Nov 25, 2016

leonardo-m commented Nov 25, 2016 •

edited

Loading

alexcrichton commented Nov 25, 2016

est31 commented Dec 15, 2016

leonardo-m commented Dec 15, 2016

est31 commented Dec 15, 2016

est31 commented Dec 20, 2016

DirkyJerky commented Dec 20, 2016 •

edited

Loading

i128 and u128 support #37900

i128 and u128 support #37900

Conversation

est31 commented Nov 20, 2016 • edited Loading

rust-highfive commented Nov 20, 2016

frewsxcv Nov 20, 2016

Choose a reason for hiding this comment

est31 Nov 20, 2016 • edited Loading

Choose a reason for hiding this comment

ollie27 Nov 20, 2016

Choose a reason for hiding this comment

est31 Nov 20, 2016

Choose a reason for hiding this comment

ollie27 commented Nov 20, 2016

bors commented Nov 21, 2016

aturon commented Nov 22, 2016

est31 commented Nov 22, 2016

eddyb commented Nov 22, 2016

bors commented Nov 23, 2016

mattico commented Nov 23, 2016

est31 commented Nov 23, 2016

bors commented Nov 24, 2016

nagisa Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

est31 Nov 25, 2016

Choose a reason for hiding this comment

est31 Nov 25, 2016

Choose a reason for hiding this comment

est31 commented Nov 25, 2016 • edited Loading

leonardo-m commented Nov 25, 2016 • edited Loading

est31 commented Nov 25, 2016

leonardo-m commented Nov 25, 2016 • edited Loading

alexcrichton commented Nov 25, 2016

est31 commented Dec 15, 2016

leonardo-m commented Dec 15, 2016

est31 commented Dec 15, 2016

est31 commented Dec 20, 2016

DirkyJerky commented Dec 20, 2016 • edited Loading

est31 commented Nov 20, 2016 •

edited

Loading

est31 Nov 20, 2016 •

edited

Loading

nagisa Nov 25, 2016 •

edited

Loading

est31 commented Nov 25, 2016 •

edited

Loading

leonardo-m commented Nov 25, 2016 •

edited

Loading

leonardo-m commented Nov 25, 2016 •

edited

Loading

DirkyJerky commented Dec 20, 2016 •

edited

Loading