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

Disable inactive enum variant drop elaboration optimization #76081

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Aug 29, 2020

In #74551, users of several tier 2 platforms have reported that the optimization in #68528 was causing a miscompilation. After that issue was filed, I double-checked my work and, as far as I can tell, that optimization is theoretically sound and implemented correctly. It's possible, however, that #68528 ends up triggering an LLVM or rustc codegen bug that results in invalid code on those platforms. It's also possible that the same codegen bug is lurking on tier 1 platforms, but does not present as a SEGFAULT.

This PR disables the optimization in #68528. This should be useful for a perf run and so that affected users can apply this patch to build Firefox using the latest rustc. However, it's not clear whether we should actually merge it. I'm not sure what the policy should be for disabling optimizations that cause unrelated bugs (assuming I'm correct that #68528 is sound) on tier 2 platforms. Do we disable it for everyone? No one? Only affected platforms? All architectures besides x86?

This PR is more complex than one might expect because there's a const-checking pass (#71824) that runs after drop elaboration and depends on the basic ideas of #68528 to allow certain functions (notably Option::unwrap) to be const. Now that drop elaboration no longer removes provably dead drop terminators in cases like these, I have to replicate some of the logic from drop elaboration in the const-checking module. If we decide to disable the optimization for all platforms, we could move these const checks back before drop elaboration to simplify the code somewhat. It was split up originally to avoid duplicating work.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2020
@ecstatic-morse
Copy link
Contributor Author

r? @pnkfelix (Since they are assigned to #74551)

@ecstatic-morse
Copy link
Contributor Author

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Aug 29, 2020

⌛ Trying commit c8d7d03 with merge efa179a97926d932a0f4aee5186d5e20710c8258...

@bors
Copy link
Contributor

bors commented Aug 29, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: efa179a97926d932a0f4aee5186d5e20710c8258 (efa179a97926d932a0f4aee5186d5e20710c8258)

@rust-timer
Copy link
Collaborator

Queued efa179a97926d932a0f4aee5186d5e20710c8258 with parent 7fc048f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (efa179a97926d932a0f4aee5186d5e20710c8258): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@joshtriplett
Copy link
Member

Looks like this is a substantial loss across the board.

@ecstatic-morse
Copy link
Contributor Author

@joshtriplett Yes, there's a reason I'm trying quite hard to avoid disabling this 😄. Ideally we would be able to find someone with the time, LLVM expertise and access to hardware needed to debug the root cause of #74551. Unfortunately, I think @pnkfelix no longer has the third one, and I've never had any of them.

I suppose we could disable this optimization only on affected platforms, but that sort of divergence feels pretty bad. Maybe we should nominate this for T-compiler discussion? #74551 was already discussed but that was a month ago.

@joshtriplett
Copy link
Member

I'm not a fan of having optimization passes vary by target for reasons of brokenness. If it were a matter of "it helps on some platforms but not others", that'd be a good reason.

@crlf0710 crlf0710 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 18, 2020
@Dylan-DPC-zz
Copy link

@ecstatic-morse any updates?

@ecstatic-morse
Copy link
Contributor Author

Nightly users (or ones who know about RUSTC_BOOTSTRAP) can disable this via a flag now (see #77423). I'll close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants