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

Implement i128 lowering for backends without stable i128 support #45676

Closed
est31 opened this issue Nov 1, 2017 · 13 comments
Closed

Implement i128 lowering for backends without stable i128 support #45676

est31 opened this issue Nov 1, 2017 · 13 comments
Labels
A-codegen Area: Code generation C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@est31
Copy link
Member

est31 commented Nov 1, 2017

While {i128, u128} support works greatly on many backends (x86 32 bit and 64 bit versions for example), emscripten and other smaller backends have a shaky support story for i128 integers. Often a backend gets written with support for C/C++ in mind, and those languages don't mandate i128 integer support, so those backends don't implement it. You are getting assertions or worse if presenting the backend with IR that uses i128. While we offer intrinsics for many i128 related operations in the compiler_builtins crate (offering the i128 operation implementations beyond the limited set of targets that C written comiler-rt supports), a backend still needs to implement sret support and other features. So for these backends, i128 is disabled right now.

Having spotty support for i128 means that even if we stabilize i128, many users of the language will avoid using the native i128 types, and instead adopt crates that do that lowering manually, which in turn leads to fragmentation of the ecosystem and prevents them from having a native i128 experience which includes support for proper literals. This is not what we have a native i128 support for!

Therefore, rustc should implement lowering of i128 that backends which don't have support for i128 can opt into.

One way to expose this feature could be as a member i128_lowering to the TargetOptions struct, which is enabled for the targets in question.

cc tracking issue #35118.

cc @alexcrichton @nagisa @sfackler

@scottmcm is doing awesome work on the issue:

@est31
Copy link
Member Author

est31 commented Nov 1, 2017

@nagisa want to write mentor instructions for this one?

@nagisa
Copy link
Member

nagisa commented Nov 1, 2017

Sure.

So, the “why” of the task has been pretty well explained by @est31, leaving me with “what” and “how”. Lets go through them in order.

Every operation (+, -, *, /, %, >>, << etc) when applied to 128-bit wide operands should be replaced with a call to a corresponding implementation in the compiler-builtins crate, when it is known, that the LLVM backend for the target is incapable of doing it itself. The function names to call are well-known. For example * is __muloti4. The names can be collected from the builtins crate.

There are two approaches to implementing this. As a transformation pass over MIR and directly during translation. Both are equally easy (or difficult).

For MIR pass, consider checking out how other MIR passes are implemented (linked is one of the more simple passes). There you would match on statements with Rvalue::BinaryOp and Rvalue::CheckedBinaryOp in its rvalue side, replacing them with a call terminator.

If you decide to adjust trans instead, the relevant portion of the code is these two match-cases and the functions they call.

It might be less involving to apply the change to trans, because it avoids you having to materialise function references and their types from thin air in MIR. It might involve making every 128-bit builtin a language item (probably a good idea nevertheless), whereas in trans only a function declaration in LLVM needs to be created (for that the code for overflow intrinisics can be followed fairly closely).

If you have any questions, feel free to ask on irc.mozilla.org (#rustc, #rust-internals). You can find me on the server as nagisa, though I’m sure anybody in the channel(s) will be glad to share their insight.

@nagisa nagisa added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed E-needs-mentor labels Nov 1, 2017
@scottmcm
Copy link
Member

scottmcm commented Nov 2, 2017

I'd like to take a stab at this! See you on IRC when I get confused :)

@main--
Copy link
Contributor

main-- commented Nov 2, 2017

Are these calls inlineable? I imagine that could make a big difference in some cases.

@nagisa
Copy link
Member

nagisa commented Nov 2, 2017

Yes, they are, although most of the builtins functions are fairly large and might not pass the inlining threshold.

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 8, 2017

@scottmcm any progress on this?

@scottmcm
Copy link
Member

Sorry for the delay on this. I got it somewhat working in trans (it correctly compiled i128 to compiler-builtins calls which worked in my test program) treating them as intrinsics, but subsequent discussions in #rustc convinced me I should start over with lang items in MIR.

@ebfull
Copy link
Contributor

ebfull commented Nov 18, 2017

@scottmcm Cool! Do you have a development branch somewhere?

@scottmcm
Copy link
Member

scottmcm commented Nov 19, 2017

@ebfull Posted a PR for part 1: #46093

It can be experimented with if you define the lang items yourself.

Edit: the definitions are in compiler-builtins, now, so you just need -Z lower_128bit_ops.

bors added a commit that referenced this issue Nov 24, 2017
Add a MIR pass to lower 128-bit operators to lang item calls

Runs only with `-Z lower_128bit_ops` since it's not hooked into targets yet.

This isn't really useful on its own, but the declarations for the lang items need to be in the compiler before compiler-builtins can be updated to define them, so this is part 1 of at least 3.

cc #45676 @est31 @nagisa
arielb1 pushed a commit to arielb1/rust that referenced this issue Nov 30, 2017
…s, r=nagisa

Update compiler-builtins and use it in the 128-bit lowering MIR test

This picks up the lang item implementations from rust-lang/compiler-builtins#210

cc rust-lang#45676 @est31 @nagisa
arielb1 pushed a commit to arielb1/rust that referenced this issue Nov 30, 2017
…s, r=nagisa

Update compiler-builtins and use it in the 128-bit lowering MIR test

This picks up the lang item implementations from rust-lang/compiler-builtins#210

cc rust-lang#45676 @est31 @nagisa
arielb1 pushed a commit to arielb1/rust that referenced this issue Nov 30, 2017
…s, r=nagisa

Update compiler-builtins and use it in the 128-bit lowering MIR test

This picks up the lang item implementations from rust-lang/compiler-builtins#210

cc rust-lang#45676 @est31 @nagisa
arielb1 pushed a commit to arielb1/rust that referenced this issue Nov 30, 2017
…s, r=nagisa

Update compiler-builtins and use it in the 128-bit lowering MIR test

This picks up the lang item implementations from rust-lang/compiler-builtins#210

cc rust-lang#45676 @est31 @nagisa
bors added a commit that referenced this issue Dec 3, 2017
Update compiler-builtins and use it in the 128-bit lowering MIR test

This picks up the lang item implementations from rust-lang/compiler-builtins#210

cc #45676 @est31 @nagisa
bors added a commit that referenced this issue Dec 4, 2017
Add an i128_lowering flag in TargetOptions

Not actually enabled by default anywhere yet.

r? @nagisa

cc #45676 @est31
@scottmcm
Copy link
Member

scottmcm commented Dec 8, 2017

Edit: Oh, const context. That makes sense. Will copy-paste the "not in const" block that's in so many of the other passes and see if that handles it.

So I tried enabling lowering for x86 as a smoke test, and it seems to want the MIR of the functions I'm calling. I'm going through adding #[inline] all over, which is fixing it, but I wonder if I should be doing something else. The error:

error: internal compiler error: src\librustc_metadata\cstore_impl.rs:131: get_optimized_mir: missing MIR for `DefId(8/0:
243 ~ compiler_builtins[9532]::int[0]::shift[0]::__ashlti3[0])`

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.24.0-dev running on x86_64-pc-windows-msvc

note: run with `RUST_BACKTRACE=1` for a backtrace

thread 'rustc' panicked at 'Box<Any>', src\librustc_errors\lib.rs:501:8
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::windows::backtrace::unwind_backtrace
             at .\src\libstd\sys\windows\backtrace\mod.rs:65
   1: std::sys_common::backtrace::print
             at .\src\libstd\sys_common\backtrace.rs:56
   2: std::panicking::default_hook::{{closure}}
             at .\src\libstd\panicking.rs:381
   3: std::panicking::default_hook
             at .\src\libstd\panicking.rs:391
   4: std::panicking::rust_panic_with_hook
             at .\src\libstd\panicking.rs:577
   5: std::panicking::begin_panic<rustc_errors::ExplicitBug>
             at .\src\libstd\panicking.rs:538
   6: rustc_errors::Handler::bug
             at .\src\librustc_errors\lib.rs:501
   7: std::thread::local::LocalKey<core::cell::Cell<core::option::Option<(const rustc::ty::context::tls::ThreadLocalGlob
alCtxt*, const rustc::ty::context::tls::ThreadLocalInterners*)>>>::with<core::cell::Cell<core::option::Option<(const rus
tc::ty::context::tls::ThreadLocalGlobalCtxt*, const rustc::ty::context::tls::ThreadLocalInterners*)>>,closure,!>
             at .\src\libstd\thread\local.rs:288
   8: rustc::ty::context::tls::with_opt<closure,!>
             at .\src\librustc\ty\context.rs:1478
   9: rustc::session::opt_span_bug_fmt<syntax_pos::span_encoding::Span>
             at .\src\librustc\session\mod.rs:1043
  10: rustc::session::bug_fmt
             at .\src\librustc\session\mod.rs:1027
  11: rustc_metadata::cstore_impl::provide_extern::optimized_mir<rustc::hir::def_id::DefId>
             at .\src\librustc_metadata\cstore_impl.rs:71
  12: rustc::ty::maps::queries::optimized_mir::compute_result
             at .\src\librustc\ty\maps\plumbing.rs:383
  13: rustc::dep_graph::graph::DepGraph::with_task_impl<rustc::ty::context::TyCtxt,rustc::hir::def_id::DefId,rustc::mir:
:Mir*,rustc::ich::hcx::StableHashingContext>
             at .\src\librustc\dep_graph\graph.rs:273
  14: rustc_errors::Handler::track_diagnostics<closure,(rustc::mir::Mir*, rustc::dep_graph::graph::DepNodeIndex)>
             at .\src\librustc_errors\lib.rs:564
  15: rustc::ty::context::TyCtxt::cycle_check<closure,((rustc::mir::Mir*, rustc::dep_graph::graph::DepNodeIndex), alloc:
:vec::Vec<rustc_errors::diagnostic::Diagnostic>)>
             at .\src\librustc\ty\maps\plumbing.rs:121
  16: rustc::ty::maps::queries::optimized_mir::force
             at .\src\librustc\ty\maps\plumbing.rs:484
  17: rustc::ty::maps::queries::optimized_mir::try_get
             at .\src\librustc\ty\maps\plumbing.rs:524
  18: rustc::ty::maps::TyCtxtAt::optimized_mir
             at .\src\librustc\ty\maps\plumbing.rs:565
  19: rustc::ty::context::TyCtxt::instance_mir
             at .\src\librustc\ty\mod.rs:2334
  20: rustc_trans::mir::constant::MirConstContext::trans_def
             at .\src\librustc_trans\mir\constant.rs:302
  21: rustc_trans::mir::constant::MirConstContext::trans
             at .\src\librustc_trans\mir\constant.rs:415
  22: rustc_trans::mir::constant::MirConstContext::trans_def
             at .\src\librustc_trans\mir\constant.rs:303
  23: rustc_trans::mir::constant::MirConstContext::trans
             at .\src\librustc_trans\mir\constant.rs:415
  24: rustc_trans::mir::MirContext::trans_constant
             at .\src\librustc_trans\mir\constant.rs:1064
  25: rustc_trans::mir::MirContext::trans_operand
             at .\src\librustc_trans\mir\operand.rs:317
  26: rustc_trans::mir::MirContext::trans_rvalue_operand
             at .\src\librustc_trans\mir\rvalue.rs:473
  27: rustc_trans::mir::trans_mir
             at .\src\librustc_trans\mir\mod.rs:320
  28: rustc_trans::base::trans_instance
             at .\src\librustc_trans\base.rs:505
  29: rustc_trans::trans_item::TransItemExt::define<rustc::middle::trans::TransItem>
             at .\src\librustc_trans\trans_item.rs:73
  30: rustc_trans::base::compile_codegen_unit
             at .\src\librustc_trans\base.rs:1161
  31: rustc::ty::maps::queries::compile_codegen_unit::compute_result
             at .\src\librustc\ty\maps\plumbing.rs:383
  32: rustc::dep_graph::graph::DepGraph::with_task_impl<rustc::ty::context::TyCtxt,syntax_pos::symbol::InternedString,ru
stc::middle::trans::Stats,rustc::ich::hcx::StableHashingContext>
             at .\src\librustc\dep_graph\graph.rs:273
  33: rustc_errors::Handler::track_diagnostics<closure,(rustc::middle::trans::Stats, rustc::dep_graph::graph::DepNodeInd
ex)>
             at .\src\librustc_errors\lib.rs:564
  34: rustc::ty::context::TyCtxt::cycle_check<closure,((rustc::middle::trans::Stats, rustc::dep_graph::graph::DepNodeInd
ex), alloc::vec::Vec<rustc_errors::diagnostic::Diagnostic>)>
             at .\src\librustc\ty\maps\plumbing.rs:121
  35: rustc::ty::maps::queries::compile_codegen_unit::force
             at .\src\librustc\ty\maps\plumbing.rs:484
  36: rustc::ty::maps::queries::compile_codegen_unit::try_get
             at .\src\librustc\ty\maps\plumbing.rs:524
  37: rustc::ty::maps::TyCtxtAt::compile_codegen_unit
             at .\src\librustc\ty\maps\plumbing.rs:565
  38: rustc::ty::context::TyCtxt::compile_codegen_unit
             at .\src\librustc\ty\maps\plumbing.rs:558
  39: rustc_trans::base::trans_crate
             at .\src\librustc_trans\base.rs:888
  40: rustc_trans::{{impl}}::trans_crate
             at .\src\librustc_trans\lib.rs:181
  41: rustc_driver::driver::phase_4_translate_to_llvm<rustc_trans::LlvmTransCrate>
             at .\src\librustc_driver\driver.rs:1113
  42: rustc_driver::driver::compile_input::{{closure}}
             at .\src\librustc_driver\driver.rs:249
  43: std::thread::local::LocalKey<core::cell::Cell<core::option::Option<(const rustc::ty::context::tls::ThreadLocalGlob
alCtxt*, const rustc::ty::context::tls::ThreadLocalInterners*)>>>::with<core::cell::Cell<core::option::Option<(const rus
tc::ty::context::tls::ThreadLocalGlobalCtxt*, const rustc::ty::context::tls::ThreadLocalInterners*)>>,closure,core::resu
lt::Result<core::result::Result<(rustc::session::config::OutputFilenames, rustc_trans::back::write::OngoingCrateTranslat
ion, rustc::dep_graph::graph::DepGraph), rustc::session::CompileIncomplete>, rustc::session::CompileIncomplete>>
             at .\src\libstd\thread\local.rs:288
  44: std::thread::local::LocalKey<core::cell::Cell<fn(syntax_pos::span_encoding::Span, mut core::fmt::Formatter*) -> co
re::result::Result<(), core::fmt::Error>>>::with<core::cell::Cell<fn(syntax_pos::span_encoding::Span, mut core::fmt::For
matter*) -> core::result::Result<(), core::fmt::Error>>,closure,core::result::Result<core::result::Result<(rustc::sessio
n::config::OutputFilenames, rustc_trans::back::write::OngoingCrateTranslation, rustc::dep_graph::graph::DepGraph), rustc
::session::CompileIncomplete>, rustc::session::CompileIncomplete>>
             at .\src\libstd\thread\local.rs:288
  45: rustc::ty::context::TyCtxt::create_and_enter<closure,core::result::Result<core::result::Result<(rustc::session::co
nfig::OutputFilenames, rustc_trans::back::write::OngoingCrateTranslation, rustc::dep_graph::graph::DepGraph), rustc::ses
sion::CompileIncomplete>, rustc::session::CompileIncomplete>>
             at .\src\librustc\ty\context.rs:1069
  46: rustc_driver::driver::compile_input
             at .\src\librustc_driver\driver.rs:216
  47: rustc_driver::run_compiler
             at .\src\librustc_driver\lib.rs:253

bors added a commit that referenced this issue Dec 20, 2017
Fix -Z lower_128bit_ops handling of statics

Avoids ICEs such as the following:
>  error: internal compiler error: src\librustc_metadata\cstore_impl.rs:131:
>  get_optimized_mir: missing MIR for `DefId(8/0:40 ~
>  compiler_builtins[9532]::int[0]::addsub[0]::rust_i128_addo[0])`

r? @nagisa

cc #45676 @est31
@XAMPPRocky XAMPPRocky added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Feb 12, 2018
@nagisa nagisa closed this as completed Mar 29, 2018
@est31
Copy link
Member Author

est31 commented Mar 29, 2018

@nagisa I think this needs to be reopened: #49487 (comment)

We should take supporting i128 on emscripten as a benchmark that our lowering works.

@scottmcm
Copy link
Member

scottmcm commented Mar 30, 2018

Reactivating since this definitely doesn't work right now -- if nothing else I never got to lowering of intrinsics (to support things like i128::wrapping_add).

Random note: with MIRI landed, it could be nice if the compiler_builtins versions became const, so that things would work naturally in const and such without MIRI needing to special-case them.

@scottmcm scottmcm reopened this Mar 30, 2018
@nagisa
Copy link
Member

nagisa commented Mar 30, 2018

Okay, I added a note to that effect to the tracking issue list.

@est31 est31 closed this as completed Oct 19, 2018
@rust-lang rust-lang locked and limited conversation to collaborators Oct 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-codegen Area: Code generation C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants