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

Add test for #59352 #77693

Merged
merged 1 commit into from
Jan 16, 2021
Merged

Add test for #59352 #77693

merged 1 commit into from
Jan 16, 2021

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 8, 2020

Issue #59352 reported an optimization regression with rustc 1.32.0+. That regression could be tracked to a change that caused a function to miss the size limit of llvm's inlining, which results in an unreachable panicing branch being generated.
Enabling mir inline solves the issue, but is currently only done for mir-opt-level>=2.

This PR adds a test that can serve as a regression test for #59352, if/when mir inlining gets mature enough for opt-level 1, or some other optimization can remove the panic.

@camelid camelid added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2020
@camelid
Copy link
Member

camelid commented Oct 30, 2020

Not sure what happened here: This PR never got an assignee! Must have been a highfive bug.

@camelid
Copy link
Member

camelid commented Oct 30, 2020

Assigned @Amanieu since I know they do some codegen stuff.

@Amanieu: Feel free to assign someone else instead.

@jyn514 jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-codegen Area: Code generation labels Oct 30, 2020
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2020
@Dylan-DPC-zz
Copy link

r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

r? @oli-obk cc @rust-lang/wg-mir-opt

How do we feel about a codegen test using mir-opt-level=2? It feels like we should maybe not land this for now (i.e. close) until it actually touches on behavior users can expect to see on stable; in particular my impression was that the various MIR opt levels are not very stable so we shouldn't really rely on them. I guess this might be better suited as a mir-opt test that can explicitly check we're performing the inlining here?

@bugadani
Copy link
Contributor Author

bugadani commented Dec 8, 2020

I guess this might be better suited as a mir-opt test that can explicitly check we're performing the inlining here?

I agree with you on most points, but the particular issue isn't about mir inlining, but a generated code performance regression. I believe mir-inlining itself wouldn't be a representative test for said issue.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 9, 2020

Ideally we'd have this happen entirely in MIR, but I don't see our current optimizations being up to the task. So... what do you think about a mix? This test as a regression test with the same test as a mir-opt test. Both tests should have comments referencing each other, so we can remove the LLVM test once mir-opts guarantee that the panic is compiled out even before going to LLVM.

@bugadani
Copy link
Contributor Author

bugadani commented Dec 9, 2020

Sounds good to me

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2021
@camelid
Copy link
Member

camelid commented Jan 15, 2021

Ping from triage: @bugadani could you address Oli's comment? Thanks!

@bugadani
Copy link
Contributor Author

@oli-obk I'm not exactly sure this is what you had in mind, but I added a mir-opt test that will actually fail when the panic gets optimized away. I kept the mir-opt-level=2 in that test so that the test output directly shows the panic.

@oli-obk oli-obk added the A-mir-opt Area: MIR optimizations label Jan 15, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Jan 15, 2021

Thanks, this is exactly what I had in mind. That test shows a few different things that we'll need to optimize in MIR before getting that panic optimized out.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 15, 2021

📌 Commit a0c5857 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2021
…laumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#77693 (Add test for rust-lang#59352)
 - rust-lang#80515 (Improve JS performance by storing length before comparing to it in loops)
 - rust-lang#81030 (Update mdbook)
 - rust-lang#81033 (Remove useless `clean::Variant` struct)
 - rust-lang#81049 (inline: Round word-size cost estimates up)
 - rust-lang#81054 (Drop a few unneeded borrows)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b7a9d6a into rust-lang:master Jan 16, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-mir-opt Area: MIR optimizations A-testsuite Area: The testsuite used to check the correctness of rustc 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.

9 participants