Prune unreachable variants of coroutines #135471
Conversation
rustbot
added
S-waiting-on-review
|
I guess one thing that this PR doesn't do but it could is actually remove the variants from the |
This comment has been minimized.
This comment has been minimized.
compiler-errors
force-pushed
the
coroutine-unreachable
branch
from
a7a5903 to
802fb2a
Compare
| // This is preceded by a read of the discriminant. If we don't find this, then | ||
| // we must have optimized away the switch, so bail. | ||
| let StatementKind::Assign(box (discr_place, Rvalue::Discriminant(discr_local))) = | ||
| &bb0.statements[if is_pinned { 1 } else { 0 }].kind |
There was a problem hiding this comment.
If we optimize out too much, we may not even have a statement left.
There was a problem hiding this comment.
Yeah, but we already check that there's at least 1 statement in https://github.com/rust-lang/rust/pull/135471/files#diff-2b899097018634da368ec3c772523e10def01aef54b950e07c125a49e374a3e3R329. I don't know if there will ever be a case where that succeeds but this fails.
| } | ||
| // The derefer may have inserted a local to access the variant. | ||
| // Make sure we keep track of it here. | ||
| StatementKind::Assign(box (place, Rvalue::CopyForDeref(deref_place))) => { |
There was a problem hiding this comment.
This could be a Rvalue::Operand too.
There was a problem hiding this comment.
Rvalue::Operand(RValue::Copy(_)) would mean that GVN has passed by, and I think GVN would've collapsed all of them into this one: https://github.com/rust-lang/rust/pull/135471/files#diff-2b899097018634da368ec3c772523e10def01aef54b950e07c125a49e374a3e3R332
I can probably assert against it I guess :)
|
Also, I'm really not certain whether this PR is worth landing. I can't really seem to find production code where this functionality even gets triggered :) |
|
Did you try it on the codebase that the issue was reported from? https://github.com/chipsalliance/caliptra-mcu-sw |
|
I mean, yes, that codebase does trigger the optimization (well, I didn't check but I would expect it to given the mir-opt test I wrote). I just mean in practice, I'm not totally certain how many coroutine variants we'd actually be pruning -- I assume most people aren't writing trivially optimizable (well, trivially optimized-out) yield points in their async code. That is to say, I'm not good at doing the type of analysis that indicates whether this logic (which is fairly nontrivial) is warranted. |
Overall I do not think the vibes are very good. This is a lot of complexity for an optimization that seems usually ineffectual, and from my perspective of knowing a bit about generator lowering, doesn't seem like a common-sense optimization (in the same sense that we expect LLVM to find the optimal instruction sequence for some bit-twiddling operation). |
|
I do not expect LLVM to be able to approximate anything near to this optimization, especially if it were extended to totally drop the unreachable variants from the coroutine layout (i.e. make the coroutine smaller) But also I don't want to carry this forward anyways so idgaf |
|
Yeah I also don't expect LLVM to figure this out. I just think there are two reasons to do an optimization, "it's useful" (which this is not demonstrated to be) and "we've educated our users to expect this" (which I do not think this fits either). |
Fixes #135468
SimplifyCfgdoesn't detect cases where coroutine variants are effectively unreachable since they'll never reach a specific variant if we repeatedly poll the coroutine from the "uninitialized" variant 0. This PR attempts to implement that, since otherwise futures suffer from dead code problems across await points.I gotta explain a bunch about why this is so weird, sitll. But, anyone have a vibe-check about the approach?
r? mir-opt