Skip to content
This repository was archived by the owner on Jan 7, 2022. It is now read-only.

Cache the results of the monomorphisation call resolver #79

Closed
wants to merge 2 commits into from
Closed

Cache the results of the monomorphisation call resolver #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db183db
Cache the results of the monomorphisation call resolver.
vext01 Dec 2, 2019
de2c5d9
Cycle breaker.
vext01 Dec 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/librustc/ty/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use crate::ty::subst::{GenericArg, InternalSubsts, SubstsRef, Subst};
use crate::ty::ReprOptions;
use crate::traits;
use crate::traits::{Clause, Clauses, GoalKind, Goal, Goals};
use crate::ty::{self, DefIdTree, Ty, TypeAndMut};
use crate::ty::{self, DefIdTree, Ty, TypeAndMut, Instance};
use crate::ty::{TyS, TyKind, List};
use crate::ty::{AdtKind, AdtDef, Region, Const};
use crate::ty::{PolyFnSig, InferTy, ParamTy, ProjectionTy, ExistentialPredicate, Predicate};
Expand Down Expand Up @@ -1107,6 +1107,10 @@ pub struct GlobalCtxt<'tcx> {
layout_interner: ShardedHashMap<&'tcx LayoutDetails, ()>,

output_filenames: Arc<OutputFilenames>,

/// Caches the results of `Instance::resolve()` so that Yorick's SIR lowering can use them
/// later. Normally only the monomorphisation collector can resolve instances.
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's not true. The backend can call Instance::resolve*.

pub call_resolution_map: Lock<Option<FxHashMap<(DefId, SubstsRef<'tcx>), Instance<'tcx>>>>,
}

impl<'tcx> TyCtxt<'tcx> {
Expand Down Expand Up @@ -1302,6 +1306,7 @@ impl<'tcx> TyCtxt<'tcx> {
allocation_interner: Default::default(),
alloc_map: Lock::new(interpret::AllocMap::new()),
output_filenames: Arc::new(output_filenames.clone()),
call_resolution_map: Lock::new(None),
}
}

Expand Down
11 changes: 11 additions & 0 deletions src/librustc_mir/monomorphize/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,13 @@ pub fn collect_crate_mono_items(
tcx: TyCtxt<'_>,
mode: MonoItemCollectionMode,
) -> (FxHashSet<MonoItem<'_>>, InliningMap<'_>) {
// Initialise Yorick call resolution map.
{
let mut resolutions = tcx.call_resolution_map.borrow_mut();
assert!(resolutions.is_none()); // Should only be initialised once.
*resolutions = Some(FxHashMap::default());
}

let _prof_timer = tcx.prof.generic_activity("monomorphization_collector");

let roots = time(tcx.sess, "collecting roots", || {
Expand Down Expand Up @@ -727,6 +734,10 @@ fn visit_fn_use<'tcx>(
ty::Instance::resolve_for_fn_ptr
};
let instance = resolver(tcx, ty::ParamEnv::reveal_all(), def_id, substs).unwrap();

let mut resolutions = tcx.call_resolution_map.borrow_mut();
resolutions.as_mut().unwrap().insert((def_id, substs), instance);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not correct. Doing a cast to an fnptr can give a different result than a direct call, while both defid and substs are the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be correct if the hashmap were keyed (def_id, substs, caller_pos) where caller_pos identifies the position of the call terminator in question?

Copy link
Collaborator

@bjorn3 bjorn3 Dec 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but only when you don't codegen generic functions. When codegenning generic functions, you will really need to monomorphize the substs and then use Instance::resolve* in the codegen backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's what I feared. We may have to make some fairly substantial changes to our system in light of this. Thanks again for this discussion.

By the way, between us we could probably implement a missing part of the rustc guide here:
https://rust-lang.github.io/rustc-guide/mir/index.html?highlight=instance#mir-data-types

The main MIR data type is Mir. It contains the data for a single function (along with sub-instances of Mir for "promoted constants", but you can read about those below).

And the link to below is "to be written".

So it seems that the type parameters become "promoted constants" during monomorphisation.

(I also think the main MIR data type is now Body not Mir)

Copy link
Collaborator

Choose a reason for hiding this comment

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

So it seems that the type parameters become "promoted constants" during monomorphisation.

What do you mean?

(I also think the main MIR data type is now Body not Mir)

Mir has been renamed to Body recently.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

The wording in the compiler guide talks in these terms, which surprised me a little:

The main MIR data type is Mir. It contains the data for a single function (along with sub-instances of Mir for "promoted constants"

It's talking about making a generic type parameter into a concrete one, I think? Through the act of promotion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is actually talking about turning

fn abc() {
    let a: &'static u8 = 1 + 2;
}

into

fn abc() {
    static PROMOTED0: u8 = 1 + 2;
    let a: &'static u8 = &PROMOTED0;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. We could probably improve the wording there too.


visit_instance_use(tcx, instance, is_direct_call, output);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_yk_sections/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ test = false
[dependencies]
rustc = { path = "../librustc" }
rustc_yk_link = { path = "../librustc_yk_link" }
ykpack = { git = "https://github.com/softdevteam/yk" }
ykpack = { git = "https://github.com/vext01/yk", rev = "b4da4c7d08661b38f265fe072f88d354a2c4c8fa" }
rustc_index = { path = "../librustc_index" }
rustc_codegen_utils = { path = "../librustc_codegen_utils" }
rustc_data_structures = { path = "../librustc_data_structures" }
Expand Down
31 changes: 22 additions & 9 deletions src/librustc_yk_sections/emit_sir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
//!
//! Serialisation itself is performed by an external library: ykpack.

use rustc::ty::{self, TyCtxt, TyS, Const, Ty, Instance};
use rustc::ty::{self, TyCtxt, TyS, Const, Ty};
use syntax::ast::{UintTy, IntTy};
use rustc::hir::def_id::{DefId, LOCAL_CRATE};
use rustc::mir::{
Expand All @@ -34,6 +34,7 @@ use std::convert::{TryFrom, TryInto};
use rustc_index::vec::IndexVec;
use ykpack;
use rustc::ty::fold::TypeFoldable;
use rustc::ty::InstanceDef;
use syntax_pos::sym;

const SECTION_NAME: &'static str = ".yk_sir";
Expand Down Expand Up @@ -210,14 +211,26 @@ impl<'a, 'tcx> ConvCx<'a, 'tcx> {
}, ..
}, ..
}, ..) = func {
// A statically known call target.
let inst = Instance::new(*target_def_id, substs);
let sym_name = match substs.needs_subst() {
// If the instance isn't full instantiated, then it has no symbol name.
true => None,
false => Some(String::from(&*self.tcx.symbol_name(inst).name.as_str())),
};
ykpack::CallOperand::Fn(self.lower_def_id(target_def_id), sym_name)
let map = self.tcx.call_resolution_map.borrow();
let maybe_inst = map.as_ref().unwrap().get(&(*target_def_id, substs));
if let Some(inst) = maybe_inst {
let sym_name = match substs.needs_subst() {
// If the instance isn't fully instantiated, then it has no symbol name.
true => None,
false => Some(String::from(
&*self.tcx.symbol_name(*inst).name.as_str())),
};

match inst.def {
InstanceDef::Item(def_id) => ykpack::CallOperand::Fn(
self.lower_def_id(&def_id), sym_name),
InstanceDef::Virtual(def_id, _) => ykpack::CallOperand::Virtual(
self.lower_def_id(&def_id), sym_name),
_ => ykpack::CallOperand::Unknown,
}
} else {
ykpack::CallOperand::Unknown
}
} else {
// FIXME -- implement other callables.
ykpack::CallOperand::Unknown
Expand Down