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

Iteratively simplify const conditions #107702

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Feb 5, 2023

cc @est31

It turns out that the drop flags are correctly inserted. However, the intermediate MIR transformation SimplifyConstCondition does not clean up the unreachable blocks. In addition, MIR validation understandably does not apply const analysis when tracking storage liveness, despite the fact that the "violating" block is apparently unreachable after applying a few rounds of data-flow const analysis.

We shall take the example from the description of #104843. After applying SimplifyConstCondition once, we have the following control flow graph.

SimplifyConstCondition

Note that Block 10 is only reachable if the drop flag _5 is set to true, except that _5 is always false.

There are open questions. Is it desirable to apply DataflowConstProp iteratively? Is it the right way to apply this transformation given that we need to perform the patching a few times?

Unfortunately, this will not provide answer to #103108.

@rustbot
Copy link
Collaborator

rustbot commented Feb 5, 2023

r? @cjgillot

(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 Feb 5, 2023
@cjgillot
Copy link
Contributor

cjgillot commented Feb 5, 2023

DataflowConstProp is an experimental optimization, which is much heavier than SimplifyConstCondition. Therefore, we do not want to enable it by default, like SimplifyConstCondition is.

In the provided CFG, bb1 is has no parent, is that so? Can SimplifyConstCondition remove it by itself? For instance by tracking the number of predecessors of each block, and making those which have 0 trivial?

I'd be interested in regular ConstProp being extended to become aware of cfg. Not to the extend of DataflowConstProp, but we could have a few interesting wins:

  • visit blocks in reverse post-order so SSA assignments are propagated everywhere;
  • skip visiting unreachable blocks;
  • fold SwitchInt and Assert terminators into Goto, and leaving less cruft behind.

Would those improvements be able to reproduce what you want to achieve here?

@dingxiangfei2009 dingxiangfei2009 changed the title iteratively simplify const conditions Iteratively simplify const conditions Feb 6, 2023
@cjgillot cjgillot 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 Mar 8, 2023
@dingxiangfei2009
Copy link
Contributor Author

@cjgillot Sorry, I am back on working on this. I was working on something else in the last two weeks.

@Dylan-DPC
Copy link
Member

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

@Dylan-DPC Dylan-DPC closed this Aug 3, 2023
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants