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.

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

Prepare MutVisitors to handle interned projections #65197

Merged
merged 12 commits into from
Oct 19, 2019

Conversation

spastorino
Copy link
Member

The following are all the files where mir's MutVisitor is implemented. The - there stands for no changes, visit_place wasn't making any change on Places. x stands for this file was changed to make visit_place do whatever it was doing with the base but avoid modifying the projection, instead just create a new one and assign to it.

[-] src/librustc_mir/transform/no_landing_pads.rs
[x] src/librustc_mir/transform/promote_consts.rs
[x] src/librustc_mir/transform/generator.rs
[x] src/librustc_mir/transform/erase_regions.rs
[-] src/librustc_mir/transform/instcombine.rs
[x] src/librustc_mir/transform/inline.rs
[x] src/librustc_mir/transform/simplify.rs
[x] src/librustc_mir/util/def_use.rs
[-] src/librustc_mir/transform/const_prop.rs
[-] src/librustc_mir/transform/cleanup_post_borrowck.rs
[x] src/librustc_mir/borrow_check/nll/renumber.rs
[-] src/librustc_mir/transform/copy_prop.rs

There is some code repetition, just created the PR so we can start discussing it.

/cc @oli-obk @nikomatsakis

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2019
@Centril
Copy link
Contributor

Centril commented Oct 8, 2019

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned estebank Oct 8, 2019
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try to reuse the existing visit_local more instead of duplicating its logic (even if the code is super tiny). You could always clone the elem and mutate it by calling self.visit_local. Or create a LocalMutVisitor trait that internally uses MutVisitor and does the vec->mutate->collect->into_boxed_slice dance

src/librustc_mir/borrow_check/nll/renumber.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/nll/renumber.rs Outdated Show resolved Hide resolved
src/librustc_mir/borrow_check/nll/renumber.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/generator.rs Show resolved Hide resolved
src/librustc_mir/transform/generator.rs Show resolved Hide resolved
src/librustc_mir/transform/generator.rs Show resolved Hide resolved
src/librustc_mir/transform/inline.rs Outdated Show resolved Hide resolved
@spastorino spastorino force-pushed the place-mut-visitor-adjusts2 branch 2 times, most recently from ec5e77f to 021a77b Compare October 8, 2019 18:08
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-10-08T18:46:01.1269711Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-08T18:46:01.1491341Z ##[command]git config gc.auto 0
2019-10-08T18:46:01.1583902Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-08T18:46:01.1664782Z ##[command]git config --get-all http.proxy
2019-10-08T18:46:01.1811414Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65197/merge:refs/remotes/pull/65197/merge
---
2019-10-08T18:53:30.9124041Z    Compiling serde_json v1.0.40
2019-10-08T18:53:33.0998357Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-10-08T18:53:45.1501718Z     Finished release [optimized] target(s) in 1m 37s
2019-10-08T18:53:45.1637059Z tidy check
2019-10-08T18:53:45.8784674Z tidy error: /checkout/src/librustc/mir/visit.rs:798: line longer than 100 chars
2019-10-08T18:53:47.2235647Z some tidy checks failed
2019-10-08T18:53:47.2238966Z 
2019-10-08T18:53:47.2238966Z 
2019-10-08T18:53:47.2249711Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-10-08T18:53:47.2251227Z 
2019-10-08T18:53:47.2251263Z 
2019-10-08T18:53:47.2259650Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-10-08T18:53:47.2260089Z Build completed unsuccessfully in 0:01:40
2019-10-08T18:53:47.2260089Z Build completed unsuccessfully in 0:01:40
2019-10-08T18:53:47.2314515Z == clock drift check ==
2019-10-08T18:53:47.2329962Z   local time: Tue Oct  8 18:53:47 UTC 2019
2019-10-08T18:53:47.3957172Z   network time: Tue, 08 Oct 2019 18:53:47 GMT
2019-10-08T18:53:47.3960811Z == end clock drift check ==
2019-10-08T18:53:48.7723724Z ##[error]Bash exited with code '1'.
2019-10-08T18:53:48.7781085Z ##[section]Starting: Checkout
2019-10-08T18:53:48.7782874Z ==============================================================================
2019-10-08T18:53:48.7782950Z Task         : Get sources
2019-10-08T18:53:48.7783001Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@spastorino spastorino force-pushed the place-mut-visitor-adjusts2 branch 2 times, most recently from 6b9316b to 04e491a Compare October 9, 2019 16:26
src/librustc/mir/visit.rs Show resolved Hide resolved
src/librustc/mir/visit.rs Outdated Show resolved Hide resolved
@spastorino
Copy link
Member Author

@oli-obk @nikomatsakis I've fixed the things we have discussed and I've decided to split this PR in the let's not mutate projection and the interning part so we can make progress. I don't see a reason why this would regress performance, anyway just asking bots to run perf on this PR.

So the main discussion is if we want this macro or something like the following design proposed by @nikomatsakis ...

trait PlaceVisitor: MirVisitor { // no MutVisitor equivalent
  fn visit_projection(...);
}

