Skip a shared borrow of a immutable local variables #53909
Conversation
rust-highfive
added
the
S-waiting-on-review
|
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 |
| @@ -223,10 +223,10 @@ impl<'tcx> Mir<'tcx> { | |||
| } else if self.local_decls[local].name.is_some() { | |||
| LocalKind::Var | |||
| } else { | |||
| debug_assert!( | |||
There was a problem hiding this comment.
I'd say just delete the debug_assert!, there's no value in leaving it in commented out
| } | ||
|
|
||
| let mut has_storage_dead = HasStorageDead(BitArray::new(mir.local_decls.len())); | ||
| has_storage_dead.visit_mir(mir); |
There was a problem hiding this comment.
In a similar prototype that I was working on, I combined this bit-array computation into a new enum for locals_are_invalidated_at_exit. (That is, I tried to ensure that we only did this work when locals_are_invalidated_at_exit is false here.) Just a stray thought on something you might want to consider incorporating.
There was a problem hiding this comment.
agree, I will do this join as a next step, but may be the bit-array can be used somewhere else? or just make it an Option but I cannot find a nice name for it
There was a problem hiding this comment.
Well, if I understand correctly, you can only have the bit-array when locals_are_invalidated_at_exit is false in the first place. So even if the bit-array can be used somewhere else, you should be able to extract it, assuming you've changed the type like so: locals_are_invalidated_at_exit: LocalInvalidationState, where
enum InvalidationState {
LocalsAreInvalidated(BitArray<Local>),
LocalsNotInvalidated,
}
| @@ -63,7 +63,12 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
| block.and(Rvalue::Repeat(value_operand, count)) | |||
| } | |||
| ExprKind::Borrow { region, borrow_kind, arg } => { | |||
| let arg_place = unpack!(block = this.as_place(block, arg)); | |||
| debug!("#### as rvalue {:?}", (borrow_kind)); | |||
There was a problem hiding this comment.
nit: why are there parens around (borrow_kind) here?
|
The travis failures seem to indicate that you need to update some of the |
|
@bors try |
|
⌛ Trying commit 55e69cfc1952ea0290b4a9b02123958a69631e8d with merge 5dacc1649d847cca076ace72bc21130bc114a383... |
| mir: &Mir<'tcx>, | ||
| has_storage_dead: &BitArray<Local>, | ||
| locals_are_invalidated_at_exit: bool, | ||
| ) -> bool { |
There was a problem hiding this comment.
Nit: this closing paren should be "unindented"; or just run rustfmt
| Place::Local(_) => false, | ||
| Place::Promoted(_) => false, | ||
| Place::Local(index) => { | ||
| let ignore = !locals_are_invalidated_at_exit && |
There was a problem hiding this comment.
It feels like this is worth a comment. Something like:
If a local variable is immutable, then we only need to track borrows to guard against two kinds of errors:
- The variable being dropped while still borrowed (e.g., because the fn returns a reference to a local variable)
- The variable being moved while still borrowed
In particular, the variable cannot be mutated -- the "access checks" will fail -- so we don't have to worry about mutation while borrowed.
There was a problem hiding this comment.
But writing that made me realize we may be missing a case! That is, we're not checking for moves. This is ultimately only having an effect in constants, so it may be hard to write code that exploits this at present, but it seems bad.
| } | ||
|
|
||
| fn expr_as_place(&mut self, | ||
| mut block: BasicBlock, | ||
| expr: Expr<'tcx>) | ||
| expr: Expr<'tcx>, | ||
| immutable: bool) |
There was a problem hiding this comment.
maybe we can pass a hir::Mutability instead -- something more declarative than a naked boolean
| } | ||
|
|
||
| fn expr_as_temp(&mut self, | ||
| mut block: BasicBlock, | ||
| temp_lifetime: Option<region::Scope>, | ||
| expr: Expr<'tcx>) | ||
| expr: Expr<'tcx>, | ||
| immutable: bool) |
| let this = self; | ||
|
|
||
| let expr_span = expr.span; | ||
| let source_info = this.source_info(expr_span); | ||
| if let ExprKind::Scope { region_scope, lint_level, value } = expr.kind { | ||
| return this.in_scope((region_scope, source_info), lint_level, block, |this| { | ||
| this.as_temp(block, temp_lifetime, value) | ||
| if immutable { | ||
| this.as_immutable_temp(block, temp_lifetime, value) |
There was a problem hiding this comment.
then we would pass the argument here (this.as_temp(block, temp_lifetime, value, mutability)), perhaps?
| self.tcx, | ||
| self.mir, | ||
| &self.borrow_set.has_storage_dead, | ||
| self.locals_are_invalidated_at_exit) { |
There was a problem hiding this comment.
Nit: formatting seems off. I think the ) should go on the next line:
if place.ignore_borrow(
foo,
bar,
) {
return;
}
|
☀️ Test successful - status-travis |
|
@rust-timer build 5dacc1649d847cca076ace72bc21130bc114a383 |
|
Success: Queued 5dacc1649d847cca076ace72bc21130bc114a383 with parent 1c2e17f, comparison URL. |
|
😮 |
|
This brings ucd down to 116% ! |
mikhail-m1
changed the title
| has_storage_dead.visit_mir(mir); | ||
| let mut has_storage_dead_or_moved = has_storage_dead.0; | ||
| for move_out in &move_data.moves { | ||
| let mut path = &move_data.move_paths[move_out.path]; |
There was a problem hiding this comment.
I think we should make this a helper function. Something like:
impl MoveData {
/// For the move path `mpi`, returns the root local variable (if any) that starts the path.
/// (e.g., for a path like `a.b.c` returns `Some(a)`)
pub fn base_local(&self, mut mpi: MovePathIndex) -> Option<Local> {
loop {
let path = &self.move_paths[mpi];
if let Place::Local(l) = path.place { return Some(l); }
if let Some(parent) = path.parent { mpi = parent; continue } else { return None }
}
}
}| // In particular, the variable cannot be mutated -- the "access checks" will fail -- | ||
| // so we don't have to worry about mutation while borrowed. | ||
| Place::Local(index) => { | ||
| if let LocalsStateAtExit::SomeAreInvalidated { has_storage_dead_or_moved } = |
There was a problem hiding this comment.
Nit: Personally I'd use a match here, which I think would indent more cleanly:
match locals_state_at_exit {
LocalsStateAtExit::AllAreInvalidated => false,
LocalsStateAtExit::SomeAreInvalidated { has_storage_dead_or_moved } => {
...
}
}as a bonus, this is an exhaustive match, so if we ever add a third variant, we'll get a compilation error.
| pub fn as_immutable_place<M>(&mut self, | ||
| block: BasicBlock, | ||
| expr: M) | ||
| -> BlockAnd<Place<'tcx>> |
There was a problem hiding this comment.
Nit: indentation is wacky. Run rustfmt?
| self.expr_as_place(block, expr, Mutability::Mut) | ||
| } | ||
|
|
||
| /// Compile `expr`, yielding a place that we can move from etc. |
There was a problem hiding this comment.
Nit: I'd add a comment here like:
Mutability note: The caller of this method promises only to read from the resulting
place. The place itself may or may not be mutable:
- If this expr is a place expr like
a.b, then we will return that place. - Otherwise, a temporary is created: in that event, it will be an immutable temporary.
There was a problem hiding this comment.
Maybe argues for calling it as_read_only_place
nikomatsakis
added
S-waiting-on-author
|
@bors r+ |
|
📌 Commit d0c1e5a has been approved by |
bors
added
S-waiting-on-bors
Skip a shared borrow of a immutable local variables issue #53643 r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
issue #53643
r? @nikomatsakis