-
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
[WIP] Take 2: change how MIR represents Place #54426
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
22099d7
to
3cfa0bc
Compare
src/librustc/mir/tcx.rs
Outdated
pub fn split_projection<'cx, 'gcx>( | ||
&self, | ||
tcx: TyCtxt<'cx, 'gcx, 'tcx>, | ||
) -> (NeoPlace<'tcx>, Option<&'tcx PlaceElem<'tcx>>) { |
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.
I find this operation dishearteningly inefficient :(.
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.
If we don't "re-intern" the slice of elements, then in what way is it inefficient?
I think it'd be nice to be able to treat a NeoPlace
as either a list or a tree as you choose, and this operation allows for that (although I would probably tweak the signature; I'd prefer it returns something like "Result<(NeoPlace, ProjectionElem), Base>`. ie., either you reached the base case, or else not.
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.
Maybe something like
enum NeoPlaceTree {
Leaf(PlaceBase),
Branch(NeoPlace<'tcx>, ProjectionElem<'tcx>),
}
and into_tree(self) -> NeoPlaceTree
as the signature (lifetimes elided, clearly).
src/librustc/mir/tcx.rs
Outdated
tcx: &TyCtxt<'cx, 'gcx, 'tcx>, | ||
) -> Option<Field> { | ||
let (place, by_ref) = if let Some(ProjectionElem::Deref) = self.elems.last() { | ||
(self.projection_base(*tcx), true) |
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.
You can rewrite this as going backwards in the projection list.
If you want, you can use let mut elems = self.elems.iter().rev();
and then call next()
on that once, check whether it's Deref
, and if it is, call next()
again.
You can also use indices.
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.
It feels like this is a case where treating the projection as a tree (sort of what this code is doing) is a bit cleaner. In particular, we want to look for something like P.f
or *P.f
and then test the type of P
. We can certainly "walk backwards" over the elements, but once we've peeled off the *
and the .f
we sitll kind of want to extract the P
as a unit, right?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c18a525
to
0e689b1
Compare
src/librustc/mir/tcx.rs
Outdated
Some((projection, place_elems)) => ( | ||
NeoPlace { | ||
base: self.clone().base, | ||
elems: tcx.intern_place_elems(place_elems), |
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.
There is no need to intern again here; just use place_elems
directly. It is already pointing into interned memory, right?
8e4e8aa
to
aee5eae
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 |
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 |
606860a
to
f0ac652
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 |
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 #58254) made this pull request unmergeable. Please resolve the merge conflicts. |
After conversations about MIR 2.0, I think this PR doesn't make sense anymore as is. In any case this is being discussed in Zulip. |
Closes #52708
Introduce the new
Place
with "incremental refactoring" instead of the previous approach which is hard to maintain.r? @nikomatsakis