Skip to content

incr.comp.: Refactor in_scope_traits query to take DefId instead of HirId as query-key #44414

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
Closed
michaelwoerister opened this issue Sep 8, 2017 · 4 comments
Closed

incr.comp.: Refactor in_scope_traits query to take DefId instead of HirId as query-key #44414

michaelwoerister opened this issue Sep 8, 2017 · 4 comments
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@michaelwoerister
Copy link
Member

It must be possible for all "input" queries to reconstruct their query-key from their DepNode. The easiest way to accomplish this is to make the query-key a DefId.

The in_scope_traits query violates this condition at the moment. A simple way to fix this would be to make it a two-level map: The query returns a per-item map that is indexed by ItemLocalId. The query-key would be the HirId::owner.

cc @alexcrichton @nikomatsakis
@michaelwoerister michaelwoerister added the A-incr-comp Area: Incremental compilation label Sep 8, 2017
@alexcrichton
Copy link
Member

Oh is HirId not a suitable key for queries? Apologies if I mistakenly thought it was!

There's a few other queries that use HirId right now as well, and will those need to be fixed as well?

@michaelwoerister
Copy link
Member Author

Well, let's say DefId is better because in that case we can reconstruct the query-key from the DepNode. If it's really needed we could provide that same support for HirId, but that would mean that we have to always build a Fingerprint -> HirId table during HIR lowering (we already do something like that for DefPathHash -> DefId).

@alexcrichton alexcrichton mentioned this issue Sep 8, 2017
Merged
@alexcrichton
Copy link
Member

I took a slightly different approach in #44435 which just blanket converts to DefId, but let me know if that's in error! (there was quite a few new queries using HirId which also needed to change and it didn't seem so obvious for a two-level cache)

@alexcrichton alexcrichton added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 8, 2017
@michaelwoerister
Copy link
Member Author

See #44435 (comment). I have not yet looked at the other queries that use HirId as key.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Sep 11, 2017
@alexcrichton
rustc: Remove HirId from queries
caaf365 
This'll allow us to reconstruct query parameters purely from the `DepNode`
they're associated with. Some queries could move straight to `HirId` but others
that don't always have a correspondance between `HirId` and `DefId` moved to
two-level maps where the query operates over a `DefIndex`, returning a map,
which is then keyed off `ItemLocalId`.

Closes rust-lang#44414
bors added a commit that referenced this issue Sep 11, 2017
@bors
Auto merge of #44435 - alexcrichton:in-scope, r=michaelwoerister
efa3ec6 
rustc: Remove HirId from queries

This'll allow us to reconstruct query parameters purely from the `DepNode`
they're associated with.

Closes #44414
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@alexcrichton @michaelwoerister