Address ICEs running w/ incremental compilation and building glium #35166
Conversation
|
☔ The latest upstream changes (presumably #35163) made this pull request unmergeable. Please resolve the merge conflicts. |
| s.push_str(&tcx.crate_name(self.krate)); | ||
| } else { | ||
| s.push_str(&tcx.sess.cstore.original_crate_name(self.krate)); | ||
| } |
There was a problem hiding this comment.
TyCtxt::crate_name() should already take care of the local/extern crate switch.
|
I'm not sure that I like how the Hir/Metadata problem is solved here via the graph post-processing step. It means that during the current compilation session, we still have a graph missing edges, right? Shouldn't you run into the |
The way we do HIR inlining introduces reads of the "Hir" into the graph, but this Hir in fact belongs to other crates, so when we try to load later, we ICE because the Hir nodes in question don't belond to the crate (and we haven't done inlining yet). This pass rewrites those HIR nodes to the metadata from which the inlined HIR was loaded.
The biggest problem, actually, is krate numbers being removed entirely, which can lead to array-index-out-of-bounds errors. cc rust-lang#35123 -- not a complete fix, since really we ought to "map" the old crate numbers to the new ones, not just detect changes.
The reads will occur naturally as the HIR/MIR is fetched from the tracked tables, and this winds up adding reads to the hir of foreign def-ids somehow.
When we hash the inputs to a MetaData node, we have to hash them in a consistent order. We achieve this by sorting the stringfied `DefPath` entries. Also, micro-optimie by cache more results across the saving process.
It's nice to get a rough idea of how much work we're saving.
nikomatsakis
force-pushed
the
incr-comp-ice-34991-2
branch
from
a11a4e2 to
903142a
Compare
As we said on IRC, I will investigate the alternate approach of "detecting" inlined ids and remapping more eagerly. |
We now detect inlined id's earlier (in the HIR map) and rewrite a read of them to be a read of the metadata for the associated item.
|
@michaelwoerister see last commit, which takes a different strategy that also seems to work. |
I cannot figure out how to write a test for this, but I observed incorrect edges as a result of not using memoized pattern here (e.g., LateLintCheck -> SizedConstraint).
this can actually be expensive!
| } | ||
|
|
||
| #[derive(Debug, RustcEncodable, RustcDecodable)] | ||
| pub struct KrateInfo { |
There was a problem hiding this comment.
Why KrateInfo instead of CrateInfo?
There was a problem hiding this comment.
Why KrateInfo instead of CrateInfo?
Because crate is a keyword... but yeah I realize now we often use C when we can (personally I would just always use K...).
|
Looks good to me now! I think the new metadata tracking strategy is a clear improvement over the graph post processing. |
|
@michaelwoerister do you want me to rename the uses of
|
Yes, please. |
Per the discussion on rust-lang#34765, we make one `DepNode::Mir` variant and use it to represent both the MIR tracking map as well as passes that operate on MIR. We also track loads of cached MIR (which naturally comes from metadata). Note that the "HAIR" pass adds a read of TypeckItemBody because it uses a myriad of tables that are not individually tracked.
it now carries a def-id; supply a dummy
|
@michaelwoerister ok, to fixup tests, I wound up fixing #34765 -- care to review last few commits :) |
The new `Predecessors` type computes a set of interesting targets and their HIR predecessors, and discards everything in between.
Produces a deterministic hash, at least for a single platform / compiler-version.
This massively speeds up serialization. It also seems to produce deterministic metadata hashes (before I was seeing inconsistent results). Fixes rust-lang#35232.
| @@ -538,8 +551,7 @@ impl<'b, 'tcx> SharedCrateContext<'b, 'tcx> { | |||
|
|
|||
| pub fn get_mir(&self, def_id: DefId) -> Option<CachedMir<'b, 'tcx>> { | |||
There was a problem hiding this comment.
This could probably use the memoize functions.
|
@bors r+ |
|
📌 Commit 82b6dc2 has been approved by |
|
@bors r=mw |
|
📌 Commit 76eecc7 has been approved by |
|
⌛ Testing commit 76eecc7 with merge d7012c1... |
|
@bors r=mw force |
|
📌 Commit e0b82d5 has been approved by |
Address ICEs running w/ incremental compilation and building glium Fixes for various ICEs I encountered trying to build glium with incremental compilation enabled. Building glium now works. Of the 4 ICEs, I have test cases for 3 of them -- I didn't isolate a test for the last commit and kind of want to go do other things -- most notably, figuring out why incremental isn't saving much *effort*. But if it seems worthwhile and I can come back and try to narrow down the problem. r? @michaelwoerister Fixes #34991 Fixes #32015
Fixes for various ICEs I encountered trying to build glium with incremental compilation enabled. Building glium now works. Of the 4 ICEs, I have test cases for 3 of them -- I didn't isolate a test for the last commit and kind of want to go do other things -- most notably, figuring out why incremental isn't saving much effort.
But if it seems worthwhile and I can come back and try to narrow down the problem.
r? @michaelwoerister
Fixes #34991
Fixes #32015