Skip to content

Commit

Permalink
Rollup merge of #128494 - RalfJung:mir-lazy-lists, r=compiler-errors
Browse files Browse the repository at this point in the history
MIR required_consts, mentioned_items: ensure we do not forget to fill these lists

Bodies initially get created with empty required_consts and mentioned_items, but at some point those should be filled. Make sure we notice when that is forgotten.
  • Loading branch information
tgross35 authored Aug 1, 2024
2 parents 7a7ca4e + 6d312d7 commit 8844e08
Show file tree
Hide file tree
Showing 11 changed files with 113 additions and 44 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -888,7 +888,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
body: &'tcx mir::Body<'tcx>,
) -> InterpResult<'tcx> {
// Make sure all the constants required by this frame evaluate successfully (post-monomorphization check).
for &const_ in &body.required_consts {
for &const_ in body.required_consts() {
let c =
self.instantiate_from_current_frame_and_normalize_erasing_regions(const_.const_)?;
c.eval(*self.tcx, self.param_env, const_.span).map_err(|err| {
Expand Down
48 changes: 42 additions & 6 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -383,23 +383,25 @@ pub struct Body<'tcx> {

/// Constants that are required to evaluate successfully for this MIR to be well-formed.
/// We hold in this field all the constants we are not able to evaluate yet.
/// `None` indicates that the list has not been computed yet.
///
/// This is soundness-critical, we make a guarantee that all consts syntactically mentioned in a
/// function have successfully evaluated if the function ever gets executed at runtime.
pub required_consts: Vec<ConstOperand<'tcx>>,
pub required_consts: Option<Vec<ConstOperand<'tcx>>>,

/// Further items that were mentioned in this function and hence *may* become monomorphized,
/// depending on optimizations. We use this to avoid optimization-dependent compile errors: the
/// collector recursively traverses all "mentioned" items and evaluates all their
/// `required_consts`.
/// `None` indicates that the list has not been computed yet.
///
/// This is *not* soundness-critical and the contents of this list are *not* a stable guarantee.
/// All that's relevant is that this set is optimization-level-independent, and that it includes
/// everything that the collector would consider "used". (For example, we currently compute this
/// set after drop elaboration, so some drop calls that can never be reached are not considered
/// "mentioned".) See the documentation of `CollectionMode` in
/// `compiler/rustc_monomorphize/src/collector.rs` for more context.
pub mentioned_items: Vec<Spanned<MentionedItem<'tcx>>>,
pub mentioned_items: Option<Vec<Spanned<MentionedItem<'tcx>>>>,

/// Does this body use generic parameters. This is used for the `ConstEvaluatable` check.
///
Expand Down Expand Up @@ -477,8 +479,8 @@ impl<'tcx> Body<'tcx> {
spread_arg: None,
var_debug_info,
span,
required_consts: Vec::new(),
mentioned_items: Vec::new(),
required_consts: None,
mentioned_items: None,
is_polymorphic: false,
injection_phase: None,
tainted_by_errors,
Expand Down Expand Up @@ -507,8 +509,8 @@ impl<'tcx> Body<'tcx> {
arg_count: 0,
spread_arg: None,
span: DUMMY_SP,
required_consts: Vec::new(),
mentioned_items: Vec::new(),
required_consts: None,
mentioned_items: None,
var_debug_info: Vec::new(),
is_polymorphic: false,
injection_phase: None,
Expand Down Expand Up @@ -785,6 +787,40 @@ impl<'tcx> Body<'tcx> {
// No inlined `SourceScope`s, or all of them were `#[track_caller]`.
caller_location.unwrap_or_else(|| from_span(source_info.span))
}

#[track_caller]
pub fn set_required_consts(&mut self, required_consts: Vec<ConstOperand<'tcx>>) {
assert!(
self.required_consts.is_none(),
"required_consts for {:?} have already been set",
self.source.def_id()
);
self.required_consts = Some(required_consts);
}
#[track_caller]
pub fn required_consts(&self) -> &[ConstOperand<'tcx>] {
match &self.required_consts {
Some(l) => l,
None => panic!("required_consts for {:?} have not yet been set", self.source.def_id()),
}
}

#[track_caller]
pub fn set_mentioned_items(&mut self, mentioned_items: Vec<Spanned<MentionedItem<'tcx>>>) {
assert!(
self.mentioned_items.is_none(),
"mentioned_items for {:?} have already been set",
self.source.def_id()
);
self.mentioned_items = Some(mentioned_items);
}
#[track_caller]
pub fn mentioned_items(&self) -> &[Spanned<MentionedItem<'tcx>>] {
match &self.mentioned_items {
Some(l) => l,
None => panic!("mentioned_items for {:?} have not yet been set", self.source.def_id()),
}
}
}

impl<'tcx> Index<BasicBlock> for Body<'tcx> {
Expand Down
8 changes: 5 additions & 3 deletions compiler/rustc_middle/src/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1066,9 +1066,11 @@ macro_rules! super_body {

$self.visit_span($(& $mutability)? $body.span);

for const_ in &$($mutability)? $body.required_consts {
let location = Location::START;
$self.visit_const_operand(const_, location);
if let Some(required_consts) = &$($mutability)? $body.required_consts {
for const_ in required_consts {
let location = Location::START;
$self.visit_const_operand(const_, location);
}
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_build/src/build/custom/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ pub(super) fn build_custom_mir<'tcx>(
spread_arg: None,
var_debug_info: Vec::new(),
span,
required_consts: Vec::new(),
mentioned_items: Vec::new(),
required_consts: None,
mentioned_items: None,
is_polymorphic: false,
tainted_by_errors: None,
injection_phase: None,
Expand Down
13 changes: 6 additions & 7 deletions compiler/rustc_mir_transform/src/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -744,8 +744,8 @@ impl<'tcx> Inliner<'tcx> {
// Copy required constants from the callee_body into the caller_body. Although we are only
// pushing unevaluated consts to `required_consts`, here they may have been evaluated
// because we are calling `instantiate_and_normalize_erasing_regions` -- so we filter again.
caller_body.required_consts.extend(
callee_body.required_consts.into_iter().filter(|ct| ct.const_.is_required_const()),
caller_body.required_consts.as_mut().unwrap().extend(
callee_body.required_consts().into_iter().filter(|ct| ct.const_.is_required_const()),
);
// Now that we incorporated the callee's `required_consts`, we can remove the callee from
// `mentioned_items` -- but we have to take their `mentioned_items` in return. This does
Expand All @@ -755,12 +755,11 @@ impl<'tcx> Inliner<'tcx> {
// We need to reconstruct the `required_item` for the callee so that we can find and
// remove it.
let callee_item = MentionedItem::Fn(func.ty(caller_body, self.tcx));
if let Some(idx) =
caller_body.mentioned_items.iter().position(|item| item.node == callee_item)
{
let caller_mentioned_items = caller_body.mentioned_items.as_mut().unwrap();
if let Some(idx) = caller_mentioned_items.iter().position(|item| item.node == callee_item) {
// We found the callee, so remove it and add its items instead.
caller_body.mentioned_items.remove(idx);
caller_body.mentioned_items.extend(callee_body.mentioned_items);
caller_mentioned_items.remove(idx);
caller_mentioned_items.extend(callee_body.mentioned_items());
} else {
// If we can't find the callee, there's no point in adding its items. Probably it
// already got removed by being inlined elsewhere in the same function, so we already
Expand Down
36 changes: 24 additions & 12 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,10 @@ use rustc_hir::def::DefKind;
use rustc_hir::def_id::LocalDefId;
use rustc_hir::intravisit::{self, Visitor};
use rustc_index::IndexVec;
use rustc_middle::mir::visit::Visitor as _;
use rustc_middle::mir::{
traversal, AnalysisPhase, Body, CallSource, ClearCrossCrate, ConstOperand, ConstQualifs,
LocalDecl, MirPass, MirPhase, Operand, Place, ProjectionElem, Promoted, RuntimePhase, Rvalue,
SourceInfo, Statement, StatementKind, TerminatorKind, START_BLOCK,
AnalysisPhase, Body, CallSource, ClearCrossCrate, ConstOperand, ConstQualifs, LocalDecl,
MirPass, MirPhase, Operand, Place, ProjectionElem, Promoted, RuntimePhase, Rvalue, SourceInfo,
Statement, StatementKind, TerminatorKind, START_BLOCK,
};
use rustc_middle::ty::{self, TyCtxt, TypeVisitableExt};
use rustc_middle::util::Providers;
Expand Down Expand Up @@ -339,12 +338,15 @@ fn mir_promoted(

// Collect `required_consts` *before* promotion, so if there are any consts being promoted
// we still add them to the list in the outer MIR body.
let mut required_consts = Vec::new();
let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts);
for (bb, bb_data) in traversal::reverse_postorder(&body) {
required_consts_visitor.visit_basic_block_data(bb, bb_data);
RequiredConstsVisitor::compute_required_consts(&mut body);
// If this has an associated by-move async closure body, that doesn't get run through these
// passes itself, it gets "tagged along" by the pass manager. `RequiredConstsVisitor` is not
// a regular pass so we have to also apply it manually to the other body.
if let Some(coroutine) = body.coroutine.as_mut() {
if let Some(by_move_body) = coroutine.by_move_body.as_mut() {
RequiredConstsVisitor::compute_required_consts(by_move_body);
}
}
body.required_consts = required_consts;

// What we need to run borrowck etc.
let promote_pass = promote_consts::PromoteTemps::default();
Expand Down Expand Up @@ -561,9 +563,6 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
tcx,
body,
&[
// Before doing anything, remember which items are being mentioned so that the set of items
// visited does not depend on the optimization level.
&mentioned_items::MentionedItems,
// Add some UB checks before any UB gets optimized away.
&check_alignment::CheckAlignment,
// Before inlining: trim down MIR with passes to reduce inlining work.
Expand Down Expand Up @@ -657,6 +656,19 @@ fn inner_optimized_mir(tcx: TyCtxt<'_>, did: LocalDefId) -> Body<'_> {
return body;
}

// Before doing anything, remember which items are being mentioned so that the set of items
// visited does not depend on the optimization level.
// We do not use `run_passes` for this as that might skip the pass if `injection_phase` is set.
mentioned_items::MentionedItems.run_pass(tcx, &mut body);
// If this has an associated by-move async closure body, that doesn't get run through these
// passes itself, it gets "tagged along" by the pass manager. Since we're not using the pass
// manager we have to do this by hand.
if let Some(coroutine) = body.coroutine.as_mut() {
if let Some(by_move_body) = coroutine.by_move_body.as_mut() {
mentioned_items::MentionedItems.run_pass(tcx, by_move_body);
}
}

// If `mir_drops_elaborated_and_const_checked` found that the current body has unsatisfiable
// predicates, it will shrink the MIR to a single `unreachable` terminator.
// More generally, if MIR is a lone `unreachable`, there is nothing to optimize.
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_mir_transform/src/mentioned_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,9 @@ impl<'tcx> MirPass<'tcx> for MentionedItems {
}

fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut mir::Body<'tcx>) {
debug_assert!(body.mentioned_items.is_empty());
let mut mentioned_items = Vec::new();
MentionedItemsVisitor { tcx, body, mentioned_items: &mut mentioned_items }.visit_body(body);
body.mentioned_items = mentioned_items;
body.set_mentioned_items(mentioned_items);
}
}

Expand Down
14 changes: 10 additions & 4 deletions compiler/rustc_mir_transform/src/promote_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,9 @@ struct Promoter<'a, 'tcx> {
temps: &'a mut IndexVec<Local, TempState>,
extra_statements: &'a mut Vec<(Location, Statement<'tcx>)>,

/// Used to assemble the required_consts list while building the promoted.
required_consts: Vec<ConstOperand<'tcx>>,

/// If true, all nested temps are also kept in the
/// source MIR, not moved to the promoted MIR.
keep_original: bool,
Expand Down Expand Up @@ -924,11 +927,14 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> {
let span = self.promoted.span;
self.assign(RETURN_PLACE, rvalue, span);

// Now that we did promotion, we know whether we'll want to add this to `required_consts`.
// Now that we did promotion, we know whether we'll want to add this to `required_consts` of
// the surrounding MIR body.
if self.add_to_required {
self.source.required_consts.push(promoted_op);
self.source.required_consts.as_mut().unwrap().push(promoted_op);
}

self.promoted.set_required_consts(self.required_consts);

self.promoted
}
}
Expand All @@ -947,7 +953,7 @@ impl<'a, 'tcx> MutVisitor<'tcx> for Promoter<'a, 'tcx> {

fn visit_const_operand(&mut self, constant: &mut ConstOperand<'tcx>, _location: Location) {
if constant.const_.is_required_const() {
self.promoted.required_consts.push(*constant);
self.required_consts.push(*constant);
}

// Skipping `super_constant` as the visitor is otherwise only looking for locals.
Expand Down Expand Up @@ -1011,9 +1017,9 @@ fn promote_candidates<'tcx>(
extra_statements: &mut extra_statements,
keep_original: false,
add_to_required: false,
required_consts: Vec::new(),
};

// `required_consts` of the promoted itself gets filled while building the MIR body.
let mut promoted = promoter.promote_candidate(candidate, promotions.len());
promoted.source.promoted = Some(promotions.next_index());
promotions.push(promoted);
Expand Down
13 changes: 11 additions & 2 deletions compiler/rustc_mir_transform/src/required_consts.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::{ConstOperand, Location};
use rustc_middle::mir::{traversal, Body, ConstOperand, Location};

pub struct RequiredConstsVisitor<'a, 'tcx> {
required_consts: &'a mut Vec<ConstOperand<'tcx>>,
}

impl<'a, 'tcx> RequiredConstsVisitor<'a, 'tcx> {
pub fn new(required_consts: &'a mut Vec<ConstOperand<'tcx>>) -> Self {
fn new(required_consts: &'a mut Vec<ConstOperand<'tcx>>) -> Self {
RequiredConstsVisitor { required_consts }
}

pub fn compute_required_consts(body: &mut Body<'tcx>) {
let mut required_consts = Vec::new();
let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts);
for (bb, bb_data) in traversal::reverse_postorder(&body) {
required_consts_visitor.visit_basic_block_data(bb, bb_data);
}
body.set_required_consts(required_consts);
}
}

impl<'tcx> Visitor<'tcx> for RequiredConstsVisitor<'_, 'tcx> {
Expand Down
12 changes: 9 additions & 3 deletions compiler/rustc_mir_transform/src/shim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ fn new_body<'tcx>(
arg_count: usize,
span: Span,
) -> Body<'tcx> {
Body::new(
let mut body = Body::new(
source,
basic_blocks,
IndexVec::from_elem_n(
Expand All @@ -326,7 +326,10 @@ fn new_body<'tcx>(
None,
// FIXME(compiler-errors): is this correct?
None,
)
);
// Shims do not directly mention any consts.
body.set_required_consts(Vec::new());
body
}

pub struct DropShimElaborator<'a, 'tcx> {
Expand Down Expand Up @@ -969,13 +972,16 @@ pub fn build_adt_ctor(tcx: TyCtxt<'_>, ctor_id: DefId) -> Body<'_> {
};

let source = MirSource::item(ctor_id);
let body = new_body(
let mut body = new_body(
source,
IndexVec::from_elem_n(start_block, 1),
local_decls,
sig.inputs().len(),
span,
);
// A constructor doesn't mention any other items (and we don't run the usual optimization passes
// so this would otherwise not get filled).
body.set_mentioned_items(Vec::new());

crate::pass_manager::dump_mir_for_phase_change(tcx, &body);

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_monomorphize/src/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1229,15 +1229,15 @@ fn collect_items_of_instance<'tcx>(

// Always visit all `required_consts`, so that we evaluate them and abort compilation if any of
// them errors.
for const_op in &body.required_consts {
for const_op in body.required_consts() {
if let Some(val) = collector.eval_constant(const_op) {
collect_const_value(tcx, val, mentioned_items);
}
}

// Always gather mentioned items. We try to avoid processing items that we have already added to
// `used_items` above.
for item in &body.mentioned_items {
for item in body.mentioned_items() {
if !collector.used_mentioned_items.contains(&item.node) {
let item_mono = collector.monomorphize(item.node);
visit_mentioned_item(tcx, &item_mono, item.span, mentioned_items);
Expand Down

0 comments on commit 8844e08

Please sign in to comment.