Skip to content
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.

Sign up for GitHub

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

Stop using the unpack! macro in MIR building #127416

Closed
wants to merge 3 commits into from
+234 −251
Closed

Stop using the unpack! macro in MIR building #127416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9b0e97
Remove the non-assigning form of `unpack!`
Zalathar Jul 6, 2024
5066e6c
Stop using `unpack!` for `BlockAnd<()>`
Zalathar Jul 6, 2024
8357e4e
Stop using the `unpack!` macro
Zalathar Jul 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions compiler/rustc_mir_build/src/build/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
StmtKind::Expr { scope, expr } => {
this.block_context.push(BlockFrame::Statement { ignores_expr_result: true });
let si = (*scope, source_info);
unpack!(
block = this.in_scope(si, LintLevel::Inherited, |this| {
block = this
.in_scope(si, LintLevel::Inherited, |this| {
this.stmt_expr(block, *expr, Some(*scope))
})
);
.unpack_block_and_unit();
}
StmtKind::Let {
remainder_scope,
Expand Down Expand Up @@ -166,14 +166,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let dummy_place = this.temp(this.tcx.types.never, else_block_span);
let failure_entry = this.cfg.start_new_block();
let failure_block;
unpack!(
failure_block = this.ast_block(
failure_block = this
.ast_block(
dummy_place,
failure_entry,
*else_block,
this.source_info(else_block_span),
)
);
.unpack_block_and_unit();
this.cfg.terminate(
failure_block,
this.source_info(else_block_span),
Expand Down Expand Up @@ -226,7 +226,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
});
matching.and(failure)
});
let failure = unpack!(block = failure_and_block);
let failure = failure_and_block.unpack(&mut block);
this.cfg.goto(failure, source_info, failure_entry);

if let Some(source_scope) = visibility_scope {
Expand Down Expand Up @@ -267,8 +267,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let initializer_span = this.thir[init].span;
let scope = (*init_scope, source_info);

unpack!(
block = this.in_scope(scope, *lint_level, |this| {
block = this
.in_scope(scope, *lint_level, |this| {
this.declare_bindings(
visibility_scope,
remainder_span,
Expand All @@ -279,10 +279,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
this.expr_into_pattern(block, &pattern, init)
// irrefutable pattern
})
)
.unpack_block_and_unit();
} else {
let scope = (*init_scope, source_info);
unpack!(this.in_scope(scope, *lint_level, |this| {
let _: BlockAnd<()> = this.in_scope(scope, *lint_level, |this| {
this.declare_bindings(
visibility_scope,
remainder_span,
Expand All @@ -291,7 +291,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
None,
);
block.unit()
}));
});

debug!("ast_block_stmts: pattern={:?}", pattern);
this.visit_primary_bindings(
Expand Down Expand Up @@ -333,7 +333,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
this.block_context
.push(BlockFrame::TailExpr { tail_result_is_ignored, span: expr.span });

unpack!(block = this.expr_into_dest(destination, block, expr_id));
block = this.expr_into_dest(destination, block, expr_id).unpack_block_and_unit();
let popped = this.block_context.pop();

assert!(popped.is_some_and(|bf| bf.is_tail_expr()));
Expand All @@ -355,7 +355,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Finally, we pop all the let scopes before exiting out from the scope of block
// itself.
for scope in let_scope_stack.into_iter().rev() {
unpack!(block = this.pop_scope((*scope, source_info), block));
block = this.pop_scope((*scope, source_info), block).unpack_block_and_unit();
}
// Restore the original source scope.
this.source_scope = outer_source_scope;
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_mir_build/src/build/expr/as_operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block.and(Operand::Constant(Box::new(constant)))
}
Category::Constant | Category::Place | Category::Rvalue(..) => {
let operand = unpack!(block = this.as_temp(block, scope, expr_id, Mutability::Mut));
let operand =
this.as_temp(block, scope, expr_id, Mutability::Mut).unpack(&mut block);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings about this, the flow of block is easier to follow with the macro imo. Have we considered:

Suggested change
this.as_temp(block, scope, expr_id, Mutability::Mut).unpack(&mut block);
this.as_temp(&mut block, scope, expr_id, Mutability::Mut);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like using &mut block as an in+out parameter, and have used it in some of my previous coverage work.

The main reason I didn't try to use it here was to keep this PR as modest as possible. But it's certainly something I'd like to explore.

// Overwrite temp local info if we have something more interesting to record.
if !matches!(local_info, LocalInfo::Boring) {
let decl_info =
Expand Down Expand Up @@ -174,7 +175,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// type, and that value is coming from the deref of a box.
if let ExprKind::Deref { arg } = expr.kind {
// Generate let tmp0 = arg0
let operand = unpack!(block = this.as_temp(block, scope, arg, Mutability::Mut));
let operand =
this.as_temp(block, scope, arg, Mutability::Mut).unpack(&mut block);

// Return the operand *tmp0 to be used as the call argument
let place = Place {
Expand Down
32 changes: 17 additions & 15 deletions compiler/rustc_mir_build/src/build/expr/as_place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
mut block: BasicBlock,
expr_id: ExprId,
) -> BlockAnd<Place<'tcx>> {
let place_builder = unpack!(block = self.as_place_builder(block, expr_id));
let place_builder = self.as_place_builder(block, expr_id).unpack(&mut block);
block.and(place_builder.to_place(self))
}

Expand All @@ -393,7 +393,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
mut block: BasicBlock,
expr_id: ExprId,
) -> BlockAnd<Place<'tcx>> {
let place_builder = unpack!(block = self.as_read_only_place_builder(block, expr_id));
let place_builder = self.as_read_only_place_builder(block, expr_id).unpack(&mut block);
block.and(place_builder.to_place(self))
}

Expand Down Expand Up @@ -432,8 +432,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
ExprKind::Field { lhs, variant_index, name } => {
let lhs_expr = &this.thir[lhs];
let mut place_builder =
unpack!(block = this.expr_as_place(block, lhs, mutability, fake_borrow_temps,));
let mut place_builder = this
.expr_as_place(block, lhs, mutability, fake_borrow_temps)
.unpack(&mut block);
if let ty::Adt(adt_def, _) = lhs_expr.ty.kind() {
if adt_def.is_enum() {
place_builder = place_builder.downcast(*adt_def, variant_index);
Expand All @@ -442,8 +443,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block.and(place_builder.field(name, expr.ty))
}
ExprKind::Deref { arg } => {
let place_builder =
unpack!(block = this.expr_as_place(block, arg, mutability, fake_borrow_temps,));
let place_builder = this
.expr_as_place(block, arg, mutability, fake_borrow_temps)
.unpack(&mut block);
block.and(place_builder.deref())
}
ExprKind::Index { lhs, index } => this.lower_index_expression(
Expand Down Expand Up @@ -472,9 +474,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}

ExprKind::PlaceTypeAscription { source, ref user_ty } => {
let place_builder = unpack!(
block = this.expr_as_place(block, source, mutability, fake_borrow_temps,)
);
let place_builder = this
.expr_as_place(block, source, mutability, fake_borrow_temps)
.unpack(&mut block);
if let Some(user_ty) = user_ty {
let annotation_index =
this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation {
Expand Down Expand Up @@ -502,9 +504,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
ExprKind::ValueTypeAscription { source, ref user_ty } => {
let source_expr = &this.thir[source];
let temp = unpack!(
block = this.as_temp(block, source_expr.temp_lifetime, source, mutability)
);
let temp = this
.as_temp(block, source_expr.temp_lifetime, source, mutability)
.unpack(&mut block);
if let Some(user_ty) = user_ty {
let annotation_index =
this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation {
Expand Down Expand Up @@ -570,7 +572,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// these are not places, so we need to make a temporary.
debug_assert!(!matches!(Category::of(&expr.kind), Some(Category::Place)));
let temp =
unpack!(block = this.as_temp(block, expr.temp_lifetime, expr_id, mutability));
this.as_temp(block, expr.temp_lifetime, expr_id, mutability).unpack(&mut block);
block.and(PlaceBuilder::from(temp))
}
}
Expand Down Expand Up @@ -612,12 +614,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fake_borrow_temps = fake_borrow_temps.unwrap_or(base_fake_borrow_temps);

let base_place =
unpack!(block = self.expr_as_place(block, base, mutability, Some(fake_borrow_temps),));
self.expr_as_place(block, base, mutability, Some(fake_borrow_temps)).unpack(&mut block);

// Making this a *fresh* temporary means we do not have to worry about
// the index changing later: Nothing will ever change this temporary.
// The "retagging" transformation (for Stacked Borrows) relies on this.
let idx = unpack!(block = self.as_temp(block, temp_lifetime, index, Mutability::Not));
let idx = self.as_temp(block, temp_lifetime, index, Mutability::Not).unpack(&mut block);

block = self.bounds_check(block, &base_place, idx, expr_span, source_info);

Expand Down
Loading
Loading