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

Remove dead code after destination propagation #105420

Merged
merged 1 commit into from
Dec 18, 2022
Merged

Remove dead code after destination propagation #105420

merged 1 commit into from
Dec 18, 2022

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Dec 7, 2022 •
edited
Loading

Fixes #105428.

cc @JakobDegen

@rustbot
Copy link
Collaborator

rustbot commented Dec 7, 2022

r? @fee1-dead

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 7, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 7, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@JakobDegen
Copy link
Contributor

Ah, yeah, that explains it. Good catch!

@oli-obk
Copy link
Contributor

oli-obk commented Dec 7, 2022

r? @oli-obk

I don't see a minimal repro on the issue. Do you know how to trigger it and can add a test?

@rustbot rustbot assigned oli-obk and unassigned fee1-dead Dec 7, 2022
@tmiasko
Copy link
Contributor Author

tmiasko commented Dec 7, 2022

Custom MIR doesn't support function calls yet, so I don't think I can add a test (reproducing the issue requires dead code of specific form to exist precisely at the time when destination propagation runs).

@cjgillot
Copy link
Contributor

cjgillot commented Dec 7, 2022

Doesn't DestProp preserve CFG?
In this case, we should probably remove dead blocks before DestProp, and assert there are none for DestProp to be sound?

@tmiasko
Copy link
Contributor Author

tmiasko commented Dec 7, 2022

When remove_dead_blocks changes the CFG it is also responsible for indicating that through appropriate API.

Do you prefer to remove dead blocks upfront? It is correct either way.

@JakobDegen
Copy link
Contributor

and assert there are none for DestProp to be sound?

Bit of a nitpick, but probably worth pointing out: This is not strictly speaking a soundness bug. Dest prop is introducing UB in dead code, which is "fine."

Custom MIR doesn't support function calls yet, so I don't think I can add a test (reproducing the issue requires dead code of specific form to exist precisely at the time when destination propagation runs).

I can add support for this this weekend. We can delay this PR until then, or I can just add a test in a follow up at that time.

@saethlin
Copy link
Member

saethlin commented Dec 15, 2022 •
edited
Loading

Dest prop is introducing UB in dead code, which is "fine."

Can/should we adjust MIR validation to ignore dead code? (if this PR is blocked on testing via custom MIR, such a PR would be too, but still...)

Or... should we have MIR validation make a copy of the MIR, run remove_dead_blocks on the copy, and validate that?

@JakobDegen
Copy link
Contributor

JakobDegen commented Dec 15, 2022 •
edited
Loading

Can/should we adjust MIR validation to ignore dead code?

Not in general. "Well-formed MIR" is something that ought to have a precise definition that does not depend on things like reachability, and (ignoring that that doesn't exist today) we should not intentionally diverge from that in the validator.

That being said, this particular check should probably be turned off in dead code because the MIR is technically well-formed. Will file an issue

@tmiasko tmiasko force-pushed the dest-prop-dead-code branch 2 times, most recently from 397df84 to adf53d4 Compare December 16, 2022 11:20
@tmiasko
Copy link
Contributor Author

tmiasko commented Dec 16, 2022

  • Added a regression test for the original issue.

@JakobDegen
Copy link
Contributor

LGTM

@tmiasko
Copy link
Contributor Author

tmiasko commented Dec 18, 2022

@bors r=JakobDegen rollup

@bors
Copy link
Contributor

bors commented Dec 18, 2022

📌 Commit adf53d4 has been approved by JakobDegen

It is now in the queue for this repository.

@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 Dec 18, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 18, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2022
@bors
Auto merge of rust-lang#105876 - matthiaskrgr:rollup-a9dgzjt, r=matth…
d0dc9ef 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#96584 (Fix `x setup -h -v` should work)
 - rust-lang#105420 (Remove dead code after destination propagation)
 - rust-lang#105844 (Make the x tool use the x and x.ps1 scripts)
 - rust-lang#105854 (remove redundant clone)
 - rust-lang#105858 (Another `as_chunks` example)
 - rust-lang#105870 (avoid .into() conversion to identical types)
 - rust-lang#105875 (don't destuct references just to reborrow)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f2f297a into rust-lang:master Dec 18, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 18, 2022
@tmiasko tmiasko deleted the dest-prop-dead-code branch December 18, 2022 22:03
@RalfJung RalfJung mentioned this pull request Jan 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@JakobDegen JakobDegen JakobDegen left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.

Broken MIR in encoding_rs related to MIR inlining + DestProp
9 participants
@tmiasko @rustbot @JakobDegen @oli-obk @cjgillot @saethlin @bors @RalfJung @fee1-dead