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

incr.comp.: Make sure dependencies are recorded when feeding queries during eval-always queries. #109935

Merged
merged 2 commits into from
Apr 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 0 additions & 2 deletions compiler/rustc_passes/src/hir_id_validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ use rustc_middle::hir::nested_filter;
use rustc_middle::ty::TyCtxt;

pub fn check_crate(tcx: TyCtxt<'_>) {
tcx.dep_graph.assert_ignored();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually running inside of an eval-always query, not in with_ignore(). The two kinds both used TaskDepsRef::Ignore before this PR. Now that there is a TaskDepsRef::EvalAlways, the assertion fails.

Looking at the code again, I think we might actually want to just run it normally. I think the original with_ignore() scope was just a performance optimization when this check was run for release builds too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the DepGraph::with_ignore() scope in 5f52a96. It shouldn't make a difference for performance since the function doesn't do much in release builds.


if tcx.sess.opts.unstable_opts.hir_stats {
crate::hir_stats::print_hir_stats(tcx);
}
Expand Down
44 changes: 33 additions & 11 deletions compiler/rustc_query_system/src/dep_graph/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,16 +143,29 @@ impl<K: DepKind> DepGraph<K> {
assert_eq!(_green_node_index, DepNodeIndex::SINGLETON_DEPENDENCYLESS_ANON_NODE);

// Instantiate a dependy-less red node only once for anonymous queries.
let (_red_node_index, _prev_and_index) = current.intern_node(
let (red_node_index, red_node_prev_index_and_color) = current.intern_node(
profiler,
&prev_graph,
DepNode { kind: DepKind::RED, hash: Fingerprint::ZERO.into() },
smallvec![],
None,
false,
);
assert_eq!(_red_node_index, DepNodeIndex::FOREVER_RED_NODE);
assert!(matches!(_prev_and_index, None | Some((_, DepNodeColor::Red))));
assert_eq!(red_node_index, DepNodeIndex::FOREVER_RED_NODE);
match red_node_prev_index_and_color {
None => {
// This is expected when we have no previous compilation session.
assert!(prev_graph_node_count == 0);
}
Some((prev_red_node_index, DepNodeColor::Red)) => {
assert_eq!(prev_red_node_index.as_usize(), red_node_index.as_usize());
colors.insert(prev_red_node_index, DepNodeColor::Red);
}
Some((_, DepNodeColor::Green(_))) => {
// There must be a logic error somewhere if we hit this branch.
panic!("DepNodeIndex::FOREVER_RED_NODE evaluated to DepNodeColor::Green")
}
}

DepGraph {
data: Some(Lrc::new(DepGraphData {
Expand Down Expand Up @@ -353,10 +366,8 @@ impl<K: DepKind> DepGraphData<K> {
}))
};

let task_deps_ref = match &task_deps {
Some(deps) => TaskDepsRef::Allow(deps),
None => TaskDepsRef::Ignore,
};
let task_deps_ref =
task_deps.as_ref().map(TaskDepsRef::Allow).unwrap_or(TaskDepsRef::EvalAlways);

let result = K::with_deps(task_deps_ref, || task(cx, arg));
let edges = task_deps.map_or_else(|| smallvec![], |lock| lock.into_inner().reads);
Expand Down Expand Up @@ -461,6 +472,11 @@ impl<K: DepKind> DepGraph<K> {
K::read_deps(|task_deps| {
let mut task_deps = match task_deps {
TaskDepsRef::Allow(deps) => deps.lock(),
TaskDepsRef::EvalAlways => {
// We don't need to record dependencies of eval_always
// queries. They are re-evaluated unconditionally anyway.
return;
}
TaskDepsRef::Ignore => return,
TaskDepsRef::Forbid => {
panic!("Illegal read of: {dep_node_index:?}")
Expand Down Expand Up @@ -556,7 +572,10 @@ impl<K: DepKind> DepGraph<K> {
let mut edges = SmallVec::new();
K::read_deps(|task_deps| match task_deps {
TaskDepsRef::Allow(deps) => edges.extend(deps.lock().reads.iter().copied()),
TaskDepsRef::Ignore => {} // During HIR lowering, we have no dependencies.
TaskDepsRef::EvalAlways => {
edges.push(DepNodeIndex::FOREVER_RED_NODE);
}
TaskDepsRef::Ignore => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we insert this red dependency in both cases? Or should we ICE when feeding in ignore mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've been wondering about that too. I think adding a dependency would be surprising. We are inside a with_ignore() after all. But ICEing might be useful?

One factor that might influence a decision here is that we seem to wrap everything inside of TaskDepsRef::Ignore by default:

pub fn new(gcx: &'tcx GlobalCtxt<'tcx>) -> Self {
let tcx = TyCtxt { gcx };
ImplicitCtxt {
tcx,
query: None,
diagnostics: None,
query_depth: 0,
task_deps: TaskDepsRef::Ignore,
}
}

If we changed that to TaskDepsRef::Forbid there would be less chance of things being silently ignored and as a result less harm in treating TaskDepsRef::Ignore as "yes, really turn off dependency tracking". We use DepGraph::with_ignore() in a few places where we really don't want any dep-tracking to happen, e.g. when re-computing the value of a green node.

But I haven't tried what the fallout of switching to TaskDepsRef::Forbid would be. It might need some cleanup.

Overall, I think DepGraph::with_ignore() should really reliable turn off dep-tracking -- but we should use it in as few places as possible. This PR would be an example of replacing it with something more targeted.

TaskDepsRef::Forbid => {
panic!("Cannot summarize when dependencies are not recorded.")
}
Expand Down Expand Up @@ -1349,10 +1368,13 @@ pub enum TaskDepsRef<'a, K: DepKind> {
/// `TaskDeps`. This is used when executing a 'normal' query
/// (no `eval_always` modifier)
Allow(&'a Lock<TaskDeps<K>>),
/// New dependencies are ignored. This is used when
/// executing an `eval_always` query, since there's no
/// This is used when executing an `eval_always` query. We don't
/// need to track dependencies for a query that's always
/// re-executed. This is also used for `dep_graph.with_ignore`
/// re-executed -- but we need to know that this is an `eval_always`
/// query in order to emit dependencies to `DepNodeIndex::FOREVER_RED_NODE`
/// when directly feeding other queries.
EvalAlways,
/// New dependencies are ignored. This is also used for `dep_graph.with_ignore`.
Ignore,
/// Any attempt to add new dependencies will cause a panic.
/// This is used when decoding a query result from disk,
Expand Down
16 changes: 16 additions & 0 deletions tests/incremental/issue-108481-feed-eval-always.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// revisions: cpass1 cpass2

#![crate_type = "rlib"]

use std::fmt::Debug;

// MCVE kindly provided by Nilstrieb at
// https://github.com/rust-lang/rust/issues/108481#issuecomment-1493080185

#[derive(Debug)]
pub struct ConstGeneric<const CHUNK_SIZE: usize> {
_p: [(); CHUNK_SIZE],
}

#[cfg(cpass1)]
impl<const CHUNK_SIZE: usize> ConstGeneric<CHUNK_SIZE> {}