Skip to content

Commit

Permalink
Auto merge of #48602 - eddyb:get-in-line, r=<try>
Browse files Browse the repository at this point in the history
rustc_mir: always run the inlining pass.

**DO NOT MERGE**: there are still issues around debuginfo and maybe even misoptimizations.
As suggested by @pcwalton, maybe it's time to test the overhead / wins from turning MIR inlining on.
  • Loading branch information
bors committed Feb 28, 2018
2 parents affe297 + f1878ff commit a173a82
Show file tree
Hide file tree
Showing 15 changed files with 109 additions and 61 deletions.
1 change: 1 addition & 0 deletions src/librustc_metadata/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ impl<'a> CrateLoader<'a> {
cnum_map: RefCell::new(cnum_map),
cnum,
codemap_import_info: RefCell::new(vec![]),
last_filemap_index: Cell::new(0),
attribute_cache: RefCell::new([Vec::new(), Vec::new()]),
dep_kind: Cell::new(dep_kind),
source: cstore::CrateSource {
Expand Down
2 changes: 2 additions & 0 deletions src/librustc_metadata/cstore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ pub struct CrateMetadata {
pub cnum_map: RefCell<CrateNumMap>,
pub cnum: CrateNum,
pub codemap_import_info: RefCell<Vec<ImportedFileMap>>,
// Cache the last used filemap for translating spans as an optimization.
pub last_filemap_index: Cell<usize>,
pub attribute_cache: RefCell<[Vec<Option<Rc<[ast::Attribute]>>>; 2]>,

pub root: schema::CrateRoot,
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_metadata/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ macro_rules! provide {

let $cdata = $tcx.crate_data_as_rc_any($def_id.krate);
let $cdata = $cdata.downcast_ref::<cstore::CrateMetadata>()
.expect("CrateStore crated ata is not a CrateMetadata");
.expect("CrateStore crate data is not a CrateMetadata");
$compute
})*

Expand Down
100 changes: 59 additions & 41 deletions src/librustc_metadata/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use std::cell::Ref;
use std::collections::BTreeMap;
use std::io;
use std::mem;
use std::ops::RangeInclusive;
use std::rc::Rc;
use std::u32;

Expand All @@ -50,9 +51,6 @@ pub struct DecodeContext<'a, 'tcx: 'a> {
sess: Option<&'a Session>,
tcx: Option<TyCtxt<'a, 'tcx, 'tcx>>,

// Cache the last used filemap for translating spans as an optimization.
last_filemap_index: usize,

lazy_state: LazyState,
}

Expand All @@ -70,7 +68,7 @@ pub trait Metadata<'a, 'tcx>: Copy {
cdata: self.cdata(),
sess: self.sess().or(tcx.map(|tcx| tcx.sess)),
tcx,
last_filemap_index: 0,

lazy_state: LazyState::NoNode,
}
}
Expand Down Expand Up @@ -270,63 +268,46 @@ impl<'a, 'tcx> SpecializedDecoder<DefIndex> for DecodeContext<'a, 'tcx> {

impl<'a, 'tcx> SpecializedDecoder<Span> for DecodeContext<'a, 'tcx> {
fn specialized_decode(&mut self) -> Result<Span, Self::Error> {
let tag = u8::decode(self)?;
let cnum_tag = u32::decode(self)?;

if tag == TAG_INVALID_SPAN {
if cnum_tag == TAG_INVALID_SPAN_CNUM {
return Ok(DUMMY_SP)
}

debug_assert_eq!(tag, TAG_VALID_SPAN);
let original_cnum = CrateNum::from_u32(cnum_tag - TAG_VALID_SPAN_CNUM_START);
let cnum = self.map_encoded_cnum_to_current(original_cnum);

let lo = BytePos::decode(self)?;
let len = BytePos::decode(self)?;
let hi = lo + len;

let sess = if let Some(sess) = self.sess {
sess
} else {
bug!("Cannot decode Span without Session.")
};

let imported_filemaps = self.cdata().imported_filemaps(&sess.codemap());
let filemap = {
// Optimize for the case that most spans within a translated item
// originate from the same filemap.
let last_filemap = &imported_filemaps[self.last_filemap_index];

if lo >= last_filemap.original_start_pos &&
lo <= last_filemap.original_end_pos {
last_filemap
} else {
let mut a = 0;
let mut b = imported_filemaps.len();

while b - a > 1 {
let m = (a + b) / 2;
if imported_filemaps[m].original_start_pos > lo {
b = m;
} else {
a = m;
}
}

self.last_filemap_index = a;
&imported_filemaps[a]
}
let span_cdata_rc_any;
let span_cdata = if original_cnum == LOCAL_CRATE {
self.cdata()
} else {
// FIXME(eddyb) this requires the `tcx` which isn't always available.
// However, currently only MIR inlining can end up producing such
// cross-crate spans, and decoding MIR always provides a `tcx`.
span_cdata_rc_any = self.tcx().crate_data_as_rc_any(cnum);
span_cdata_rc_any.downcast_ref::<CrateMetadata>()
.expect("CrateStore crate data is not a CrateMetadata")
};

// Make sure our binary search above is correct.
debug_assert!(lo >= filemap.original_start_pos &&
lo <= filemap.original_end_pos);
let filemap = span_cdata.imported_filemap_containing(&sess.codemap(), lo, |filemap| {
filemap.original_start_pos..=filemap.original_end_pos
});

// Make sure we correctly filtered out invalid spans during encoding
debug_assert!(hi >= filemap.original_start_pos &&
hi <= filemap.original_end_pos);
debug_assert!(lo + len >= filemap.original_start_pos &&
lo + len <= filemap.original_end_pos);

let lo = (lo + filemap.translated_filemap.start_pos) - filemap.original_start_pos;
let hi = (hi + filemap.translated_filemap.start_pos) - filemap.original_start_pos;

Ok(Span::new(lo, hi, NO_EXPANSION))
Ok(Span::new(lo, lo + len, NO_EXPANSION))
}
}

Expand Down Expand Up @@ -1169,4 +1150,41 @@ impl<'a, 'tcx> CrateMetadata {
*self.codemap_import_info.borrow_mut() = imported_filemaps;
self.codemap_import_info.borrow()
}

pub fn imported_filemap_containing<F>(&'a self,
local_codemap: &codemap::CodeMap,
pos: BytePos,
range_of: F)
-> Ref<'a, cstore::ImportedFileMap>
where F: Fn(&cstore::ImportedFileMap) -> RangeInclusive<BytePos>
{
Ref::map(self.imported_filemaps(local_codemap), |imported_filemaps| {
// Optimize for the case that most spans within a translated item
// originate from the same filemap.
let last_filemap = &imported_filemaps[self.last_filemap_index.get()];
if range_of(last_filemap).contains(pos) {
last_filemap
} else {
let mut a = 0;
let mut b = imported_filemaps.len();

while b - a > 1 {
let m = (a + b) / 2;
if range_of(&imported_filemaps[m]).start > pos {
b = m;
} else {
a = m;
}
}

self.last_filemap_index.set(a);
let filemap = &imported_filemaps[a];

// Make sure our binary search above is correct.
debug_assert!(range_of(filemap).contains(pos));

filemap
}
})
}
}
27 changes: 23 additions & 4 deletions src/librustc_metadata/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use cstore::CrateMetadata;
use index::Index;
use index_builder::{FromId, IndexBuilder, Untracked};
use isolated_encoder::IsolatedEncoder;
Expand Down Expand Up @@ -147,7 +148,7 @@ impl<'a, 'tcx> SpecializedEncoder<DefIndex> for EncodeContext<'a, 'tcx> {
impl<'a, 'tcx> SpecializedEncoder<Span> for EncodeContext<'a, 'tcx> {
fn specialized_encode(&mut self, span: &Span) -> Result<(), Self::Error> {
if *span == DUMMY_SP {
return TAG_INVALID_SPAN.encode(self)
return TAG_INVALID_SPAN_CNUM.encode(self)
}

let span = span.data();
Expand All @@ -164,11 +165,29 @@ impl<'a, 'tcx> SpecializedEncoder<Span> for EncodeContext<'a, 'tcx> {
if !self.filemap_cache.contains(span.hi) {
// Unfortunately, macro expansion still sometimes generates Spans
// that malformed in this way.
return TAG_INVALID_SPAN.encode(self)
return TAG_INVALID_SPAN_CNUM.encode(self)
}

TAG_VALID_SPAN.encode(self)?;
span.lo.encode(self)?;
let cnum = CrateNum::from_u32(self.filemap_cache.crate_of_origin);
let cnum_tag = TAG_VALID_SPAN_CNUM_START + cnum.as_u32();
cnum_tag.encode(self)?;
let original_lo = if cnum == LOCAL_CRATE {
span.lo
} else {
// Imported spans were adjusted when they were decoded, so
// they have to be translated back into their crate of origin.
let codemap = self.tcx.sess.codemap();
let span_cdata_rc_any = self.tcx.crate_data_as_rc_any(cnum);
let span_cdata = span_cdata_rc_any.downcast_ref::<CrateMetadata>()
.expect("CrateStore crate data is not a CrateMetadata");
// FIXME(eddyb) It'd be easier to just put `original_{start,end}_pos`
// in `syntax_pos::FileMap` instead of `ImportedFileMap`.
let filemap = span_cdata.imported_filemap_containing(&codemap, span.lo, |filemap| {
filemap.translated_filemap.start_pos..=filemap.translated_filemap.end_pos
});
(span.lo + filemap.original_start_pos) - filemap.translated_filemap.start_pos
};
original_lo.encode(self)?;

// Encode length which is usually less than span.hi and profits more
// from the variable-length integer encoding that we use.
Expand Down
3 changes: 3 additions & 0 deletions src/librustc_metadata/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@
#![feature(conservative_impl_trait)]
#![feature(fs_read_write)]
#![feature(i128_type)]
#![feature(inclusive_range)]
#![feature(inclusive_range_syntax)]
#![feature(libc)]
#![feature(proc_macro_internals)]
#![feature(quote)]
#![feature(range_contains)]
#![feature(rustc_diagnostic_macros)]
#![feature(specialization)]
#![feature(rustc_private)]
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_metadata/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -521,5 +521,5 @@ pub struct GeneratorData<'tcx> {
impl_stable_hash_for!(struct GeneratorData<'tcx> { layout });

// Tags used for encoding Spans:
pub const TAG_VALID_SPAN: u8 = 0;
pub const TAG_INVALID_SPAN: u8 = 1;
pub const TAG_INVALID_SPAN_CNUM: u32 = 0;
pub const TAG_VALID_SPAN_CNUM_START: u32 = 1;
4 changes: 1 addition & 3 deletions src/librustc_mir/transform/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ impl MirPass for Inline {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
source: MirSource,
mir: &mut Mir<'tcx>) {
if tcx.sess.opts.debugging_opts.mir_opt_level >= 2 {
Inliner { tcx, source }.run_pass(mir);
}
Inliner { tcx, source }.run_pass(mir);
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/test/codegen/internalize-closures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@

// compile-flags: -C no-prepopulate-passes

#![feature(stmt_expr_attributes)]

pub fn main() {

// We want to make sure that closures get 'internal' linkage instead of
// 'weak_odr' when they are not shared between codegen units
// CHECK: define internal {{.*}}_ZN20internalize_closures4main{{.*}}$u7b$$u7b$closure$u7d$$u7d$
let c = |x:i32| { x + 1 };
let c = #[inline(never)] |x:i32| { x + 1 };
let _ = c(1);
}
6 changes: 3 additions & 3 deletions src/test/codegen/remap_path_prefix/aux_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

// ignore-test: this is not a test

#[inline]
pub fn some_aux_mod_function() -> i32 {
1234
#[inline(never)]
pub fn some_aux_mod_function<T>() -> usize {
::std::mem::size_of::<T>()
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

// compile-flags: -g -Zremap-path-prefix-from={{cwd}} -Zremap-path-prefix-to=/the/aux-cwd -Zremap-path-prefix-from={{src-base}}/remap_path_prefix/auxiliary -Zremap-path-prefix-to=/the/aux-src

#[inline]
pub fn some_aux_function() -> i32 {
1234
#[inline(never)]
pub fn some_aux_function<T>() -> usize {
std::mem::size_of::<T>()
}
6 changes: 3 additions & 3 deletions src/test/codegen/remap_path_prefix/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ include!("aux_mod.rs");
pub static FILE_PATH: &'static str = file!();

fn main() {
remap_path_prefix_aux::some_aux_function();
aux_mod::some_aux_mod_function();
some_aux_mod_function();
remap_path_prefix_aux::some_aux_function::<()>();
aux_mod::some_aux_mod_function::<()>();
some_aux_mod_function::<()>();
}

// Here we check that local debuginfo is mapped correctly.
Expand Down
1 change: 1 addition & 0 deletions src/test/compile-fail/issue-22638.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ impl C {
struct D (Box<A>);

impl D {
#[inline(never)]
pub fn matches<F: Fn()>(&self, f: &F) {
//~^ ERROR reached the type-length limit while instantiating `D::matches::<[closure
let &D(ref a) = self;
Expand Down
3 changes: 3 additions & 0 deletions src/test/compile-fail/type_length_limit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ link! { F, G }

pub struct G;

#[inline(never)]
fn drop<T>(_: T) {}

fn main() {
drop::<Option<A>>(None);
}
1 change: 1 addition & 0 deletions src/test/debuginfo/associated-types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ fn assoc_return_value<T: TraitWithAssocType>(arg: T) -> T::Type {
return arg.get_value();
}

#[inline(never)]
fn assoc_tuple<T: TraitWithAssocType>(arg: (T, T::Type)) {
zzz(); // #break
}
Expand Down

0 comments on commit a173a82

Please sign in to comment.