Skip to content

Conversation

gurry
Copy link
Contributor

@gurry gurry commented Apr 30, 2024

Removes the below check which prevents unions from being const propagated:

// Do not const prop union fields as they can be
// made to produce values that don't match their
// underlying layout's type (see ICE #121534).
// If the last element of the `Adt` tuple
// is `Some` it indicates the ADT is a union
if let AggregateKind::Adt(_, _, _, _, Some(_)) = **kind {
return None;
};

It is not needed because after PR #124504 we mark unions as NoPropagation over here:

if ty.is_union() {
// Do not const prop unions as they can
// ICE during layout calc
*val = ConstPropMode::NoPropagation;
which is enough to prevent them from being const propagated.

@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@gurry gurry changed the title Remove redundant union check in `KnownPanicsLint const prop Remove redundant union check in KnownPanicsLint const prop Apr 30, 2024
@gurry gurry force-pushed the remove-redundant-code branch from 78f3f5c to e0c73be Compare April 30, 2024 09:45
because we are already marking unions `NoPropagation` in
`CanConstProp::check()`. That is enough to prevent any attempts
at const propagating unions and this second check is not needed.

Also improve a comment in `CanConstProp::check()`
@gurry gurry force-pushed the remove-redundant-code branch from e0c73be to 741d40f Compare April 30, 2024 09:48
@oli-obk
Copy link
Contributor

oli-obk commented Apr 30, 2024

Thanks!

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 30, 2024

📌 Commit 741d40f has been approved by oli-obk

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 Apr 30, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2024
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#123247 (Mention Both HRTB and Generic Lifetime Param in `E0637` documentation)
 - rust-lang#124511 (Remove many `#[macro_use] extern crate foo` items)
 - rust-lang#124550 (Remove redundant union check in `KnownPanicsLint` const prop)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5a4e83c into rust-lang:master Apr 30, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2024
Rollup merge of rust-lang#124550 - gurry:remove-redundant-code, r=oli-obk

Remove redundant union check in `KnownPanicsLint` const prop

Removes the below check which prevents unions from being const propagated:https://github.com/rust-lang/rust/blob/f9dca46218d4b8efa062aec4fd0820cbb4942aa2/compiler/rustc_mir_transform/src/known_panics_lint.rs#L587-L594

It is not needed because after PR rust-lang#124504 we mark unions as `NoPropagation` over here: https://github.com/rust-lang/rust/blob/f9dca46218d4b8efa062aec4fd0820cbb4942aa2/compiler/rustc_mir_transform/src/known_panics_lint.rs#L899-L902 which is enough to prevent them from being const propagated.
@rustbot rustbot added this to the 1.80.0 milestone Apr 30, 2024
@gurry gurry deleted the remove-redundant-code branch May 1, 2024 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants