-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
incr.comp.: Make sure dependencies are recorded when feeding queries during eval-always queries. #109935
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 { | ||||||||||||||||||||||
|
@@ -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); | ||||||||||||||||||||||
|
@@ -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:?}") | ||||||||||||||||||||||
|
@@ -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 => {} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 One factor that might influence a decision here is that we seem to wrap everything inside of rust/compiler/rustc_middle/src/ty/context/tls.rs Lines 40 to 49 in a412564
If we changed that to But I haven't tried what the fallout of switching to Overall, I think |
||||||||||||||||||||||
TaskDepsRef::Forbid => { | ||||||||||||||||||||||
panic!("Cannot summarize when dependencies are not recorded.") | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
@@ -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, | ||||||||||||||||||||||
|
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> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
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 usedTaskDepsRef::Ignore
before this PR. Now that there is aTaskDepsRef::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.There was a problem hiding this comment.
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.