ConstProp: Correctly remove const if unknown value assigned to it. by aDotInTheVoid · Pull Request #118426 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Closes #118328

The problematic sequence of MIR is:

      _1 = const 0_usize;
      _1 = const _; // This is an associated constant we can't know before monomorphization.
      _0 = _1;

When ConstProp::visit_assign happens on _1 = const 0_usize;, it records that 0x0usize is the value for _1.
Next visit_assign happens on _1 = const _;. Because the rvalue .has_param(), it can't be const evaled.
Finaly, visit_assign happens on _0 = _1;. Here it would think the value of _1 was 0x0usize from step 1.

The solution is to remove consts when checking the RValue fails, as they may have contained values that should now be invalidated, as that local was overwritten.

This should probably be back-ported to beta. Stable is more iffy, as it's gone unidentified since 1.70, so I only think it's worthwhile if there's another reason for a 1.74.1 release anyway.

r? @wesleywiser

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

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

Nov 28, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

}

fn main() {
assert_eq!(size_of::(), std::mem::size_of::());

Choose a reason for hiding this comment

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

I don't think this is actually running, and in general I'm not sure what the right way to write a robust test for this is.

Choose a reason for hiding this comment

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

By "running", you mean testing that the assert doesn't fire? If so, then you may want to copy this test into tests/ui/$SOMEWHERE and mark it as a // run-pass test.

(please also give this test file a better name, preferably one that actually describes what it does, here and possibly in tests/ui if you copy it there).

Please rebase out the second and fourth commits in this stack, since the latter undoes the former, and it's not particularly useful for git history that way. Either that, or just drop the last commit, since it's possibly useful to keep the instrumentation around, as long as it's debug or trace level.

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

r=me when ci is green, since this seems like a pretty nasty regression

cc @cjgillot who may be curious about this const prop misoptimization, perhaps you have some ideas to make this code less fragile.

Choose a reason for hiding this comment

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

R=me with test fortified.

@@ -0,0 +1,23 @@
// unit-test: ConstProp
// compile-flags: -O
// skip-filecheck

Choose a reason for hiding this comment

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

Could you add the filecheck annotations? See other files for example.

Choose a reason for hiding this comment

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

done

perhaps you have some ideas to make this code less fragile.

Yes. Removing this pass entirely, replaced by gvn.

@bors r=compiler-errors,cjgillot

📌 Commit 6e956c0 has been approved by compiler-errors,cjgillot

It is now in the queue for this repository.

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

Nov 29, 2023

bors added a commit to rust-lang-ci/rust that referenced this pull request

Nov 29, 2023

…iaskrgr

Rollup of 7 pull requests

Successful merges:

rust-lang#118157 (Add never_patterns feature gate)
rust-lang#118191 (Suggest let or == on typo'd let-chain)
rust-lang#118231 (also add is_empty to const raw slices)
rust-lang#118333 (Print list of missing target features when calling a function with target features outside an unsafe block)
rust-lang#118426 (ConstProp: Correctly remove const if unknown value assigned to it.)
rust-lang#118428 (rustdoc: Move AssocItemRender and RenderMode to html::render.)
rust-lang#118438 (Update nto-qnx.md)

Failed merges:

rust-lang#118268 (Pretty print Fn<(..., ...)> trait refs with parentheses (almost) always)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Nov 29, 2023

Rollup merge of rust-lang#118426 - aDotInTheVoid:const-wat, r=compiler-errors,cjgillot

ConstProp: Correctly remove const if unknown value assigned to it.

Closes rust-lang#118328

The problematic sequence of MIR is:

          _1 = const 0_usize;
          _1 = const _; // This is an associated constant we can't know before monomorphization.
          _0 = _1;

When ConstProp::visit_assign happens on _1 = const 0_usize;, it records that 0x0usize is the value for _1.
Next visit_assign happens on _1 = const _;. Because the rvalue .has_param(), it can't be const evaled.
Finaly, visit_assign happens on _0 = _1;. Here it would think the value of _1 was 0x0usize from step 1.

The solution is to remove consts when checking the RValue fails, as they may have contained values that should now be invalidated, as that local was overwritten.

This should probably be back-ported to beta. Stable is more iffy, as it's gone unidentified since 1.70, so I only think it's worthwhile if there's another reason for a 1.74.1 release anyway.

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 1, 2023

Labels

beta-accepted

Accepted for backporting to the compiler in the beta channel.

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.