Uh oh!
There was an error while loading. Please reload this page.
[WIP] Remove StorageDead and StorageLive statements using MIR locals of type () - #70004
Closed
Zoxc wants to merge 2 commits into
Closed
[WIP] Remove StorageDead and StorageLive statements using MIR locals of type ()#70004Zoxc wants to merge 2 commits into
StorageDead and StorageLive statements using MIR locals of type ()#70004Zoxc wants to merge 2 commits into
Conversation
Zoxc
commented
Mar 14, 2020
ContributorAuthor
@bors try @rust-timer queue |
rust-timer
commented
Mar 14, 2020
Collaborator
Awaiting bors try build completion |
bors
commented
Mar 14, 2020
Collaborator
bors added a commit
that referenced
this pull request
Mar 14, 2020
[WIP] Remove `StorageDead` and `StorageLive` statements using MIR locals of type `()` r? @oli-obk
rust-highfive
commented
Mar 14, 2020
Contributor
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 |
alex
reviewed
Mar 14, 2020
Comment on lines
+13
to
+14
| let unit_locals: SmallVec<[_; 32]> = | ||
| body.local_decls.iter().map(|local| local.ty == tcx.types.unit).collect(); |
Member
There was a problem hiding this comment.
I'm far from an expert in this code, but what's the need to allocate an array here instead of just indexing into body.local_decls and comparing the types on line 20?
Contributor
There was a problem hiding this comment.
the code below mutates the body, so we can't hold an immutable at the same time. technically it is doable, but not with the current API. Maybe basic_block_mut could return a reference to the rest of the body... not sure, but there's still quite some design space available here
Contributor
There was a problem hiding this comment.
Shouldn't you be able to use https://doc.rust-lang.org/nightly/nightly-rustc/rustc/mir/struct.BodyAndCache.html#method.basic_blocks_and_local_decls_mut for that?
bors
commented
Mar 14, 2020
Collaborator
☀️ Try build successful - checks-azure |
rust-timer
commented
Mar 14, 2020
Collaborator
Queued ba94172 with parent 0f1e814, future comparison URL. |
Centril
reviewed
Mar 14, 2020
| for block in body.basic_blocks_mut() { | ||
| for stmt in &mut block.statements { | ||
| match &stmt.kind { | ||
| StatementKind::StorageLive(l) | StatementKind::StorageDead(l) => { |
Contributor
There was a problem hiding this comment.
Suggested change
| StatementKind::StorageLive(l) | StatementKind::StorageDead(l) =>{ | |
| ifletStatementKind::StorageLive(l) | StatementKind::StorageDead(l) =&stmt.kind{ |
rust-timer
commented
Mar 14, 2020
Collaborator
Finished benchmarking try commit ba94172, comparison URL. |
jonas-schievink
commented
Mar 14, 2020
Contributor
|
Zoxc
commented
Mar 14, 2020
ContributorAuthor
What uses |
jonas-schievink
commented
Mar 14, 2020
Contributor
MIR borrowck if I'm not mistaken. From #68622:
|
jonas-schievink
commented
Mar 14, 2020
Contributor
The failing test looks like this: // https://github.com/rust-lang/rust/issues/55223unionFoo<'a>{y:&'a(),long_live_the_unit:&'static(),}constFOO:&() = {//~ ERROR any use of this value will cause an errorlet y = ();unsafe{Foo{y:&y }.long_live_the_unit}};fnmain(){}The error is no longer emitted, which makes sense given the changes |
bjorn3
commented
Mar 14, 2020
Member
This runs after borrowck. |
jonas-schievink
commented
Mar 14, 2020
Contributor
@bjorn3 yeah, was about to comment that. There still seem to be things relying on this though, do const checking passes use optimized MIR maybe? |
wesleywiser
commented
Mar 15, 2020
Member
If we do this for |
Zoxc
commented
Mar 15, 2020
ContributorAuthor
@wesleywiser Yes. My goal was to improve the compiler performance here, so I just checked for |
bjorn3
commented
Mar 15, 2020
Member
Maybe do this during codegen of StorageLive and StorageDead? That way const eval won't notice the change, and layout info is already computed previously. |
oli-obk
commented
Mar 15, 2020
Contributor
we should not diverge codegen and const eval. cc @RalfJung a while back I tried making all zsts have integer addresses, that would have worked here, too. there were some problems with it though. please run |
RalfJung
commented
Mar 15, 2020
Member
I think Miri will be fine in the sense that this strictly removes UB. The question is, is anything relying on that being UB? But all the relevant analyses should be looking at StorageDead/Live, I assume that will just mean they will be less precise for optimizations that run after this. This removal means that address of ZST locals are (after this pass) guaranteed to remain the same throughout the execution of the function, including ZST locals inside a loop remaining the same for multiple loop iterations: loop{let x = ();println!("{:?}",&x as*const _);// prints the same thing each round}@ecstatic-morse@eddyb I hope our dataflow analyses treat this correctly, i.e. they will treat these locals as being alive throughout the entire function? |
Zoxc
commented
Mar 20, 2020
ContributorAuthor
I think the test failure happens because Miri does not mark the locals as deallocated on return like it should. I also noticed that it doesn't initialize locals as dead if It seems like the semantics for Mir used for constant contexts diverge from Mir intended to execute that run-time. I'm not sure how intentional this is, but it doesn't seem great. |
rust-highfive
commented
Mar 21, 2020
Contributor
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 |
eddyb
commented
Mar 21, 2020
Contributor
Hmm, do we use |
Member
I don't think Miri depends on whether this is CTFE or run-time code. We should make sure our dataflow analyses agree! This is observable to the program by checking address identity: by removing This is just an optimization to avoid having to scan the entire body, as that's slow for some big constants: rust/src/librustc_mir/interpret/eval_context.rs Lines 540 to 541 in 5f13820 It should not change anything, as those do not have Storage* anyway. That is a difference in MIR building between constants and run-time, nor a difference in Miri semantics. (Another difference is that MIR in constants gets arithmetic overflow checks even in release mode. Not sure if there are more differences.) |
Contributor
That's not true though, except at the outermost scope (and there we might still emit |
Member
I guess this means MIR building changed since @oli-obk introduced this optimization. |
Contributor
What's more likely IMO is @oli-obk only tried simple examples, and nothing actually needing If you remind me (e.g. later on Zulip) I can try to craft an example that produced (oops, if you saw @RalfJung's comment being edited, it's because apparently, "Quote Reply" on the new GitHub mobile app edits the original comment?! just reported it, I'll try to avoid it until it's fixed) |
Dylan-DPC-zz pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Apr 9, 2020
… 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
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 10, 2020
…=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
nikomatsakis
commented
Apr 10, 2020
Contributor
Closing this pull request as Zoxc is stepping back from compiler development; see rust-lang/team#316. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
14 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
r? @oli-obk