Skip to content

Analyse storage liveness and preserve it during generator transformation #44480

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
Merged
merged 4 commits into from
Sep 14, 2017
Merged

Analyse storage liveness and preserve it during generator transformation #44480

merged 4 commits into from
Sep 14, 2017

Conversation

@Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Sep 10, 2017

This uses a dataflow analysis on StorageLive and StorageDead statements to infer where the storage of locals are live. The result of this analysis is intersected with the regular liveness analysis such that a local is can only be live when its storage is. This fixes #44184. If the storage of a local is live across a suspension point, we'll insert a StorageLive statement for it after the suspension point so storage liveness is preserved. This fixes #44179.

r? @arielb1

arielb1
arielb1 reviewed Sep 11, 2017
View reviewed changes
src/librustc_mir/transform/generator.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

A function return automatically does a StorageDead of all locals, so there is no need to insert anything here.

arielb1
arielb1 reviewed Sep 11, 2017
View reviewed changes
src/librustc_mir/transform/generator.rs Outdated
Copy link
Contributor

@arielb1 arielb1 Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

nit: I would rename this to liveness

arielb1
arielb1 reviewed Sep 11, 2017
View reviewed changes
src/librustc_mir/transform/generator.rs Outdated
Copy link
Contributor

@arielb1 arielb1 Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

... and also have a let live_locals = storage_liveness; here. It's free to use better names.

arielb1
arielb1 reviewed Sep 11, 2017
View reviewed changes
src/librustc_mir/transform/generator.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: double space the poisoned state

src/librustc_mir/transform/generator.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: double space the poisoned state

@arielb1
Copy link
Contributor

arielb1 commented Sep 11, 2017

Nice! r=me modulo nits.

arielb1
arielb1 reviewed Sep 11, 2017
View reviewed changes
src/librustc_mir/transform/generator.rs Outdated
Copy link
Contributor

@arielb1 arielb1 Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I find this rather ugly. Can you have a

fn make_storage_live(..., point: &SuspensionPoint, terminates_to: BasicBlock) {
}

And call it from both collects to avoid the ugly mutation? I think you can.

arielb1
arielb1 reviewed Sep 11, 2017
View reviewed changes
src/librustc_mir/transform/generator.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

(this is the part that could be extracted to a function)

@carols10cents carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 11, 2017
@arielb1
Copy link
Contributor

arielb1 commented Sep 11, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 11, 2017

📌 Commit 23d3d9f has been approved by arielb1

@arielb1
Copy link
Contributor

arielb1 commented Sep 11, 2017

I think it's possible to merge the 3 special case states into SuspensionPoint and clean up quite a bit of code, but I can leave that to a further PR.

@bors
Copy link
Collaborator

bors commented Sep 12, 2017

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

@arielb1
Copy link
Contributor

arielb1 commented Sep 12, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 12, 2017

📌 Commit b6bbd4f has been approved by arielb1

@bors
Copy link
Collaborator

bors commented Sep 13, 2017

⌛ Testing commit b6bbd4f0929d8308c340fe0c71dbf1dfbc9ff226 with merge 81fa6e7db64256e6d7049627669ff64e99176959...

@bors
Copy link
Collaborator

bors commented Sep 13, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Sep 13, 2017

i686-gnu-nopt failed, an error index failed, legit.

I think you need to build the error index yourself to find out what is happening at line 11731 😓

[01:34:13] failures:
[01:34:13] 
[01:34:13] ---- /checkout/obj/build/i686-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index (line 11731) stdout ----
[01:34:13] 	thread 'rustc' panicked at 'assertion failed: `(left == right)`
[01:34:13]   left: `2`,
[01:34:13]  right: `1`', /checkout/src/librustc_data_structures/bitslice.rs:119:4
[01:34:13] thread 'rustc' panicked at 'couldn't compile the test', /checkout/src/librustdoc/test.rs:280:12
[01:34:13] 
[01:34:13] 
[01:34:13] failures:
[01:34:13]     /checkout/obj/build/i686-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index (line 11731)
[01:34:13] 
[01:34:13] test result: FAILED. 660 passed; 1 failed; 18 ignored; 0 measured; 0 filtered out

@arielb1
Copy link
Contributor

arielb1 commented Sep 14, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 14, 2017

📌 Commit 0e8e659 has been approved by arielb1

@bors
Copy link
Collaborator

bors commented Sep 14, 2017

⌛ Testing commit 0e8e659 with merge 5dfc84c...

bors added a commit that referenced this pull request Sep 14, 2017
@bors
Auto merge of #44480 - Zoxc:gen-liveness, r=arielb1
5dfc84c 
Analyse storage liveness and preserve it during generator transformation

This uses a dataflow analysis on `StorageLive` and `StorageDead` statements to infer where the storage of locals are live. The result of this analysis is intersected with the regular liveness analysis such that a local is can only be live when its storage is. This fixes #44184. If the storage of a local is live across a suspension point, we'll insert a `StorageLive` statement for it after the suspension point so storage liveness is preserved. This fixes #44179.

r? @arielb1
@bors
Copy link
Collaborator

bors commented Sep 14, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 5dfc84c to master...

@bors bors merged commit 0e8e659 into rust-lang:master Sep 14, 2017
@kennytm kennytm mentioned this pull request Sep 26, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@arielb1 arielb1 arielb1 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@arielb1 arielb1

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Milestone

No milestone

5 participants

@Zoxc @arielb1 @bors @kennytm @carols10cents