-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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 utility to find locals that don't use StorageLive
annotations and use it for MaybeStorageLive
#70447
Conversation
Unfortunately this causes an assertion to trigger. @tmandry I would like to make |
The job 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 |
☔ The latest upstream changes (presumably #70451) made this pull request unmergeable. Please resolve the merge conflicts. |
I wonder if it makes sense to share code with the part of Miri that decides which locals are live when a stack frame is pushed? On the one hand, having independent implementations could be useful as they can check each other, on the other hand, sharing code always seems nice. Currently, these to analyses differ in the sense that Miri treats all locals as initially-live in consts and statics, to avoid the overhead of walking the entire MIR. @oli-obk added this optimization a while ago, but @eddyb said it probably never was correct. An alternative would be to just say that the CTFE machine, for performance reasons, just ignores |
@ecstatic-morse I think you can take that part out of |
@RalfJung We should probably be encoding whether a variable is live on function entry in its |
If we can be explicit in the MIR about this, that would be even better. :) |
6645209
to
ae89af5
Compare
The job 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 |
ae89af5
to
cde5ad0
Compare
☔ The latest upstream changes (presumably #70449) made this pull request unmergeable. Please resolve the merge conflicts. |
cde5ad0
to
240c7de
Compare
☔ The latest upstream changes (presumably #70672) made this pull request unmergeable. Please resolve the merge conflicts. |
// Locals that are always live or ones that need to be stored across | ||
// suspension points are not eligible for overlap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire function only cares about stored_locals
, to be clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me after nits
Before, it ignored the first argument and marked all variables without `Storage*` annotations as dead.
240c7de
to
209087b
Compare
📌 Commit 209087b has been approved by |
🌲 The tree is currently closed for pull requests below priority 10, this pull request will be tested once the tree is reopened |
… r=tmandry Add utility to find locals that don't use `StorageLive` annotations and use it for `MaybeStorageLive` Addresses rust-lang#70004 (comment) (cc @RalfJung). The only dataflow analysis that is incorrect in this case is `MaybeStorageLive`. `transform/generator.rs` implemented custom handling for this class of locals, but other consumers of this analysis (there's one in [clippy](https://github.com/rust-lang/rust-clippy/blob/513b46793e98ce5b412d388a91f6371d6a9b290b/clippy_lints/src/redundant_clone.rs#L402)) would be incorrect. r? @tmandry
☀️ Test successful - checks-azure |
Addresses #70004 (comment) (cc @RalfJung).
The only dataflow analysis that is incorrect in this case is
MaybeStorageLive
.transform/generator.rs
implemented custom handling for this class of locals, but other consumers of this analysis (there's one in clippy) would be incorrect.r? @tmandry