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.

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

[mir-opt] Allow debuginfo to be generated for a constant or a Place #73210

Merged
merged 2 commits into from
Dec 15, 2020
Merged

[mir-opt] Allow debuginfo to be generated for a constant or a Place #73210

merged 2 commits into from
Dec 15, 2020

Conversation

wesleywiser
Copy link
Member

Prior to this commit, debuginfo was always generated by mapping a name
to a Place. This has the side-effect that SimplifyLocals cannot remove
locals that are only used for debuginfo because their other uses have
been const-propagated.

To allow these locals to be removed, we now allow debuginfo to point to
a constant value. The ConstProp pass detects when debuginfo points to
a local with a known constant value and replaces it with the value. This
allows the later SimplifyLocals pass to remove the local.

@wesleywiser wesleywiser added the A-mir-opt Area: MIR optimizations label Jun 10, 2020
@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 Jun 10, 2020
@wesleywiser
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 10, 2020

⌛ Trying commit e36759f0f6501606e036c97a84f80294e2f6bd3a with merge 25e92ddd8e4c25f9882f1f4d1822f1b860b3788a...

@rust-highfive

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jun 10, 2020

☀️ Try build successful - checks-azure
Build commit: 25e92ddd8e4c25f9882f1f4d1822f1b860b3788a (25e92ddd8e4c25f9882f1f4d1822f1b860b3788a)

@rust-timer
Copy link
Collaborator

Queued 25e92ddd8e4c25f9882f1f4d1822f1b860b3788a with parent bb86748, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (25e92ddd8e4c25f9882f1f4d1822f1b860b3788a): comparison url.

@bors

This comment has been minimized.

Sign in to view
@JohnCSimon JohnCSimon 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2020
@JohnCSimon
Copy link
Member

triage: assigning back to author to address merge conflicts.

@wesleywiser
Copy link
Member Author

Rebased

@rust-highfive

This comment has been minimized.

Sign in to view
@JohnTitor JohnTitor mentioned this pull request Jul 1, 2020
Closed
@wesleywiser
Copy link
Member Author

Rebased and CI is passing.

eddyb
eddyb approved these changes Jul 6, 2020
View reviewed changes
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

r=me but maybe @oli-obk should take a look at the const-prop changes, I'm not sure what "only within the same block" propagation does.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 25, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 26, 2020

The debuginfo tests need updating now, too

@bors
Copy link
Contributor

bors commented Oct 26, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@oli-obk oli-obk 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 3, 2020
@crlf0710 crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 20, 2020
@Dylan-DPC-zz
Copy link

@wesleywiser any updates?

@wesleywiser
Copy link
Member Author

I still can't repro this locally but I'm starting to wonder if this has something to do with the version of gdb being tested. My local version of gdb is quite a bit newer than the one being tested on the failing platform (7.10). I'm trying to see if I can get that version from my distro to test with.

@wesleywiser
[mir-opt] Allow debuginfo to be generated for a constant or a Place
01aec8d 
Prior to this commit, debuginfo was always generated by mapping a name
to a Place. This has the side-effect that `SimplifyLocals` cannot remove
locals that are only used for debuginfo because their other uses have
been const-propagated.

To allow these locals to be removed, we now allow debuginfo to point to
a constant value. The `ConstProp` pass detects when debuginfo points to
a local with a known constant value and replaces it with the value. This
allows the later `SimplifyLocals` pass to remove the local.
@wesleywiser
Copy link
Member Author

It doesn't seem the issue is merely the version of gdb as a similarly old version works fine on Linux. I will still try to get a repro on Windows but in the meantime I'd like to go ahead and merge this with the pass disabled by default. I've had to rebase this a few times now and the PR is just large enough to be a bit unpleasant when rebasing. Once the windows-gnu issue is resolved, I'll open another PR to enable this by default.

@oli-obk does that seem fine to you?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2020

yes that seems perfectly reasonable to me

@wesleywiser
Disable the constant debuginfo promotion pass by default
0b18ed8 
It doesn't work correctly on *-pc-windows-gnu
@wesleywiser
Copy link
Member Author

Tests are blessed.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 15, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Dec 15, 2020

📌 Commit 0b18ed8 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 15, 2020
@bors
Copy link
Contributor

bors commented Dec 15, 2020

⌛ Testing commit 0b18ed8 with merge e99a89c...

@bors
Copy link
Contributor

bors commented Dec 15, 2020

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing e99a89c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 15, 2020
@bors bors merged commit e99a89c into rust-lang:master Dec 15, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 15, 2020
@bors bors mentioned this pull request Dec 15, 2020
Merged
@wesleywiser wesleywiser mentioned this pull request Oct 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@eddyb eddyb eddyb approved these changes

Assignees

@oli-obk oli-obk

Labels
A-mir-opt Area: MIR optimizations merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: 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.

None yet