introduce local-scope to prevent StorageLive/StorageDead in statics#42023
introduce local-scope to prevent StorageLive/StorageDead in statics#42023bors merged 2 commits intorust-lang:masterfrom
StorageLive/StorageDead in statics#42023Conversation
ab2f55c to
eff0d62
Compare
|
NB: Didn't have time to fully test; let travis run :) |
eff0d62 to
14b8e3d
Compare
src/librustc_mir/build/mod.rs
Outdated
There was a problem hiding this comment.
Doesn't Cx have a way to get at this?
There was a problem hiding this comment.
I didn't see an obvious one, but it could store e.g. the MirSource
src/librustc_mir/build/scope.rs
Outdated
There was a problem hiding this comment.
This is wrong in the general case - why don't we rely on temporary_scope of None?
There was a problem hiding this comment.
I was just trying to preserve the existing logic. To be honest, I don't quite understand the reasoning here -- it does seem to me that top-most scope is not necessarily correct. @arielb1 can maybe elaborate more -- it was introduced in arielb1@1425eed as part of the PR to fix #38669, though I didn't find the PR itself (presumably I could trace to find the bors commit...)
There was a problem hiding this comment.
In an expression like foo(bar(), baz()), we want the operands holding the result of bar() and baz() to be destroyed right after foo is called, and if calling baz panics then bar is destroyed first - at least that is the ordering that makes the most sense to me.
There was a problem hiding this comment.
To be clear, when you say "destroyed" here, you don't mean that we will run any destructors, you just mean that we'll reclaim the storage, right? (Because I would expect the values to have been moved to the callee in any case...)
There was a problem hiding this comment.
More concretely, if we have foo(Box::new(2), panic!()), because we evaluate arguments from left to right, we allocate the Box and then free it (because foo can also panic, we actually emit a drop flag there - but a "sufficiently smart" drop unswitching pass could eliminate that). We also emit a drop after the call on the "normal" path, but that drop is a no-op because the value is already consumed.
There was a problem hiding this comment.
We might want to add the above to the comment.
src/librustc_mir/build/scope.rs
Outdated
There was a problem hiding this comment.
FIXME(#6393) should be moved to the call-sites - it's about writing the code in one line after we get NLL.
There was a problem hiding this comment.
I don't understand what you mean by "writing the code in one line"
There was a problem hiding this comment.
self.as_operand(block, self.local_scope(), expr)
There was a problem hiding this comment.
Oh, I see. I don't think I'd even bother with a FIXME then.
|
So this is intended to be a performance optimization? |
Memory usage, primarily, but it probably affects compilation time more generally, yeah. (If you have large constants) |
f1e3b49 to
19bf544
Compare
|
r=me, but if you want to improve the comment that's ok too. |
|
Sooo what's the state of this PR? @nikomatsakis are you going to improve the comment in the way @arielb1 suggested, or do you officially r+ @arielb1? |
|
I'll improve the comment. I think it's otherwise r=arielb1, from what I understood. |
|
@bors r=arielb1 |
|
📌 Commit 8a4e593 has been approved by |
introduce local-scope to prevent `StorageLive`/`StorageDead` in statics In investigating #36799, I found that we were creating storage-live/storage-dead instructions in statics/constants, where they are not needed. This arose due to the fix for local scopes. This PR tries to fix that (and adds a test -- I'm curious if there is a way to make that test more targeted, though). r? @arielb1
|
☀️ Test successful - status-appveyor, status-travis |
In investigating #36799, I found that we were creating storage-live/storage-dead instructions in statics/constants, where they are not needed. This arose due to the fix for local scopes. This PR tries to fix that (and adds a test -- I'm curious if there is a way to make that test more targeted, though).
r? @arielb1