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

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

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

aDotInTheVoid
Copy link
Member

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;
  1. When ConstProp::visit_assign happens on _1 = const 0_usize;, it records that 0x0usize is the value for _1.
  2. Next visit_assign happens on _1 = const _;. Because the rvalue .has_param(), it can't be const evaled.
  3. 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.

@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2023

r? @wesleywiser

(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 Nov 28, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

}

fn main() {
assert_eq!(size_of::<u32>(), std::mem::size_of::<u32>());
Copy link
Member Author

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.

Copy link
Member

@compiler-errors compiler-errors Nov 28, 2023

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).

@compiler-errors
Copy link
Member

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.

@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@compiler-errors
Copy link
Member

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.

Copy link
Contributor

@cjgillot cjgillot 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 with test fortified.

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@cjgillot
Copy link
Contributor

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

Yes. Removing this pass entirely, replaced by gvn.

@aDotInTheVoid aDotInTheVoid added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 28, 2023
@WaffleLapkin
Copy link
Member

@bors r=compiler-errors,cjgillot

@bors
Copy link
Contributor

bors commented Nov 29, 2023

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

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 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
@bors bors merged commit 434232f into rust-lang:master Nov 29, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Nov 29, 2023
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:

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

1. When `ConstProp::visit_assign` happens on `_1 = const 0_usize;`, it records that `0x0usize` is the value for `_1`.
2. Next `visit_assign` happens on `_1 = const _;`. Because the rvalue `.has_param()`, it can't be const evaled.
3. 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.
@aDotInTheVoid aDotInTheVoid deleted the const-wat branch November 29, 2023 13:40
@apiraino
Copy link
Contributor

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 30, 2023
@cuviper cuviper mentioned this pull request Dec 1, 2023
@cuviper cuviper modified the milestones: 1.76.0, 1.75.0 Dec 1, 2023
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 1, 2023
[beta] backports

- Build pre-coroutine-transform coroutine body rust-lang#117686
- Fix coroutine validation for mixed panic strategy rust-lang#118422
- ConstProp: Remove const when rvalue check fails. rust-lang#118426
- Dispose llvm::TargetMachines prior to llvm::Context being disposed rust-lang#118464

r? ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConstProp misoptimises pointer-typed enum field
9 participants