Skip to content

Fix -Z lower_128bit_ops handling of statics #46583

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

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Dec 8, 2017

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

(hir::BodyOwnerKind::Static(_), _) => return,

(hir::BodyOwnerKind::Fn, _) => {
if tcx.is_const_fn(source.def_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean i128 ops won't get lowered in const fns at all? As const fns can run at runtime as well, I figure they'd have to be lowered for the run time version even if const eval can handle them.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 8, 2017
@nagisa
Copy link
Member

nagisa commented Dec 8, 2017

I think what should happen here is:

  1. We make sure MIR gets included into compiler-builtins for i128 language items. This should happen within the compiler (so the change supposedly should be somewhere within the collector);
  2. We make sure this pass gets run after constant propagation/miri runs, when those land.

@scottmcm
Copy link
Member Author

I'm not certain what I should do here. rkruppe is absolutely right that this PR is just changing an unsoundness to an incompleteness.

(1) Getting the MIR for the lang items themselves seems insufficient, as I first tried #[inline]ing them, which then just complained about the things they were calling. So that feels wrong, especially as it may require getting the MIR for a some things in core, which shouldn't be affected by this.

(2) This sounds great, though I'm not sure where this pass would then run, since I figure const fns calling other const fns need this to not be applied, at which point it feels like it can't be in the normal "run these passes as part of generating final MIR for these functions". Is there a spot in trans where the pass could run? Or at that point does having this be done on MIR no longer make sense...

@nagisa
Copy link
Member

nagisa commented Dec 10, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 10, 2017

📌 Commit 1b88022 has been approved by nagisa

@bors
Copy link
Collaborator

bors commented Dec 10, 2017

⌛ Testing commit 1b8802224370abea71de246d80a2859b40aacdf1 with merge ec06e86fd1f0a30f558e94af4e8771a9a08fe890...

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2017
@arielb1
Copy link
Contributor

arielb1 commented Dec 10, 2017

@bors r- retry

I don't think this actually fixes things - now const fns will generate bad i128 ops when called in a non-constant context.

@arielb1
Copy link
Contributor

arielb1 commented Dec 10, 2017

I think the right way to fix it in the rustc_trans::mir::constant context would be to treat the i128 ops like intrinsics and "special-case-translate" them here:

if let Some((ref dest, target)) = *destination {
let result = if fn_ty.fn_sig(tcx).abi() == Abi::RustIntrinsic {
match &tcx.item_name(def_id)[..] {
"size_of" => {
let llval = C_usize(self.ccx,
self.ccx.size_of(substs.type_at(0)).bytes());
Ok(Const::new(llval, tcx.types.usize))
}
"min_align_of" => {
let llval = C_usize(self.ccx,
self.ccx.align_of(substs.type_at(0)).abi());
Ok(Const::new(llval, tcx.types.usize))
}
_ => span_bug!(span, "{:?} in constant", terminator.kind)
}
} else {
MirConstContext::trans_def(self.ccx, def_id, substs, arg_vals)
};

by calling the appropriate binary op implementation if the function def-id matches, via an inverse of the item_for_op function:

fn item_for_op(bin_op: BinOp, is_signed: bool) -> Option<(LangItem, RhsKind)> {

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 11, 2017
@scottmcm scottmcm force-pushed the fix-static-i128-lower branch from 1b88022 to 413a495 Compare December 19, 2017 02:26
@scottmcm scottmcm changed the title Fix -Z lower_128bit_ops handling of statics [WIP] Fix -Z lower_128bit_ops handling of statics Dec 19, 2017
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])`
@scottmcm scottmcm force-pushed the fix-static-i128-lower branch from 413a495 to 4b95ca8 Compare December 19, 2017 20:08
@scottmcm scottmcm changed the title [WIP] Fix -Z lower_128bit_ops handling of statics Fix -Z lower_128bit_ops handling of statics Dec 19, 2017
@scottmcm
Copy link
Member Author

Ok, updated to special-case-translate these calls in const trans. The inverse lookup can be found here: https://github.com/rust-lang/rust/pull/46583/files#diff-823b4422fff3f3018e53dfb6dc0a1fb3R468

@arielb1
Copy link
Contributor

arielb1 commented Dec 19, 2017

r? @eddyb - you know rustc_trans::mir::constant

@rust-highfive rust-highfive assigned eddyb and unassigned nagisa Dec 19, 2017
@eddyb
Copy link
Member

eddyb commented Dec 19, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 19, 2017

📌 Commit 4b95ca8 has been approved by eddyb

@bors
Copy link
Collaborator

bors commented Dec 20, 2017

⌛ Testing commit 4b95ca8 with merge 7eb64b8...

bors added a commit that referenced this pull request 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
@bors
Copy link
Collaborator

bors commented Dec 20, 2017

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

@bors bors merged commit 4b95ca8 into rust-lang:master Dec 20, 2017
@scottmcm scottmcm deleted the fix-static-i128-lower branch December 20, 2017 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants