Skip to content

[incremental] remove unnecessary passes #41251

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

Closed
10 tasks done
nikomatsakis opened this issue Apr 12, 2017 · 13 comments
Closed
10 tasks done

[incremental] remove unnecessary passes #41251

nikomatsakis opened this issue Apr 12, 2017 · 13 comments
Labels
A-incr-comp Area: Incremental compilation E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 12, 2017

This is a sub-issue of #40746. The follow list of tasks are all executed for side-effects. I believe they can all be straight-up removed (that is, we just don't create a task at all). The idea is to remove the call to with_task or in_task as well as the associated DepNode variant and see what happens -- if you get assertion failures, it may mean that they are writing to shared state, in which case we'll want to keep the task and refactor it in some other way.

An example PR is #41063. See e.g. the first commit which removed the CheckLoops stuff.

  • src/librustc_passes/loops.rs:53: let _task = map.dep_graph.in_task(DepNode::CheckLoops);
  • src/librustc_passes/static_recursion.rs:91: let _task = hir_map.dep_graph.in_task(DepNode::CheckStaticRecursion);
  • src/librustc_borrowck/borrowck/mod.rs:64: tcx.dep_graph.with_task(DepNode::BorrowCheckKrate, tcx, (), check_crate_task);
  • src/librustc/middle/effect.rs:244: let _task = tcx.dep_graph.in_task(DepNode::EffectCheck);
  • src/librustc/middle/liveness.rs:199: let _task = tcx.dep_graph.in_task(DepNode::Liveness);
  • src/librustc_typeck/check_unused.rs:65: let _task = tcx.dep_graph.in_task(DepNode::UnusedTraitCheck);
  • src/librustc/middle/dead.rs:597: let _task = tcx.dep_graph.in_task(DepNode::DeadCheck);
    • this would use the AccessLevels query described elsewhere
  • src/librustc_typeck/coherence/mod.rs:135: let _task = tcx.dep_graph.in_task(DepNode::Coherence);
  • src/librustc_typeck/coherence/overlap.rs:42: tcx.dep_graph.in_task(DepNode::CoherenceOverlapCheck(trait_def_id));
  • src/librustc_typeck/collect.rs:171: self.tcx.dep_graph.with_task(DepNode::CollectItemSig(def_id), self.tcx, id, op);
@nikomatsakis nikomatsakis added A-incr-comp Area: Incremental compilation E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Apr 12, 2017
@nikomatsakis
Copy link
Contributor Author

@aochagavia expressed interest in this from #40746

@nikomatsakis nikomatsakis added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Apr 13, 2017
@aochagavia
Copy link
Contributor

By the way, in the issue you mention that src/librustc/middle/dead.rs:597 needs to use an AccessLevels query. However, it seems that it is already being used on line 598: let access_levels = &ty::queries::privacy_access_levels::get(tcx, DUMMY_SP, LOCAL_CRATE);

@aochagavia
Copy link
Contributor

@nikomatsakis I have a commit where I remove 4 of the 5 passes listed here. However, now I am blocked by the following question:

The pass in src/librustc_typeck/coherence/mod.rs:135 used the DepNode::Coherence variant. After deleting the pass, I wanted to delete DepNode::Coherence as well, but I discovered that it is being used in multiple places. Particularly, it is being used by this function, which in turn is being used for something related to maps in rustc/ty/maps.rs (check out this line). Should those maps also be removed? I am unable to see if they are somehow related to the task I just deleted. Maybe I should just let DepNode::Coherence alone.

@aochagavia
Copy link
Contributor

Also, the pass in src/librustc_typeck/collect.rs:171 is part of a function that clearly has side effects. In fact, removing the pass would leave the function body empty. What course of action would you suggest here?

@eddyb
Copy link
Member

eddyb commented Apr 16, 2017

Yeah, don't remove nodes used by maps. Those are on-demand queries.

@aochagavia
Copy link
Contributor

@eddyb do you have any ideas regarding my second question?

@eddyb
Copy link
Member

eddyb commented Apr 18, 2017

@aochagavia I think you'd want to call op instead of removing it altogether, i.e.:

        self.with_collect_item_sig(item.id, convert_item);
// becomes
        convert_item(self.tcx, item.id);

@nikomatsakis
Copy link
Contributor Author

@aochagavia argh, just saw your comments here. All resolved?

I've been behind on notifications. I wound up accidentally doing some of this work in #41360, actually. Sorry if I stepped on your toes with that. :(

@aochagavia
Copy link
Contributor

@nikomatsakis There is indeed some overlap, so I will have to rebase once your changes land. On the other hand, if changes need to happen fast here maybe I could work on other parts of the compiler (for instance, this issue)

@Mark-Simulacrum
Copy link
Member

@nikomatsakis All the items in the issue are marked off, does that indicate this is done? If not, maybe OP should be updated with more items or a note added that there are more issues as well.

@nikomatsakis
Copy link
Contributor Author

I think this is done -- @aochagavia, you agree?

@aochagavia
Copy link
Contributor

@nikomatsakis I don't understand the question. What should I agree on? If this is done, it is done, right?

@nikomatsakis
Copy link
Contributor Author

@aochagavia true. =) I'm not sure precisely what I was asking for. (I guess if you had noticed anything I overlooked, but if so, we'll find it sooner or later...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

No branches or pull requests

4 participants