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.

Sign up for GitHub

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

Jump to bottom

Clarify cross-crate dependency story #32015

Closed
nikomatsakis opened this issue Mar 2, 2016 · 2 comments
Closed

Clarify cross-crate dependency story #32015

nikomatsakis opened this issue Mar 2, 2016 · 2 comments
Assignees
@nikomatsakis
Labels
A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone
Incremental compi...

Comments

@nikomatsakis
Copy link
Contributor

We're currently doing some wrong-ish things with respect to dep-graph tracking across crate boundaries. I think what I want to do is this:

  • Create a DepNode::MetaData(DefId) variant that plays a role like Hir
    • it basically represents the metadata for some item
  • Whenever we load from metadata for item X, we'll generate a read on MetaData(X)
  • In the metadata, we'll include a hash for each item, either:
    • hashing the saved contents of the item
    • or hashing all the base input nodes that are deps of the things serialized into
    • basically some hash we can use to tell if the metadata for an item changed in some way
  • When we save the dep-graph, we'll save the hash out of the crate metadata
  • When we load, we'll reload the hash out of the current crate metadata and compare

For inlined items, I hope we will transition to MIR and not need to inline sooner or later, but in the meantime, I think we can basically translate reads to Hir(X) where X is an inlined def-id to MetaData(Y) where Y is the original def-id -- we may also just be able to ignore accesses to inlined def-ids, since we'll presumably add an appropriate edge from some other source.
@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-compiler A-incr-comp Area: Incremental compilation labels Mar 2, 2016
@nikomatsakis nikomatsakis self-assigned this Mar 2, 2016
@nikomatsakis nikomatsakis mentioned this issue Mar 2, 2016
Merged
bors added a commit that referenced this issue Apr 6, 2016
@bors
Auto merge of #32016 - nikomatsakis:incr-comp-save, r=mw
b12ac68 
Save/load incremental compilation dep graph

Contains the code to serialize/deserialize the dep graph to disk between executions. We also hash the item contents and compare to the new hashes. Also includes a unit test harness. There are definitely some known limitations, such as #32014 and #32015, but I am leaving those for follow-up work.

Note that this PR builds on #32007, so the overlapping commits can be excluded from review.

r? @michaelwoerister
bors added a commit that referenced this issue Apr 7, 2016
@bors
Auto merge of #32016 - nikomatsakis:incr-comp-save, r=mw
7979dd6 
Save/load incremental compilation dep graph

Contains the code to serialize/deserialize the dep graph to disk between executions. We also hash the item contents and compare to the new hashes. Also includes a unit test harness. There are definitely some known limitations, such as #32014 and #32015, but I am leaving those for follow-up work.

Note that this PR builds on #32007, so the overlapping commits can be excluded from review.

r? @michaelwoerister
@michaelwoerister
Copy link
Member

I think this is implemented (mostly via #32016) except for correctly handling access to inlined HIR.
As far as I can see, there is no special handling of inlined HIR anywhere, which means that the following will happen:

  1. When an item gets inlined during trans, we will add a new HIR-depnode for the inline-copy of the item.
  2. This inline-copy has a local DefId, thus its hash will be stored with any other HIR dep-nodes in the dep-graph.
  3. When loading the dep-graph during the next round, the corresponding AST/HIR is not present, as it has not been inlined yet.
  4. As a consequence of (3), everything dependent on the inlined item will be invalidated.

@nikomatsakis
Copy link
Contributor Author

@michaelwoerister

I think this is implemented (mostly via #32016) except for correctly handling access to inlined HIR.

I agree -- however, it seems like inlined HIR is going away. That is, once we transition to MIR trans, this point will no longer be an issue, right?

@nikomatsakis nikomatsakis added this to the Incremental compilation alpha milestone Jul 23, 2016
@nikomatsakis nikomatsakis mentioned this issue Aug 9, 2016
Merged
bors added a commit that referenced this issue Aug 9, 2016
@bors
Auto merge of #35166 - nikomatsakis:incr-comp-ice-34991-2, r=mw
e1d2bc2 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nikomatsakis nikomatsakis

Labels
A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
Incremental compilation beta
Development

No branches or pull requests
2 participants
@nikomatsakis @michaelwoerister