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

i128 and u128 support #38482

Merged
merged 47 commits into from
Dec 31, 2016
Merged

i128 and u128 support #38482

merged 47 commits into from
Dec 31, 2016

Conversation

est31
Copy link
Member

@est31 est31 commented Dec 20, 2016

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)

@est31
Copy link
Member Author

est31 commented Dec 20, 2016

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned alexcrichton Dec 20, 2016
@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@eddyb
Copy link
Member

eddyb commented Dec 20, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Dec 20, 2016

📌 Commit 84fd6c3 has been approved by eddyb

@est31
Copy link
Member Author

est31 commented Dec 20, 2016

I've removed the workaround for #37686 as the bootstrap compiler got updated.

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Dec 20, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Dec 20, 2016

📌 Commit 1abfc5c has been approved by eddyb

@eddyb
Copy link
Member

eddyb commented Dec 20, 2016

@bors p=100 (to avoid bitrotting)

@retep998
Copy link
Member

Sadly I couldn't reopen #37900 due to github.

If you push to a PR branch while the PR is closed, github won't let you reopen it. You have to reopen it first, then push the new version.

@est31
Copy link
Member Author

est31 commented Dec 20, 2016

@retep998 yes, found that out after I've already pushed it. :/

Copy link
Contributor

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the diff out of curiosity and was surprised by this.

-0x0000_0000_0000_8000...0x0000_0000_0000_7fff => I16,
-0x0000_0000_8000_0000...0x0000_0000_7fff_ffff => I32,
-0x8000_0000_0000_0000...0x7fff_ffff_ffff_ffff => I64,
_ => I128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match argument is an i64 there, I128 will never be returned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to actually add the "proper" case, but have the *128 case return a *64 instead, with a comment linking to the relevant issue?

0...0x0000_0000_0000_ffff => I16,
0...0x0000_0000_ffff_ffff => I32,
0...0xffff_ffff_ffff_ffff => I64,
_ => I128,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match argument is an u64 there, I128 will never be returned.

@bors
Copy link
Contributor

bors commented Dec 20, 2016

⌛ Testing commit 1abfc5c with merge 764d2f6...

@bors
Copy link
Contributor

bors commented Dec 20, 2016

💔 Test failed - auto-mac-32-opt

@alexcrichton
Copy link
Member

alexcrichton commented Dec 20, 2016 via email

@alexcrichton
Copy link
Member

@bors: r-

Looking at some travis logs it looks like this doesn't compile on 32-bit maybe? I think the error is:

error[E0053]: method `to_ret` has an incompatible type for trait
   --> /checkout/src/libcompiler_builtins/lib.rs:505:28
    |
426 |         fn to_ret(self) -> Self::Ret;
    |                            --------- type in trait
...
505 |         fn to_ret(self) -> u128ret {
    |                            ^^^^^^^ expected i128, found u128
error: aborting due to previous error

@bors
Copy link
Contributor

bors commented Dec 21, 2016

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

@est31
Copy link
Member Author

est31 commented Dec 21, 2016

Rebased. r? @eddyb

@eddyb
Copy link
Member

eddyb commented Dec 21, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Dec 21, 2016

📌 Commit 5b4fe10 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Dec 21, 2016

⌛ Testing commit 5b4fe10 with merge 1602354...

@bors
Copy link
Contributor

bors commented Dec 21, 2016

💔 Test failed - auto-win-msvc-32-opt

@est31
Copy link
Member Author

est31 commented Dec 21, 2016

Ok, this seems like another windows ABI issue with the intrinsics, this time on 32 bit. I'm trying to reproduce the issue right now.

Fixing the bug is a matter of finding out what LLVM expects as ABI in this case and replicating it in Rust.

@bors
Copy link
Contributor

bors commented Dec 31, 2016

📌 Commit 29e01af has been approved by eddyb

@bors
Copy link
Contributor

bors commented Dec 31, 2016

⌛ Testing commit 29e01af with merge 38bd207...

bors added a commit that referenced this pull request Dec 31, 2016
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)
@bors
Copy link
Contributor

bors commented Dec 31, 2016

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 38bd207 to master...

@bors bors merged commit 29e01af into rust-lang:master Dec 31, 2016
@jackpot51
Copy link
Contributor

Awesome!

@est31
Copy link
Member Author

est31 commented Jan 1, 2017

wow, great!!

🎉

it was the

FIRST TRY

I assure you!!
http://devopsreactions.tumblr.com/post/151057980189/getting-your-build-through-the-pipeline

http://devopsreactions.tumblr.com/post/151057980189/getting-your-build-through-the-pipeline

@DirkyJerky
Copy link
Contributor

This passed on the first try in 2017! (in utc + (-3)..9 ).
Only acceptable explanation is that 2016 truly was the year of... what animal

est31 added a commit to est31/rust that referenced this pull request Feb 4, 2017
We introduced the unadjusted ABI to work around wrong
(buggy) ABI expectations by LLVM on Windows [1].
Therefore, it should be solely used on Windows and not
on other platforms, like right now is the case.

[1]: see this comment for details rust-lang#38482 (comment)
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 5, 2017
…gisa

Don't use "unadjusted" ABI on non windows platforms

We introduced the unadjusted ABI to work around wrong
(buggy) ABI expectations by LLVM on Windows [1].
Therefore, it should be solely used on Windows and not
on other platforms, like right now is the case.

[1]: see this comment for details rust-lang#38482 (comment)
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 5, 2017
…gisa

Don't use "unadjusted" ABI on non windows platforms

We introduced the unadjusted ABI to work around wrong
(buggy) ABI expectations by LLVM on Windows [1].
Therefore, it should be solely used on Windows and not
on other platforms, like right now is the case.

[1]: see this comment for details rust-lang#38482 (comment)
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 5, 2017
…gisa

Don't use "unadjusted" ABI on non windows platforms

We introduced the unadjusted ABI to work around wrong
(buggy) ABI expectations by LLVM on Windows [1].
Therefore, it should be solely used on Windows and not
on other platforms, like right now is the case.

[1]: see this comment for details rust-lang#38482 (comment)
waynenilsen pushed a commit to compound-finance/gateway that referenced this pull request Nov 20, 2020
As discussed bigint + u8 for decimal representation also avoiding the
use of generics for the most part. For now the only imaginable way
to have errors in my mind is decimal mismatch.

Lots to fill in here and some open questions

Do we want this in a separate crate? I imagine it is possible the
eth-rpc crate may ultimately depend on this functionality.

I just figured out that u128 is natively available in rust on all
targets as LLVM has support for it rust simply exposes that. See
[this pr](rust-lang/rust#38482) from late
2016. This has been [stablized in 1.26](https://blog.rust-lang.org/2018/05/10/Rust-1.26.html).
The representable range here even with a token that is using 18
decimals of precision is 19 9s. If you have 10^19 of something
I think it is alright to say that you need to do two transactions.

I believe that u128 represents the best combination of speed
flexibility and consistency. It is very unlikely to me that
any token balance will natively overflow this representation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.