Skip to content

Commit

Permalink
Incorporated review feedback:
Browse files Browse the repository at this point in the history
Renamed the struct to make it a little clearer that it doesn't just hold one
imports map. (I couldn't bring myself to write it as `ThinLTOImportsExports`
though, mainly since the exports map is literally derived from the imports map
data.) Added some doc to the struct too.

Revised comments to add link to the newer issue that discusses why the exports
are relevant.

Renamed a few of the methods so that the two character difference is more
apparent (because 1. the method name is shorter and, perhaps more importantly,
the changed characters now lie at the beginning of the method name.)
  • Loading branch information
pnkfelix committed Apr 15, 2020
1 parent 12207f6 commit d05ae3a
Showing 1 changed file with 40 additions and 22 deletions.
62 changes: 40 additions & 22 deletions src/librustc_codegen_llvm/back/lto.rs
Expand Up @@ -463,15 +463,18 @@ fn thin_lto(
// If previous imports have been deleted, or we get an IO error
// reading the file storing them, then we'll just use `None` as the
// prev_import_map, which will force the code to be recompiled.
let prev =
if path.exists() { ThinLTOImports::load_from_file(&path).ok() } else { None };
let curr = ThinLTOImports::from_thin_lto_data(data);
let prev = if path.exists() {
ThinLTOImportMaps::load_from_file(&path).ok()
} else {
None
};
let curr = ThinLTOImportMaps::from_thin_lto_data(data);
(Some(path), prev, curr)
} else {
// If we don't compile incrementally, we don't need to load the
// import data from LLVM.
assert!(green_modules.is_empty());
let curr = ThinLTOImports::default();
let curr = ThinLTOImportMaps::default();
(None, None, curr)
};
info!("thin LTO import map loaded");
Expand All @@ -497,10 +500,14 @@ fn thin_lto(
let module_name = module_name_to_str(module_name);

// If (1.) the module hasn't changed, and (2.) none of the modules
// it imports from has changed, *and* (3.) the import-set itself has
// not changed from the previous compile when it was last
// ThinLTO'ed, then we can re-use the post-ThinLTO version of the
// module. Otherwise, freshly perform LTO optimization.
// it imports from nor exports to have changed, *and* (3.) the
// import and export sets themselves have not changed from the
// previous compile when it was last ThinLTO'ed, then we can re-use
// the post-ThinLTO version of the module. Otherwise, freshly
// perform LTO optimization.
//
// (Note that globally, the export set is just the inverse of the
// import set.)
//
// This strategy means we can always save the computed imports as
// canon: when we reuse the post-ThinLTO version, condition (3.)
Expand All @@ -509,16 +516,18 @@ fn thin_lto(
// version, the current import set *is* the correct one, since we
// are doing the ThinLTO in this current compilation cycle.)
//
// See rust-lang/rust#59535.
// For more discussion, see rust-lang/rust#59535 (where the import
// issue was discovered) and rust-lang/rust#69798 (where the
// analogous export issue was discovered).
if let (Some(prev_import_map), true) =
(prev_import_map.as_ref(), green_modules.contains_key(module_name))
{
assert!(cgcx.incr_comp_session_dir.is_some());

let prev_imports = prev_import_map.modules_imported_by(module_name);
let curr_imports = curr_import_map.modules_imported_by(module_name);
let prev_exports = prev_import_map.modules_exported_by(module_name);
let curr_exports = curr_import_map.modules_exported_by(module_name);
let prev_imports = prev_import_map.imports_of(module_name);
let curr_imports = curr_import_map.imports_of(module_name);
let prev_exports = prev_import_map.exports_of(module_name);
let curr_exports = curr_import_map.exports_of(module_name);
let imports_all_green = curr_imports
.iter()
.all(|imported_module| green_modules.contains_key(imported_module));
Expand Down Expand Up @@ -890,20 +899,29 @@ pub unsafe fn optimize_thin_module(
Ok(module)
}

/// Summarizes module import/export relationships used by LLVM's ThinLTO pass.
///
/// Note that we tend to have two such instances of `ThinLTOImportMaps` in use:
/// one loaded from a file that represents the relationships used during the
/// compilation associated with the incremetnal build artifacts we are
/// attempting to reuse, and another constructed via `from_thin_lto_data`, which
/// captures the relationships of ThinLTO in the current compilation.
#[derive(Debug, Default)]
pub struct ThinLTOImports {
pub struct ThinLTOImportMaps {
// key = llvm name of importing module, value = list of modules it imports from
imports: FxHashMap<String, Vec<String>>,
// key = llvm name of exporting module, value = list of modules it exports to
exports: FxHashMap<String, Vec<String>>,
}

impl ThinLTOImports {
fn modules_imported_by(&self, llvm_module_name: &str) -> &[String] {
impl ThinLTOImportMaps {
/// Returns modules imported by `llvm_module_name` during some ThinLTO pass.
fn imports_of(&self, llvm_module_name: &str) -> &[String] {
self.imports.get(llvm_module_name).map(|v| &v[..]).unwrap_or(&[])
}

fn modules_exported_by(&self, llvm_module_name: &str) -> &[String] {
/// Returns modules exported by `llvm_module_name` during some ThinLTO pass.
fn exports_of(&self, llvm_module_name: &str) -> &[String] {
self.exports.get(llvm_module_name).map(|v| &v[..]).unwrap_or(&[])
}

Expand All @@ -921,7 +939,7 @@ impl ThinLTOImports {
Ok(())
}

fn load_from_file(path: &Path) -> io::Result<ThinLTOImports> {
fn load_from_file(path: &Path) -> io::Result<ThinLTOImportMaps> {
use std::io::BufRead;
let mut imports = FxHashMap::default();
let mut exports: FxHashMap<_, Vec<_>> = FxHashMap::default();
Expand All @@ -946,17 +964,17 @@ impl ThinLTOImports {
current_module = Some(line.trim().to_string());
}
}
Ok(ThinLTOImports { imports, exports })
Ok(ThinLTOImportMaps { imports, exports })
}

/// Loads the ThinLTO import map from ThinLTOData.
unsafe fn from_thin_lto_data(data: *const llvm::ThinLTOData) -> ThinLTOImports {
unsafe fn from_thin_lto_data(data: *const llvm::ThinLTOData) -> ThinLTOImportMaps {
unsafe extern "C" fn imported_module_callback(
payload: *mut libc::c_void,
importing_module_name: *const libc::c_char,
imported_module_name: *const libc::c_char,
) {
let map = &mut *(payload as *mut ThinLTOImports);
let map = &mut *(payload as *mut ThinLTOImportMaps);
let importing_module_name = CStr::from_ptr(importing_module_name);
let importing_module_name = module_name_to_str(&importing_module_name);
let imported_module_name = CStr::from_ptr(imported_module_name);
Expand All @@ -981,7 +999,7 @@ impl ThinLTOImports {
.push(importing_module_name.to_owned());
}

let mut map = ThinLTOImports::default();
let mut map = ThinLTOImportMaps::default();
llvm::LLVMRustGetThinLTOModuleImports(
data,
imported_module_callback,
Expand Down

0 comments on commit d05ae3a

Please sign in to comment.