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

Don't `bug` when taking discriminant of generator during dataflow #69562

Merged

Conversation

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Feb 28, 2020

The proper fix for rust-lang/rust-clippy#5239. Rvalue::Discriminant is used on generators as well as enums. This didn't cause a test failure in rustc since we don't need to do any dataflow passes until after the generator transform that adds the Rvalue::Discriminant.

This required a small refactoring. diff -w is beneficial.

r? @oli-obk
cc @JohnTitor

@ecstatic-morse

This comment has been minimized.

Copy link
Contributor Author

ecstatic-morse commented Feb 28, 2020

Alternatively, we could just fall back to the normal SwitchInt handling when an unexpected type is the operand of an Rvalue::Discriminant. The current interface for per-edge effects can only work with enums.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Feb 29, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 29, 2020

📌 Commit 52ad1e7 has been approved by oli-obk

bors added a commit that referenced this pull request Mar 1, 2020
Rollup of 7 pull requests

Successful merges:

 - #69397 (bootstrap: Remove commit hash from LLVM version suffix to avoid rebuilds)
 - #69549 (Improve MinGW detection when cross compiling )
 - #69562 (Don't `bug` when taking discriminant of generator during dataflow)
 - #69579 (parser: Remove `Parser::prev_span`)
 - #69580 (use .copied() instead of .map(|x| *x) on iterators)
 - #69583 (Do not ICE on invalid type node after parse recovery)
 - #69605 (Use `opt_def_id()` over `def_id()`)

Failed merges:

r? @ghost
@bors bors merged commit 48ec252 into rust-lang:master Mar 1, 2020
4 checks passed
4 checks passed
pr Build #20200228.45 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
ecstatic-morse added a commit to ecstatic-morse/rust that referenced this pull request Mar 3, 2020
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.
@jonas-schievink

This comment has been minimized.

Copy link
Member

jonas-schievink commented Mar 3, 2020

@ecstatic-morse

This comment has been minimized.

Copy link
Contributor Author

ecstatic-morse commented Mar 4, 2020

@jonas-schievink See #69676.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 4, 2020
…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
Centril added a commit to Centril/rust that referenced this pull request Mar 6, 2020
…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
bors added a commit that referenced this pull request Mar 6, 2020
Pass correct place to `discriminant_switch_effect`

PR #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 #69744.

r? @oli-obk
bors added a commit that referenced this pull request Mar 7, 2020
Pass correct place to `discriminant_switch_effect`

PR #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 #69744.

r? @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.