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

Reimplement ConstProp using DataflowConstProp #110719

Closed
wants to merge 4 commits into from

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Apr 23, 2023

This PR attempts to remove the current ConstProp optimization, and replace it by an implementation based on DataflowConstProp.

The current ConstProp twists the MIR interpreter to do things it is not designed to do, including manipulating generic types. Meanwhile, @jachris has implemented a new DataflowConstProp pass that only uses a few chosen methods for arithmetic.

The main change concerns how propagatable places and operations are tracked. In the current ConstProp, all places are tracked and all operations executed, and we removed wrong or non-propagatable results. This new implementation does the opposite : place tracking is opt-in, and we only propagate supported operations. I believe this approach to be less risky from a soundness point of view.

As a consequence, this new implementation is more restricted. The main limitation is that we stop propagating inside a basic block, and only propagate SSA locals. This makes the implementation simpler, and is more consistent with this "opt-in" logic.

Some operations are added inside this PR (transmutes, algebraic simplifications, Len operations). The handling of statics and non-scalar consts are left to a follow-up PR.

This PR does not remove the const-prop lints (arithmetic overflow and unconditional panic), as I need to design a way to pass more span information around.

There has been quite a few perf runs below. Marking as ready for review, since I removed the massive regressions on diesel/keccak/serde. Kept as waiting-on-author until I bless CI.

Regressed optimizations:

  • array indexing;
  • non-scalar constants;
  • assignment of immutable statics;
  • creating rvalues with non-scalar types;
  • integer-to-pointer transmutes.

Based on #110728, #110732, #110820

@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 Apr 23, 2023
@cjgillot

This comment was marked as outdated.

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 23, 2023
@bors

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@cjgillot

This comment was marked as outdated.

@rust-timer

This comment has been minimized.

@bors

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@rust-timer

This comment has been minimized.

@rust-timer

This comment was marked as outdated.

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 23, 2023
@cjgillot

This comment was marked as outdated.

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 23, 2023
@bors

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@bors bors 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 Apr 23, 2023
@bors
Copy link
Contributor

bors commented Jul 20, 2023

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

@bors
Copy link
Contributor

bors commented Aug 30, 2023

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2023
Improvements to dataflow const-prop

Partially cherry-picked from rust-lang#110719

r? `@oli-obk`
cc `@jachris`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2023
Improvements to dataflow const-prop

Partially cherry-picked from rust-lang#110719

r? `@oli-obk`
cc `@jachris`
@rust-log-analyzer

This comment has been minimized.

@@ -1,11 +1,10 @@
Function name: <issue_84561::Foo as core::cmp::PartialEq>::eq
Raw bytes (14): 0x[01, 01, 00, 02, 01, 04, 0a, 00, 13, 00, 00, 0a, 00, 13]
Raw bytes (9): 0x[01, 01, 00, 01, 01, 04, 0a, 00, 13]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Zalathar This commit blesses the coverage map. However, I have no idea how to check that it is still correct. What do you recommend?

Copy link
Contributor

Choose a reason for hiding this comment

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

As a rule of thumb, any PR that doesn't touch coverage-specific code should feel free to just re-bless these.

