Pass correct place to discriminant_switch_effect
#69676
Conversation
PR rust-lang#69562, which fixed a bug that was causing clippy to ICE, passed the place for the *result* of `Rvalue::Discriminant` instead of the *operand* to `apply_discriminant_switch_effect`. As a result, no effect was applied at all, and we lost the perf benefits from marking inactive enum variants as uninitialized.
|
🤦♀️ that's subtle. I want to overhaul mir opt tests anyway. Then it may be easier to write such tests @bors r+ |
|
📌 Commit 8d325e6 has been approved by |
bors
added
the
S-waiting-on-bors
…t, r=oli-obk Pass correct place to `discriminant_switch_effect` PR rust-lang#69562, which fixed a bug that was causing clippy to ICE, mistakenly passed the place holding the *result* of `Rvalue::Discriminant` instead of the place holding its *operand* to `apply_discriminant_switch_effect` as the enum place. As a result, no effect was applied at all, and we lost the perf benefits from marking inactive enum variants as uninitialized. This PR corrects that mistake and adds a regression test to `mir-opt`. I fear that the regression test may prove too brittle; the test schema makes hard to test for the *absence* of certain kinds of MIR without exhaustively matching each basic block. r? @oli-obk
|
Failed in #69720 (comment) |
bors
added
S-waiting-on-author
|
The Linux Any advice @oli-obk? I want to get this merged before beta branches, so maybe we could split the test into a second PR? |
|
Yea, split the test into a separate PR. r=me on both this PR and the test PR |
ecstatic-morse
force-pushed
the
fix-enum-discr-effect
branch
from
8e61dfe
to
e82ec23
Compare
|
📌 Commit e82ec23 has been approved by |
bors
added
S-waiting-on-bors
…t, r=oli-obk Pass correct place to `discriminant_switch_effect` PR rust-lang#69562, which fixed a bug that was causing clippy to ICE, mistakenly passed the place holding the *result* of `Rvalue::Discriminant` instead of the place holding its *operand* to `apply_discriminant_switch_effect` as the enum place. As a result, no effect was applied at all, and we lost the perf benefits from marking inactive enum variants as uninitialized. **edit:** The regression test has been split into rust-lang#69744. r? @oli-obk
Rollup of 6 pull requests Successful merges: - #69656 (Use .next() instead of .nth(0) on iterators.) - #69674 (Rename DefKind::Method and TraitItemKind::Method ) - #69676 (Pass correct place to `discriminant_switch_effect`) - #69706 (Use subslice patterns in slice methods) - #69714 (Make PlaceRef take just one lifetime) - #69727 (Avoid using `unwrap()` in suggestions) Failed merges: - #69589 (ast: `Mac`/`Macro` -> `MacCall`) - #69680 (rustc_expand: Factor out `Annotatable::into_tokens` to a separate method) r? @ghost
|
@bors p=1 |
|
⌛ Testing commit e82ec23 with merge 9e12600c78511f2a6266acf2b68047a5e36ef395... |
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
@bors treeclosed=1000 retry |
bors
added
S-waiting-on-bors
|
@bors p=1001 |
|
☀️ Test successful - checks-azure |
|
@bors treeclosed- |
…li-obk Add `mir-opt` test for more precise drop elaboration Depends on #69676. This test should ensure that the problem fixed in that PR does not reoccur. This has been split out from #69676 since the test fails on certain targets where no cleanup blocks are emitted. I have to find the correct `ignore` directives. r? @oli-obk
PR #69562, which fixed a bug that was causing clippy to ICE, mistakenly passed the place holding the result of
Rvalue::Discriminantinstead of the place holding its operand toapply_discriminant_switch_effectas the enum place. As a result, no effect was applied at all, and we lost the perf benefits from marking inactive enum variants as uninitialized.edit: The regression test has been split into #69744.
r? @oli-obk