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

Tracking issue for 128-bit integer support (RFC 1504) #35118

Closed
4 of 5 tasks
nikomatsakis opened this issue Jul 29, 2016 · 124 comments
Closed
4 of 5 tasks

Tracking issue for 128-bit integer support (RFC 1504) #35118

nikomatsakis opened this issue Jul 29, 2016 · 124 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jul 29, 2016

Tracking issue for rust-lang/rfcs#1504.

cc @Amanieu

Blocking stabilization:

@nikomatsakis nikomatsakis added T-lang Relevant to the language team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. labels Jul 29, 2016
@durka
Copy link
Contributor

durka commented Aug 2, 2016

Is #[repr(u128)] enum SuchWideVeryDiscriminantWow { ... } allowed?

@Amanieu
Copy link
Member

Amanieu commented Aug 2, 2016

That's a good point, I don't see any reason why it shouldn't be allowed.

@durka
Copy link
Contributor

durka commented Aug 2, 2016

The return type of the discriminant_value intrinsic needs to be updated, then :)

@nagisa
Copy link
Member

nagisa commented Aug 2, 2016

Intrinsics are stuff internal to the compiler, therefore changes to them doesn’t need discussion in the RFCs.

@durka
Copy link
Contributor

durka commented Aug 2, 2016

I know, I just wanted to mention it here since I didn't see enums discussed in the RFC.

@est31 est31 mentioned this issue Nov 20, 2016
2 tasks
bors added a commit that referenced this issue Dec 8, 2016
i128 and u128 support

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:

* [x] Confirm that intrinsics work on 32 bit platforms and fix them if needed

* Wait for #37906 to get fixed, so that work on intrinsics can be completed *(worked around by merging the PR on my local setup)*

* [x] Investigate and fix linkcheck failure

[plugin-breaking-change]

cc #35118
@emberian
Copy link
Member

How should FFI with this type be handled? Is there any standard ABI support for these types? AFAICT, the answer is "no", which means this type should be FFI-unsafe, and the FFI unsafety lint should be updated to reject Ty{I,Ui}nt with 128-bit sizes.

@emberian
Copy link
Member

cc @est31

@retep998
Copy link
Member

retep998 commented Dec 21, 2016

On Windows there is most definitely no standard i128 yet because the standard compiler (msvc) does not support __int128 yet on x86. There are some really good guesses though based on the MSDN documentation.

@est31
Copy link
Member

est31 commented Dec 21, 2016

Is there any standard ABI support for these types?

It depends on the architecture. SysV defines the ABI for 64 bit architectures. For x86, its most likely its not defined. Same goes for windows: it should in theory be defined for 64 bit (just sadly nobody adheres to it, its a gigantic mess), but not on x86.

This directly maps the support of the i128 type in C, and I think generally it makes little sense to have FFI for types that don't exist in C.

@est31
Copy link
Member

est31 commented Dec 21, 2016

bug report in llvm about the x86_64 ABI problems: https://llvm.org/bugs/show_bug.cgi?id=31362

@nagisa
Copy link
Member

nagisa commented Dec 21, 2016

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 23, 2018
Stabilize 128-bit integers 🎉

cc rust-lang#35118

EDIT: This should be merged only after the following have been merged:
- [x] rust-lang/compiler-builtins#236
- [x] rust-lang/book#1230
bors added a commit that referenced this issue Mar 26, 2018
Stabilize 128-bit integers 🎉

cc #35118

EDIT: This should be merged only after the following have been merged:
- [x] rust-lang/compiler-builtins#236
- [x] rust-lang/book#1230
@mark-i-m
Copy link
Member

I think this is done 🎉

@est31
Copy link
Member

est31 commented Mar 27, 2018

@mark-i-m congrats!

Just don't close this issue yet as like @durka pointed out, the repr128 feature is still pointing to this tracking issue.

@mark-i-m
Copy link
Member

Could someone update the OP too create more check boxes for repr128?

@ebfull
Copy link
Contributor

ebfull commented Mar 29, 2018

The checkbox for #45676 is checked in this issue but #45676 is not actually closed. What's the status of lowering on other platforms?

@nagisa
Copy link
Member

nagisa commented Mar 29, 2018

I think that is fixed, although not in the nicest way. I closed the issue.

@est31
Copy link
Member

est31 commented Mar 29, 2018

@ebfull see my comment here: #35118 (comment)

i128 works on the entire x86 family as well as the ARM family. This covers all of the tier 1 platforms.
The platforms where we lack i128 support are tier 2 or 3.

What you are doing in zkcrypto/pairing#80 is exactly what I wanted to prevent by blocking stabilisation until all platforms support i128 lol, but people thought otherwise.

@ebfull
Copy link
Contributor

ebfull commented Mar 30, 2018

@est31 I'm not doing zkcrypto/pairing#80 until it works on all platforms, which I mention in that issue. In the mean time all I will do is remove the i128_type feature flag, but still require users to opt into usage of u128.

@est31
Copy link
Member

est31 commented Mar 30, 2018

@ebfull not blaming you. I'm partially blaming myself, thought that lowering worked already. Partially I'm blaming those people who decided to stabilize before all platforms support it. And the backend vendors who refuse to take {u,i}128 seriously.

Sadly, this is more of a "stable beta" release of i128 than an arrival of a feature that can be relied upon.

@hdevalence
Copy link

It would be nice if there was a (documented) way to set a cargo feature as the default on a given architecture. From what I can tell this isn't quite possible, since the architecture-selection code happens with #[cfg(...)]s while the crate is being compiled, by which point the crate's features are already selected. Am I missing something?

The motivation is that even if u128s are available, it may not be desirable to use them, unless they actually correspond to instructions available on that platform.

@durka
Copy link
Contributor

durka commented Apr 1, 2018 via email

@Centril Centril added disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 24, 2018
@Centril
Copy link
Contributor

Centril commented Nov 11, 2018

@mark-i-m any developments re. repr128?

@mark-i-m
Copy link
Member

Not that I'm aware of... but I just did the stabilization. I'm not sure if if anyone has claimed ownership of that task yet...

@Centril
Copy link
Contributor

Centril commented Nov 11, 2018

@mark-i-m aw; bummer :(

Let me try some one else at random...

@nagisa 👋

@nagisa
Copy link
Member

nagisa commented Nov 11, 2018

Nothing has changed re repr128… It is still the case that at least LLVM’s debuginfo API blocks us from implementing it at all. There’s very little incentive to work on it, though, and I’m not planning to do anything in that direction until a very convincing use-case for 128-bit enum discriminants comes up.

EDIT: That, or if days suddenly become 48-hour long.

@Centril
Copy link
Contributor

Centril commented Nov 19, 2018

Closing this in favor of the targeted issue #56071 since all remaining work has been done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests