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

Use queries for the HIR map #68944

Merged
merged 34 commits into from
Mar 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
cfa1d4e
Add HIR queries
Zoxc Feb 7, 2020
21386e1
Collect the new maps
Zoxc Feb 7, 2020
518c78f
Create Map after TyCtxt
Zoxc Feb 7, 2020
e1a9626
Update item functions
Zoxc Feb 7, 2020
d3c7394
Update `fn_decl_by_hir_id` and `fn_sig_by_hir_id`
Zoxc Feb 7, 2020
0c68b7a
Update `body_owner` and `maybe_body_owned_by`
Zoxc Feb 7, 2020
38e613c
Update `krate_attrs` and `get_module`
Zoxc Feb 7, 2020
b40e6ba
Update `visit_item_likes_in_module`
Zoxc Feb 7, 2020
d5827d8
Update `get_parent_node`
Zoxc Feb 7, 2020
9c4308e
Update `find`
Zoxc Feb 7, 2020
21942a5
Update `is_hir_id_module`
Zoxc Feb 7, 2020
61527c8
Update `find_entry`
Zoxc Feb 7, 2020
270ee7e
Remove comments
Zoxc Feb 8, 2020
072449c
Update `trait_impls`
Zoxc Feb 8, 2020
d99b17f
Remove the `map` field from `Map`
Zoxc Feb 8, 2020
fa09db8
Remove `AllLocalTraitImpls`
Zoxc Feb 8, 2020
e9d166f
Clean up the collector
Zoxc Feb 8, 2020
b97d438
Remove `Hir` and `HirBody` dep nodes
Zoxc Feb 8, 2020
d73268b
Remove `input_task`
Zoxc Feb 9, 2020
3538cb3
Only hash the Hir owner (including its bodies)
Zoxc Feb 9, 2020
8b16b02
Index HIR after creating TyCtxt
Zoxc Feb 9, 2020
0e316e2
Fix HIR map validation
Zoxc Feb 9, 2020
aea57ae
Don't hash HIR with bodies thrice
Zoxc Feb 10, 2020
396aeb8
Optimize the HIR map
Zoxc Feb 10, 2020
739a1ef
Create the `hir_to_node_id` map before `TyCtxt`
Zoxc Feb 12, 2020
c0b60c4
Replace `HirBody` with `hir_owner_items` in tests
Zoxc Feb 12, 2020
274fb66
Replace `Hir` with `hir_owner` in tests
Zoxc Feb 12, 2020
10b23e3
Format function_interfaces.rs
Zoxc Feb 12, 2020
6258c01
Reintroduce workaround for #62649
Zoxc Feb 12, 2020
7118f71
Update ich_nested_items.rs
Zoxc Feb 12, 2020
8b8041e
Update tests
Zoxc Feb 13, 2020
2af085d
Don't try to print missing HIR ids
Zoxc Feb 20, 2020
31183c3
Add test for #69596
Zoxc Mar 10, 2020
14fdd85
Add some comments to the new queries
Zoxc Mar 11, 2020
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
6 changes: 6 additions & 0 deletions src/librustc/arena.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,12 @@ macro_rules! arena_types {
[] type_binding: rustc_hir::TypeBinding<$tcx>,
[] variant: rustc_hir::Variant<$tcx>,
[] where_predicate: rustc_hir::WherePredicate<$tcx>,

// HIR query types
[few] indexed_hir: rustc::hir::map::IndexedHir<$tcx>,
[few] hir_definitions: rustc::hir::map::definitions::Definitions,
[] hir_owner: rustc::hir::HirOwner<$tcx>,
[] hir_owner_items: rustc::hir::HirOwnerItems<$tcx>,
], $tcx);
)
}
Expand Down
23 changes: 4 additions & 19 deletions src/librustc/dep_graph/dep_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
//! "infer" some properties for each kind of `DepNode`:
//!
//! * Whether a `DepNode` of a given kind has any parameters at all. Some
//! `DepNode`s, like `AllLocalTraitImpls`, represent global concepts with only one value.
//! `DepNode`s could represent global concepts with only one value.
//! * Whether it is possible, in principle, to reconstruct a query key from a
//! given `DepNode`. Many `DepKind`s only require a single `DefId` parameter,
//! in which case it is possible to map the node's fingerprint back to the
Expand Down Expand Up @@ -223,8 +223,8 @@ macro_rules! define_dep_nodes {
/// Construct a DepNode from the given DepKind and DefPathHash. This
/// method will assert that the given DepKind actually requires a
/// single DefId/DefPathHash parameter.
pub fn from_def_path_hash(kind: DepKind,
def_path_hash: DefPathHash)
pub fn from_def_path_hash(def_path_hash: DefPathHash,
kind: DepKind)
-> DepNode {
debug_assert!(kind.can_reconstruct_query_key() && kind.has_params());
DepNode {
Expand Down Expand Up @@ -280,7 +280,7 @@ macro_rules! define_dep_nodes {
}

if kind.has_params() {
Ok(def_path_hash.to_dep_node(kind))
Ok(DepNode::from_def_path_hash(def_path_hash, kind))
} else {
Ok(DepNode::new_no_params(kind))
}
Expand Down Expand Up @@ -337,28 +337,13 @@ impl fmt::Debug for DepNode {
}
}

impl DefPathHash {
pub fn to_dep_node(self, kind: DepKind) -> DepNode {
DepNode::from_def_path_hash(kind, self)
}
}

rustc_dep_node_append!([define_dep_nodes!][ <'tcx>
// We use this for most things when incr. comp. is turned off.
[] Null,

// Represents the body of a function or method. The def-id is that of the
// function/method.
[eval_always] HirBody(DefId),

// Represents the HIR node with the given node-id
[eval_always] Hir(DefId),

// Represents metadata from an extern crate.
[eval_always] CrateMetadata(CrateNum),

[eval_always] AllLocalTraitImpls,

[anon] TraitSelect,

[] CompileCodegenUnit(Symbol),
Expand Down
45 changes: 15 additions & 30 deletions src/librustc/dep_graph/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,28 +225,6 @@ impl DepGraph {
)
}

/// Creates a new dep-graph input with value `input`
pub fn input_task<'a, C, R>(&self, key: DepNode, cx: C, input: R) -> (R, DepNodeIndex)
where
C: DepGraphSafe + StableHashingContextProvider<'a>,
R: for<'b> HashStable<StableHashingContext<'b>>,
{
fn identity_fn<C, A>(_: C, arg: A) -> A {
arg
}

self.with_task_impl(
key,
cx,
input,
true,
identity_fn,
|_| None,
|data, key, fingerprint, _| data.alloc_node(key, SmallVec::new(), fingerprint),
hash_result::<R>,
)
}

fn with_task_impl<'a, C, A, R>(
&self,
key: DepNode,
Expand Down Expand Up @@ -676,18 +654,25 @@ impl DepGraph {
continue;
}
} else {
// FIXME: This match is just a workaround for incremental bugs and should
// be removed. https://github.com/rust-lang/rust/issues/62649 is one such
// bug that must be fixed before removing this.
match dep_dep_node.kind {
DepKind::Hir | DepKind::HirBody | DepKind::CrateMetadata => {
DepKind::hir_owner
| DepKind::hir_owner_items
| DepKind::CrateMetadata => {
Copy link
Member

Choose a reason for hiding this comment

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

I see how this is still relevant for DepKind::CrateMetadata but I'm surprised that it's needed for DepKind::hir_owner and DepKind::hir_owner_items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that hir_owner is just a rename of Hir and hir_owner_items is just a rename of HirBody, you can follow the explanation here to see how this has the same problem as Hir and HirBody. Also Hir(Something) will be replaced by hir_owner_items(Something) there, but that doesn't affect anything.

Copy link
Member

Choose a reason for hiding this comment

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

I had a chance to test this and I think the explanation in #62649 (comment) basically applies here:

  • Something::foo is a valid query key for hir_owner in rpass1 but in rpass2 it's not a valid key anymore.
  • The red-green algorithm does not detect this because RecoverKey::recover() returns Some(_) instead of None. The algorithm thus goes ahead with forcing DepNode::hir_owner(Something::foo) and ICEs.

A short-term fix for this would be to have a newtype struct HirOwnerDefId(DefId) that is a DefId with the additional invariant that it must correspond to a hir-owner. Then <HirOwnerDefId as RecoverKey>::recover() could apply the same check as done here and return None if it doesn't hold. Most uses of HirOwnerDefId should stay hidden behind the hir map wrapper, so this shouldn't be too disruptive.

Another workaround would be to diversify the DefPathData variants again so that a field and a method would not have the same DefPath anymore. Either fixes would be acceptable solutions for unblocking this PR, in my opinion.

I think one way to fix the problem at the conceptual level would be to add the invariant that RecoverKey::recover() must only return Some(_) if the recovered key is actually a valid query-key for the given query. That is, the result of RecoverKey::recover() would need to depend on the query, not just on the type of the query-key. It would then be able to do query-specific runtime checks about key validity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't block this PR though. It's just #68289 ported to it. There's no further problems caused by this PR compared to master.

It seems like you're trying to fix #62649 here. I mentioned the DefPathData solution in a comment) there. I want to check the performance of making as_local_hir_id a query, but that is blocked on this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I'm fine with merging this if you mention #62649 in the FIXME comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a reference to that issue.

if let Some(def_id) = dep_dep_node.extract_def_id(tcx) {
if def_id_corresponds_to_hir_dep_node(tcx, def_id) {
// The `DefPath` has corresponding node,
// and that node should have been marked
// either red or green in `data.colors`.
bug!(
"DepNode {:?} should have been \
if dep_dep_node.kind == DepKind::CrateMetadata {
// The `DefPath` has corresponding node,
// and that node should have been marked
// either red or green in `data.colors`.
bug!(
"DepNode {:?} should have been \
pre-marked as red or green but wasn't.",
dep_dep_node
);
dep_dep_node
);
}
} else {
// This `DefPath` does not have a
// corresponding `DepNode` (e.g. a
Expand Down
Loading