introduce local-scope to prevent StorageLive/StorageDead in statics
#42023
Conversation
nikomatsakis
force-pushed
the
issue-36799-ostn15_phf
branch
from
ab2f55c
to
eff0d62
Compare
|
NB: Didn't have time to fully test; let travis run :) |
nikomatsakis
force-pushed
the
issue-36799-ostn15_phf
branch
from
eff0d62
to
14b8e3d
Compare
src/librustc_mir/build/mod.rs
Outdated
| /// will be evaluated at compilation time. This is used to | ||
| /// suppress the addition of `StorageLive`/`StorageDead` | ||
| /// statements as well as `drop` statements. | ||
| building_const: bool, |
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
| /// Returns the scope that we should use as the lifetime of an | ||
| /// operand. Basically, an operand must live until it is consumed. | ||
| /// This is similar to, but not quite the same as, the temporary | ||
| /// scope (which can be larger or smaller). |
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
| if self.building_const { | ||
| None | ||
| } else { | ||
| Some(self.topmost_scope()) // FIXME(#6393) |
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) |
arielb1
added
the
S-waiting-on-review
nikomatsakis
force-pushed
the
issue-36799-ostn15_phf
branch
from
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 |
|
⌛ Testing commit 8a4e593 with merge 5b13bff... |
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