(They're unfortunately quite sensitive to changes in MIR lowering/optimization, in a way that tends to result in different but equivalent mappings.)

Copy link
Contributor

Choose a reason for hiding this comment

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

In particular, as long as the run-coverage tests still pass, then any changes to the mappings are probably just noise.

(Those tests only run if the profiler runtime is enabled in config.toml, so they don't run by default in dev builds or on PRs, but they do run in the full CI before merging.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the actual diff, it seems like some MIR statements that were previously being optimized out are no longer being optimized out. Nothing to worry about from a coverage perspective.

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 12, 2023
Improvements to dataflow const-prop

Partially cherry-picked from rust-lang/rust#110719

r? `@oli-obk`
cc `@jachris`
@bors
Copy link
Contributor

bors commented Sep 12, 2023

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

@cjgillot
Copy link
Contributor Author

I'd like to close this PR and pursue another direction for ConstProp.

#109597 creates a general framework for value analyses. I believe that implementing constant-folding in this pass can achieve the same level as what this PR proposes, while being more flexible:

  • we'd only use the interpreter arithmetic API, like dataflow const-prop does, and allow removing ConstPropNonsense;
  • that pass would only propagate whole values, simplifying state storage, and leaving the per-place to the dataflow version;
  • that pass would leverage arithmetic identities over unknown values, which const-prop cannot do;
  • that pass may be able to avoid duplicating constant allocations.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 14, 2023

That makes sense

@RalfJung
Copy link
Member

that pass would leverage arithmetic identities over unknown values, which const-prop cannot do;

const-prop can simplify some things here, right, like x*0 -> 0 or x+0 -> x?

@cjgillot
Copy link
Contributor Author

Yes, but it cannot do x - x = 0, which #116012 could (in #111344).

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2023
Implement constant propagation on top of MIR SSA analysis

This implements the idea I proposed in rust-lang#110719 (comment)

Based on rust-lang#109597

The value numbering "GVN" pass formulates each rvalue that appears in MIR with an abstract form (the `Value` enum), and assigns an integer `VnIndex` to each. This abstract form can be used to deduplicate values, reusing an earlier local that holds the same value instead of recomputing. This part is proposed in rust-lang#109597.

From this abstract representation, we can perform more involved simplifications, for example in rust-lang#111344.

With the abstract representation `Value`, we can also attempt to evaluate each to a constant using the interpreter. This builds a `VnIndex -> OpTy` map. From this map, we can opportunistically replace an operand or a rvalue with a constant if their value has an associated `OpTy`.

The most relevant commit is [Evaluated computed values to constants.](rust-lang@2767c49)"

r? `@oli-obk`
@oli-obk oli-obk closed this Sep 27, 2023
@cjgillot cjgillot deleted the single-const-prop branch September 28, 2023 11:17
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 22, 2023
Implement constant propagation on top of MIR SSA analysis

This implements the idea I proposed in rust-lang#110719 (comment)

Based on rust-lang#109597

The value numbering "GVN" pass formulates each rvalue that appears in MIR with an abstract form (the `Value` enum), and assigns an integer `VnIndex` to each. This abstract form can be used to deduplicate values, reusing an earlier local that holds the same value instead of recomputing. This part is proposed in rust-lang#109597.

From this abstract representation, we can perform more involved simplifications, for example in rust-lang#111344.

With the abstract representation `Value`, we can also attempt to evaluate each to a constant using the interpreter. This builds a `VnIndex -> OpTy` map. From this map, we can opportunistically replace an operand or a rvalue with a constant if their value has an associated `OpTy`.

The most relevant commit is [Evaluated computed values to constants.](rust-lang@2767c49)"

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2023
Implement constant propagation on top of MIR SSA analysis

This implements the idea I proposed in rust-lang#110719 (comment)

Based on rust-lang#109597

The value numbering "GVN" pass formulates each rvalue that appears in MIR with an abstract form (the `Value` enum), and assigns an integer `VnIndex` to each. This abstract form can be used to deduplicate values, reusing an earlier local that holds the same value instead of recomputing. This part is proposed in rust-lang#109597.

From this abstract representation, we can perform more involved simplifications, for example in rust-lang#111344.

With the abstract representation `Value`, we can also attempt to evaluate each to a constant using the interpreter. This builds a `VnIndex -> OpTy` map. From this map, we can opportunistically replace an operand or a rvalue with a constant if their value has an associated `OpTy`.

The most relevant commit is [Evaluated computed values to constants.](rust-lang@2767c49)"

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2023
Implement constant propagation on top of MIR SSA analysis

This implements the idea I proposed in rust-lang#110719 (comment)

Based on rust-lang#109597

The value numbering "GVN" pass formulates each rvalue that appears in MIR with an abstract form (the `Value` enum), and assigns an integer `VnIndex` to each. This abstract form can be used to deduplicate values, reusing an earlier local that holds the same value instead of recomputing. This part is proposed in rust-lang#109597.

From this abstract representation, we can perform more involved simplifications, for example in rust-lang#111344.

With the abstract representation `Value`, we can also attempt to evaluate each to a constant using the interpreter. This builds a `VnIndex -> OpTy` map. From this map, we can opportunistically replace an operand or a rvalue with a constant if their value has an associated `OpTy`.

The most relevant commit is [Evaluated computed values to constants.](rust-lang@2767c49)"

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 29, 2023
Implement constant propagation on top of MIR SSA analysis

This implements the idea I proposed in rust-lang#110719 (comment)

Based on rust-lang#109597

The value numbering "GVN" pass formulates each rvalue that appears in MIR with an abstract form (the `Value` enum), and assigns an integer `VnIndex` to each. This abstract form can be used to deduplicate values, reusing an earlier local that holds the same value instead of recomputing. This part is proposed in rust-lang#109597.

From this abstract representation, we can perform more involved simplifications, for example in rust-lang#111344.

With the abstract representation `Value`, we can also attempt to evaluate each to a constant using the interpreter. This builds a `VnIndex -> OpTy` map. From this map, we can opportunistically replace an operand or a rvalue with a constant if their value has an associated `OpTy`.

The most relevant commit is [Evaluated computed values to constants.](rust-lang@2767c49)"

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 30, 2023
Implement constant propagation on top of MIR SSA analysis

This implements the idea I proposed in rust-lang#110719 (comment)

Based on rust-lang#109597

The value numbering "GVN" pass formulates each rvalue that appears in MIR with an abstract form (the `Value` enum), and assigns an integer `VnIndex` to each. This abstract form can be used to deduplicate values, reusing an earlier local that holds the same value instead of recomputing. This part is proposed in rust-lang#109597.

From this abstract representation, we can perform more involved simplifications, for example in rust-lang#111344.

With the abstract representation `Value`, we can also attempt to evaluate each to a constant using the interpreter. This builds a `VnIndex -> OpTy` map. From this map, we can opportunistically replace an operand or a rvalue with a constant if their value has an associated `OpTy`.

The most relevant commit is [Evaluated computed values to constants.](rust-lang@2767c49)"

r? `@oli-obk`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 31, 2023
Implement constant propagation on top of MIR SSA analysis

This implements the idea I proposed in rust-lang/rust#110719 (comment)

Based on rust-lang/rust#109597

The value numbering "GVN" pass formulates each rvalue that appears in MIR with an abstract form (the `Value` enum), and assigns an integer `VnIndex` to each. This abstract form can be used to deduplicate values, reusing an earlier local that holds the same value instead of recomputing. This part is proposed in #109597.

From this abstract representation, we can perform more involved simplifications, for example in rust-lang/rust#111344.

With the abstract representation `Value`, we can also attempt to evaluate each to a constant using the interpreter. This builds a `VnIndex -> OpTy` map. From this map, we can opportunistically replace an operand or a rvalue with a constant if their value has an associated `OpTy`.

The most relevant commit is [Evaluated computed values to constants.](rust-lang/rust@2767c49)"

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.