Put Local, Static and Promoted as one Base variant of Place#58631
Put Local, Static and Promoted as one Base variant of Place#58631bors merged 1 commit intorust-lang:masterfrom
Conversation
rust-highfive
added
the
S-waiting-on-review
oli-obk
left a comment
oli-obk
left a comment
There was a problem hiding this comment.
lgtm
Note that this will cause a max-rss regression, so we'll merge it after the beta cutoff, so we have 6 weeks to fix it (which should happen with the enum Place -> struct Place refactoring discussed in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/place.202.2E0/near/159082516
There was a problem hiding this comment.
Yeah, vim does that :). Fixing it.
There was a problem hiding this comment.
odd indentation here. Maybe write
if let StatementKind::Assign(
Place::Base(PlaceBase::Local(assigned_to)),
box rvalue,
) = &stmt.kind {There was a problem hiding this comment.
use the same indent as the Operand::Copy
There was a problem hiding this comment.
same here, just use the Place::Base(_) pattern
There was a problem hiding this comment.
pull Place::Local(result) into a variable before this statement and use that variable here and in the next statement
There was a problem hiding this comment.
deref takes ownership so that's not possible
src/librustc_mir/build/mod.rs
Outdated
There was a problem hiding this comment.
Maybe add a Place::RETURN_PLACE associated constant to Place to make all of these Place::Base(PlaceBase::Local(RETURN_PLACE)) shorter.
There was a problem hiding this comment.
Add a FIXME to new_temp to check if we want to make it return a Place directly if all use sites want a Place::Base anyway.
|
☔ The latest upstream changes (presumably #56113) made this pull request unmergeable. Please resolve the merge conflicts. |
spastorino
force-pushed
the
place2_1
branch
2 times, most recently
from
7e9b0f8 to
4b9d48f
Compare
|
@oli-obk fixed, rebased and force pushed :). |
|
@bors r+ |
|
📌 Commit 4b9d48f0a4f006480f64bb16b56714fef9f66db3 has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=1 |
@oli-obk What happened to this? |
|
⌛ Testing commit 4b9d48f0a4f006480f64bb16b56714fef9f66db3 with merge 8601df9996e847f042bc7a4c9d97fad4b6b0448a... |
|
💔 Test failed - checks-travis |
|
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 |
bors
added
S-waiting-on-review
kennytm
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #57609) made this pull request unmergeable. Please resolve the merge conflicts. |
spastorino
force-pushed
the
place2_1
branch
2 times, most recently
from
3534f50 to
4cfe141
Compare
|
@oli-obk just in case conversation in Zulip went unnoticed. This is ready to r+. |
|
@bors r+ |
|
Seems to be a bit-rotty PR, so @bors p=20 |
|
@bors r+ |
|
📌 Commit 0f993d5 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-travis, status-appveyor |
Tested on commit rust-lang/rust@17add24. Direct link to PR: <rust-lang/rust#58631> 💔 miri on windows: test-fail → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra). 💔 miri on linux: test-fail → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
Place::Local(x) is now Place::Base(PlaceBase::Local(x)) We need to merge this after the beta cut for this rust-lang/rust#58631 to work. /cc @oli-obk
|
FWIW, I was just looking at peak memory usage and I identified the In other words, I noticed this change. |
|
@nnethercote this was an intermediate step, we are going after this ... struct Place {
base: PlaceBase,
projection: PlaceProjection,
}
enum PlaceProjection {
Base,
Projection(Box<PlaceProjection>),
Deref,
Index(..),
...
}Going to provide a PR for this probably this week. |
|
That's makes |
|
Since the final layout will be struct Place<'tcx> {
base: PlaceBase,
projection: &'tcx [PlaceProjection],
}
enum PlaceProjection {
Projection(Box<PlaceProjection>),
Deref,
Index(..),
...
}we can also box I think that ideally we'd be able to have all occurences of struct Place {
base: PlaceBase,
projection: [PlaceProjection],
}But I don't know how to initialize such a type without statically knowing the length of the |
|
FWIW Also, we shouldn't combine the base and the projections, IMO. |
|
I expect And a @nnethercote Does that work for you? |
Yes, sorry. I've pasted an intermediate step. The final layout as @eddyb has said is ... struct Place<'tcx> {
base: PlaceBase,
projection: &'tcx [PlaceProjection],
}
enum PlaceProjection {
Projection(Box<PlaceProjection>),
Deref,
Index(..),
...
} |
|
Not quite, @eddyb is right, struct Place<'tcx> {
base: PlaceBase,
projection: &'tcx ty::List<PlaceProjection>,
}
enum PlaceProjection {
Projection(Box<PlaceProjection>),
Deref,
Index(..),
...
} |
|
What is the point of the recursive |
|
Ups, that's just a copy paste bug. In the enum Place {
Base(PlaceBase)
Projection(Box<PlaceProjection>),
}
struct PlaceProjection {
base: Place,
projection: PlaceProjectionElem,
}
enum PlaceProjectionElem {
Leaf,
Deref,
Index(..),
...
}while the target situation is struct Place {
base: PlaceBase
projections: &'tcx ty::List<PlaceProjectionElem>,
}
// Note: no more PlaceProjection
enum PlaceProjectionElem {
// Note: no more "Leaf"
Deref,
Index(..),
...
} |
I'm a bit confused by all the above back and forth. |
|
@nnethercote The plan is for it to become 16 bytes ( But also, |
|
Yeah, sorry my bad I was confusing you a lot. Thanks @eddyb for clarying it. |
I can see that an array is better than a linked list. But note that the size of Anyway, if the final size will be 16 bytes, that's great. |
10 participants
Related to #52708
The
Place2.0 representation use aBasevariant forLocal,StaticandPromotedso we start making this change in the currentPlaceto make the following steps simpler.r? @oli-obk