Skip to content

Limit storage duration of inlined always live locals - #79027

Merged
bors merged 1 commit into
rust-lang:masterfrom
tmiasko:inline-always-live-locals
Nov 17, 2020
Merged

Limit storage duration of inlined always live locals#79027
bors merged 1 commit into
rust-lang:masterfrom
tmiasko:inline-always-live-locals

Conversation

@tmiasko

@tmiaskotmiasko commented Nov 13, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Closes#76375.

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2020
@tmiaskotmiasko changed the title Limit storage duration of always live locals in inlined functionsLimit storage duration of inlined always live localsNov 13, 2020
@tmiasko
tmiaskoforce-pushed the inline-always-live-locals branch 3 times, most recently from 6569ec7 to 832d02dCompareNovember 13, 2020 19:42
oli-obk
oli-obk reviewed Nov 14, 2020
View reviewed changes
Comment threadcompiler/rustc_mir/src/transform/inline.rs Outdated
Comment threadcompiler/rustc_mir/src/transform/inline.rs Outdated
Comment on lines 73 to 76

@oli-obkoli-obkNov 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So the real fix would be to make sure we do not lose storage markers? Are you sure it's lost storage markers and not simply "never added" ones? I was sure we have lots of locals without storage markers.

Or maybe we should just go for full #68622 anyway...

@tmiaskotmiaskoNov 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So the real fix would be to make sure we do not lose storage markers?

This is what I had in mind originally, but we can also consider it to be a valid transformation even if it is suboptimal.

I was sure we have lots of locals without storage markers.

Locals marked as internal do not participate in validation performed by the state transform. Though, In that context I think it makes sense to always limit storage of those locals simply to help stack space optimizations.

maybe we should just go for full #68622 anyway...

It is unclear to me how the end of storage could be determined, especially for any local whose address escapes at some point.

@oli-obkoli-obkNov 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is unclear to me how the end of storage could be determined, especially for any local whose address escapes at some point.

Ah, we'd still have a "StorageDead" of sorts: InvalidateBorrows. It's not quite the same thing, but it will allow us to figure out that if we manually computed a StorageDead, it could not be before an InvalidateBorrows of the same local.

@tmiasko
tmiaskoforce-pushed the inline-always-live-locals branch from 832d02d to e7b6b61CompareNovember 14, 2020 15:03
@oli-obk

oli-obk commented Nov 14, 2020

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Nov 14, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit e7b6b61 has been approved by oli-obk

@borsbors 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 Nov 14, 2020
bjorn3
bjorn3 reviewed Nov 14, 2020
View reviewed changes
Comment threadcompiler/rustc_mir/src/transform/inline.rs
@jonas-schievinkjonas-schievink mentioned this pull request Nov 14, 2020
Closed
@tmiasko

tmiasko commented Nov 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Blocked on #78966 (tests need to be updated after it lands).

@tmiasko
tmiaskoforce-pushed the inline-always-live-locals branch from e7b6b61 to f27d56dCompareNovember 15, 2020 10:52
@tmiasko

tmiasko commented Nov 15, 2020

Copy link
Copy Markdown
ContributorAuthor

Updated tests. This is now unblocked.

@oli-obk

oli-obk commented Nov 15, 2020

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Nov 15, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit f27d56d has been approved by oli-obk

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 17, 2020
@Dylan-DPC
Rollup merge ofrust-lang#79027- tmiasko:inline-always-live-locals, …
77dc14a
…r=oli-obk
Limit storage duration of inlined always live locals
Closesrust-lang#76375.
@Dylan-DPC-zzDylan-DPC-zz mentioned this pull request Nov 17, 2020
Closed
@m-ou-sem-ou-se mentioned this pull request Nov 17, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 17, 2020
@bors
Auto merge ofrust-lang#79128- m-ou-se:rollup-lzz1dym, r=m-ou-se
efcb3b3
Rollup of 9 pull requests
Successful merges:
- rust-lang#77939 (Ensure that the source code display is working with DOS backline)
- rust-lang#78138 (Upgrade dlmalloc to version 0.2)
- rust-lang#78967 (Make codegen tests compatible with extra inlining)
- rust-lang#79027 (Limit storage duration of inlined always live locals)
- rust-lang#79077 (document that __rust_alloc is also magic to our LLVM fork)
- rust-lang#79088 (clarify `span_label` documentation)
- rust-lang#79097 (Code block invalid html tag lint)
- rust-lang#79105 (std: Fix test `symlink_hard_link` on Windows)
- rust-lang#79107 (build-manifest: strip newline from rustc version)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 336dc18 into rust-lang:masterNov 17, 2020
@rustbotrustbot added this to the 1.50.0 milestone Nov 17, 2020
@tmiasko
tmiasko deleted the inline-always-live-locals branch November 17, 2020 12:03
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obkoli-obkoli-obk left review comments
@bjorn3bjorn3bjorn3 left review comments

Assignees

@oli-obkoli-obk

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.50.0

Development

Successfully merging this pull request may close these issues.

Broken MIR: generator contains type ... after copy & destination propagation

6 participants

@tmiasko@oli-obk@bors@bjorn3@rust-highfive@rustbot