impl<'tcx> MirVisitor<'tcx> for Promoter<'_, 'tcx> {
  delegate_super_place!(self);
  // macro creates:
  fn super_place(self) {
    
    for each projection { self.visit_projection(...); }
  }
}

impl<'a, 'tcx> PlaceVisitor<'tcx> for Promoter<'a, 'tcx> {
}

If I remember correctly Niko said that this was going to be worser than my solution but I don't remember why.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 11, 2019

⌛ Trying commit cdf33d9 with merge dc67089...

bors added a commit that referenced this pull request Oct 11, 2019
Prepare `MutVisitor`s to handle interned projections

The following are all the files where mir's `MutVisitor` is implemented. The `-` there stands for no changes, `visit_place` wasn't making any change on `Place`s. `x` stands for this file was changed to make `visit_place` do whatever it was doing with the base but avoid modifying the projection, instead just create a new one and assign to it.

```
[-] src/librustc_mir/transform/no_landing_pads.rs
[x] src/librustc_mir/transform/promote_consts.rs
[x] src/librustc_mir/transform/generator.rs
[x] src/librustc_mir/transform/erase_regions.rs
[-] src/librustc_mir/transform/instcombine.rs
[x] src/librustc_mir/transform/inline.rs
[x] src/librustc_mir/transform/simplify.rs
[x] src/librustc_mir/util/def_use.rs
[-] src/librustc_mir/transform/const_prop.rs
[-] src/librustc_mir/transform/cleanup_post_borrowck.rs
[x] src/librustc_mir/borrow_check/nll/renumber.rs
[-] src/librustc_mir/transform/copy_prop.rs
```

There is some code repetition, just created the PR so we can start discussing it.

/cc @oli-obk @nikomatsakis
@bors
Copy link
Contributor

bors commented Oct 11, 2019

☀️ Try build successful - checks-azure
Build commit: dc67089 (dc6708993f37169d5d34e73401009e506dbcffb6)

@rust-timer
Copy link
Collaborator

Queued dc67089 with parent d4f7f97, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit dc67089, comparison URL.

@Centril
Copy link
Contributor

Centril commented Oct 12, 2019

Perf seems like noise

@spastorino
Copy link
Member Author

Yeap, this is a step before interning where perf I guess there should be some perf wins. So this is ready to merge re-review and merge.

@spastorino spastorino force-pushed the place-mut-visitor-adjusts2 branch 4 times, most recently from b766eb2 to ae806f9 Compare October 18, 2019 14:04
@oli-obk
Copy link
Contributor

oli-obk commented Oct 18, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Oct 18, 2019

📌 Commit 4834996 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 18, 2019
…s2, r=oli-obk

Prepare `MutVisitor`s to handle interned projections

The following are all the files where mir's `MutVisitor` is implemented. The `-` there stands for no changes, `visit_place` wasn't making any change on `Place`s. `x` stands for this file was changed to make `visit_place` do whatever it was doing with the base but avoid modifying the projection, instead just create a new one and assign to it.

```
[-] src/librustc_mir/transform/no_landing_pads.rs
[x] src/librustc_mir/transform/promote_consts.rs
[x] src/librustc_mir/transform/generator.rs
[x] src/librustc_mir/transform/erase_regions.rs
[-] src/librustc_mir/transform/instcombine.rs
[x] src/librustc_mir/transform/inline.rs
[x] src/librustc_mir/transform/simplify.rs
[x] src/librustc_mir/util/def_use.rs
[-] src/librustc_mir/transform/const_prop.rs
[-] src/librustc_mir/transform/cleanup_post_borrowck.rs
[x] src/librustc_mir/borrow_check/nll/renumber.rs
[-] src/librustc_mir/transform/copy_prop.rs
```

There is some code repetition, just created the PR so we can start discussing it.

/cc @oli-obk @nikomatsakis
tmandry added a commit to tmandry/rust that referenced this pull request Oct 18, 2019
…s2, r=oli-obk

Prepare `MutVisitor`s to handle interned projections

The following are all the files where mir's `MutVisitor` is implemented. The `-` there stands for no changes, `visit_place` wasn't making any change on `Place`s. `x` stands for this file was changed to make `visit_place` do whatever it was doing with the base but avoid modifying the projection, instead just create a new one and assign to it.

```
[-] src/librustc_mir/transform/no_landing_pads.rs
[x] src/librustc_mir/transform/promote_consts.rs
[x] src/librustc_mir/transform/generator.rs
[x] src/librustc_mir/transform/erase_regions.rs
[-] src/librustc_mir/transform/instcombine.rs
[x] src/librustc_mir/transform/inline.rs
[x] src/librustc_mir/transform/simplify.rs
[x] src/librustc_mir/util/def_use.rs
[-] src/librustc_mir/transform/const_prop.rs
[-] src/librustc_mir/transform/cleanup_post_borrowck.rs
[x] src/librustc_mir/borrow_check/nll/renumber.rs
[-] src/librustc_mir/transform/copy_prop.rs
```

There is some code repetition, just created the PR so we can start discussing it.

/cc @oli-obk @nikomatsakis
bors added a commit that referenced this pull request Oct 18, 2019
Rollup of 17 pull requests

