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

Use PlaceBuilder to avoid a lot of slice -> vec -> slice convertions #64922

Merged
merged 1 commit into from Oct 2, 2019

Conversation

@spastorino
Copy link
Member

commented Sep 30, 2019

r? @oli-obk

@spastorino spastorino force-pushed the spastorino:make-place-builder branch from 40a5331 to 897aa23 Sep 30, 2019
@spastorino

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2019

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

commented Sep 30, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

⌛️ Trying commit 897aa23 with merge 312f214...

bors added a commit that referenced this pull request Sep 30, 2019
Use PlaceBuilder to avoid a lot of slice -> vec -> slice convertions

r? @oli-obk

As next step would be nice to remove all these functions https://github.com/rust-lang/rust/blob/40a5331753e44ef8ef2d0b68fe6ee35234ab82ad/src/librustc/mir/mod.rs#L1862-L1894 but I don't want to mix things. I think it's better if we can run perf on this PR by itself and after is merged I can try to get rid of all those functions.
src/librustc_mir/build/expr/as_place.rs Outdated Show resolved Hide resolved
src/librustc_mir/build/expr/as_place.rs Outdated Show resolved Hide resolved
src/librustc_mir/build/expr/as_place.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

☀️ Try build successful - checks-azure
Build commit: 312f214 (312f214bc826aabf7836d8893cdc87931dac5e2d)

@rust-timer

This comment has been minimized.

Copy link

commented Sep 30, 2019

Queued 312f214 with parent e0436d9, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Oct 1, 2019

Finished benchmarking try commit 312f214, comparison URL.

@spastorino spastorino force-pushed the spastorino:make-place-builder branch from 897aa23 to ff717bb Oct 1, 2019
@spastorino

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

@bors try @rust-timer queue

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

⌛️ Trying commit ff717bb with merge 909a0a1...

bors added a commit that referenced this pull request Oct 1, 2019
Use PlaceBuilder to avoid a lot of slice -> vec -> slice convertions

r? @oli-obk
@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@rust-timer abort

@oli-obk oli-obk closed this Oct 1, 2019
@oli-obk oli-obk reopened this Oct 1, 2019
@mati865

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@oli-obk perf.rlo has crashed

@Centril

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

@bors retry

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

I don't want to try or perf run, I wanted to abort it 😆

@oli-obk oli-obk closed this Oct 1, 2019
@oli-obk oli-obk reopened this Oct 1, 2019
@Centril

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

@oli-obk My command was unrelated ;) I wanted to make my rollup go test (bors queue was stuck).

@spastorino spastorino force-pushed the spastorino:make-place-builder branch from ff717bb to a3defbd Oct 1, 2019
@spastorino spastorino force-pushed the spastorino:make-place-builder branch from a3defbd to 7990f45 Oct 1, 2019
Copy link
Contributor

left a comment

Looks good to me! I left a nit (request for a comment). I also mentioned pulling this into its own file, but after reading the whole PR I'm not sure if it makes sense or not.

src/librustc_mir/build/expr/as_place.rs Show resolved Hide resolved
src/librustc_mir/build/expr/as_place.rs Show resolved Hide resolved
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

r=me with comments -- unless @oli-obk would prefer a chance to review, of course

@spastorino spastorino force-pushed the spastorino:make-place-builder branch from 7990f45 to 79dc862 Oct 2, 2019
@spastorino

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

📌 Commit 79dc862 has been approved by nikomatsakis

@spastorino spastorino requested a review from oli-obk Oct 2, 2019
@oli-obk
oli-obk approved these changes Oct 2, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 2, 2019
…komatsakis

Use PlaceBuilder to avoid a lot of slice -> vec -> slice convertions

r? @oli-obk
bors added a commit that referenced this pull request Oct 2, 2019
Rollup of 13 pull requests

Successful merges:

 - #64581 (Fix unreachable_code warnings for try{} block ok-wrapped expressions)
 - #64850 (Remove inlines from DepNode code)
 - #64914 (regression test for 64453 borrow check error.)
 - #64922 (Use PlaceBuilder to avoid a lot of slice -> vec -> slice convertions)
 - #64948 (Improve sidebar styling to make its integration easier)
 - #64961 (Make comment about dummy type a bit more clear)
 - #64967 (Don't mark borrows of zero-sized arrays as indirectly mutable)
 - #64973 (Fix typo while setting `compile-flags` in test)
 - #64980 (Enable support for `IndirectlyMutableLocals` in `rustc_peek` )
 - #64989 (Fix ICE #64964)
 - #64991 ([const-prop] Correctly handle locals that can't be propagated)
 - #64995 (Remove rustdoc warning)
 - #64997 (rustc book: nitpick SLP vectorization)

Failed merges:

r? @ghost
@bors bors merged commit 79dc862 into rust-lang:master Oct 2, 2019
4 checks passed
4 checks passed
pr Build #20191002.42 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.