Successful merges:

 - #65016 (Always inline `mem::{size_of,align_of}` in debug builds)
 - #65197 (Prepare `MutVisitor`s to handle interned projections)
 - #65201 (Disable Go and OCaml bindings when building LLVM)
 - #65364 (Collect occurrences of empty blocks for mismatched braces diagnostic)
 - #65417 (Add more coherence tests)
 - #65434 (Add long error explanation for E0577)
 - #65455 (Avoid unnecessary `TokenTree` to `TokenStream` conversions)
 - #65472 (Use a sharded dep node to dep node index map)
 - #65480 (Speed up `LexicalResolve::expansion()`)
 - #65496 (properly document panics in div_euclid and rem_euclid)
 - #65508 (add option to ping llvm ice-breakers to triagebot)
 - #65511 (save-analysis: Nest tables when processing impl block definitions)
 - #65513 (reorder fmt docs for more clarity)
 - #65532 (doc: make BitSet intro more short)
 - #65540 (show up some extra info when t!() fails)
 - #65549 (Fix left/right shift typo in wrapping rotate docs)
 - #65552 (Clarify diagnostics when using `~` as a unary op)

Failed merges:

 - #65471 (Add long error explanation for E0578)

r? @ghost
tmandry added a commit to tmandry/rust that referenced this pull request Oct 18, 2019
…s2, r=oli-obk

Prepare `MutVisitor`s to handle interned projections

The following are all the files where mir's `MutVisitor` is implemented. The `-` there stands for no changes, `visit_place` wasn't making any change on `Place`s. `x` stands for this file was changed to make `visit_place` do whatever it was doing with the base but avoid modifying the projection, instead just create a new one and assign to it.

```
[-] src/librustc_mir/transform/no_landing_pads.rs
[x] src/librustc_mir/transform/promote_consts.rs
[x] src/librustc_mir/transform/generator.rs
[x] src/librustc_mir/transform/erase_regions.rs
[-] src/librustc_mir/transform/instcombine.rs
[x] src/librustc_mir/transform/inline.rs
[x] src/librustc_mir/transform/simplify.rs
[x] src/librustc_mir/util/def_use.rs
[-] src/librustc_mir/transform/const_prop.rs
[-] src/librustc_mir/transform/cleanup_post_borrowck.rs
[x] src/librustc_mir/borrow_check/nll/renumber.rs
[-] src/librustc_mir/transform/copy_prop.rs
```

There is some code repetition, just created the PR so we can start discussing it.

/cc @oli-obk @nikomatsakis
bors added a commit that referenced this pull request Oct 19, 2019
Rollup of 19 pull requests

Successful merges:

 - #65016 (Always inline `mem::{size_of,align_of}` in debug builds)
 - #65197 (Prepare `MutVisitor`s to handle interned projections)
 - #65201 (Disable Go and OCaml bindings when building LLVM)
 - #65334 (Add long error explanation for E0575)
 - #65364 (Collect occurrences of empty blocks for mismatched braces diagnostic)
 - #65455 (Avoid unnecessary `TokenTree` to `TokenStream` conversions)
 - #65472 (Use a sharded dep node to dep node index map)
 - #65480 (Speed up `LexicalResolve::expansion()`)
 - #65493 (Add long error explanation for E0584)
 - #65496 (properly document panics in div_euclid and rem_euclid)
 - #65498 (Plugins deprecation: don’t suggest simply removing the attribute)
 - #65508 (add option to ping llvm ice-breakers to triagebot)
 - #65511 (save-analysis: Nest tables when processing impl block definitions)
 - #65513 (reorder fmt docs for more clarity)
 - #65532 (doc: make BitSet intro more short)
 - #65535 (rustc: arena-allocate the slice in `ty::GenericsPredicate`, not the whole struct.)
 - #65540 (show up some extra info when t!() fails)
 - #65549 (Fix left/right shift typo in wrapping rotate docs)
 - #65552 (Clarify diagnostics when using `~` as a unary op)

Failed merges:

 - #65390 (Add long error explanation for E0576)
 - #65434 (Add long error explanation for E0577)
 - #65471 (Add long error explanation for E0578)

r? @ghost
@bors bors merged commit 4834996 into rust-lang:master Oct 19, 2019
@bors
Copy link
Contributor

bors commented Oct 19, 2019

☔ The latest upstream changes (presumably #65570) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 19, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 25, 2019
… r=oli-obk

Intern place projection

This should sit on top of rust-lang#65197. After that one merged, I'm gonna rebase on top of it.

The important commits are the last three and there's a bunch of code repetition that I'm going to remove but for that I need to refactor some things that probably need to be added before this PR.

Anyway this work helps as is because we can run perf tests :).

r? @oli-obk /cc @nikomatsakis
Centril added a commit to Centril/rust that referenced this pull request Oct 25, 2019
… r=oli-obk

Intern place projection

This should sit on top of rust-lang#65197. After that one merged, I'm gonna rebase on top of it.

The important commits are the last three and there's a bunch of code repetition that I'm going to remove but for that I need to refactor some things that probably need to be added before this PR.

Anyway this work helps as is because we can run perf tests :).

r? @oli-obk /cc @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants