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

Jump to bottom

Do not clone the Body for ConstProp #115797

Merged
merged 5 commits into from
Sep 13, 2023
Merged

Do not clone the Body for ConstProp #115797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
054ed8e
Introduce Machine::POST_MONO_CHECKS.
cjgillot Sep 12, 2023
8a3a0dd
Store a ConstantKind in CollectAndPatch.
cjgillot Sep 12, 2023
32fe00a
Reuse CollectAndPatch for normal ConstProp.
cjgillot Aug 3, 2023
108dece
Do not clone body for ConstProp.
cjgillot Aug 3, 2023
d0cba3d
Do not fetch type to check generator.
cjgillot Aug 3, 2023
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
10 changes: 6 additions & 4 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,10 +749,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.stack_mut().push(frame);

// Make sure all the constants required by this frame evaluate successfully (post-monomorphization check).
for ct in &body.required_consts {
let span = ct.span;
let ct = self.subst_from_current_frame_and_normalize_erasing_regions(ct.literal)?;
self.eval_mir_constant(&ct, Some(span), None)?;
if M::POST_MONO_CHECKS {
for ct in &body.required_consts {
let span = ct.span;
let ct = self.subst_from_current_frame_and_normalize_erasing_regions(ct.literal)?;
self.eval_mir_constant(&ct, Some(span), None)?;
}
}

// done
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_const_eval/src/interpret/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,9 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
/// Should the machine panic on allocation failures?
const PANIC_ON_ALLOC_FAIL: bool;

/// Should post-monomorphization checks be run when a stack frame is pushed?
const POST_MONO_CHECKS: bool = true;

/// Whether memory accesses should be alignment-checked.
fn enforce_alignment(ecx: &InterpCx<'mir, 'tcx, Self>) -> CheckAlignment;

Expand Down
114 changes: 37 additions & 77 deletions compiler/rustc_mir_transform/src/const_prop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@ use rustc_middle::mir::visit::{
use rustc_middle::mir::*;
use rustc_middle::ty::layout::{LayoutError, LayoutOf, LayoutOfHelpers, TyAndLayout};
use rustc_middle::ty::{self, GenericArgs, Instance, ParamEnv, Ty, TyCtxt, TypeVisitableExt};
use rustc_span::{def_id::DefId, Span, DUMMY_SP};
use rustc_span::{def_id::DefId, Span};
use rustc_target::abi::{self, Align, HasDataLayout, Size, TargetDataLayout};
use rustc_target::spec::abi::Abi as CallAbi;

use crate::dataflow_const_prop::Patch;
use crate::MirPass;
use rustc_const_eval::interpret::{
self, compile_time_machine, AllocId, ConstAllocation, ConstValue, FnArg, Frame, ImmTy,
Expand Down Expand Up @@ -83,43 +84,32 @@ impl<'tcx> MirPass<'tcx> for ConstProp {
return;
}

let is_generator = tcx.type_of(def_id.to_def_id()).instantiate_identity().is_generator();
// FIXME(welseywiser) const prop doesn't work on generators because of query cycles
// computing their layout.
let is_generator = def_kind == DefKind::Generator;
if is_generator {
trace!("ConstProp skipped for generator {:?}", def_id);
return;
}

trace!("ConstProp starting for {:?}", def_id);

let dummy_body = &Body::new(
body.source,
(*body.basic_blocks).to_owned(),
body.source_scopes.clone(),
body.local_decls.clone(),
Default::default(),
body.arg_count,
Default::default(),
body.span,
body.generator_kind(),
body.tainted_by_errors,
);

// FIXME(oli-obk, eddyb) Optimize locals (or even local paths) to hold
// constants, instead of just checking for const-folding succeeding.
// That would require a uniform one-def no-mutation analysis
// and RPO (or recursing when needing the value of a local).
let mut optimization_finder = ConstPropagator::new(body, dummy_body, tcx);
let mut optimization_finder = ConstPropagator::new(body, tcx);

// Traverse the body in reverse post-order, to ensure that `FullConstProp` locals are
// assigned before being read.
let rpo = body.basic_blocks.reverse_postorder().to_vec();
for bb in rpo {
let data = &mut body.basic_blocks.as_mut_preserves_cfg()[bb];
for &bb in body.basic_blocks.reverse_postorder() {
let data = &body.basic_blocks[bb];
optimization_finder.visit_basic_block_data(bb, data);
}

let mut patch = optimization_finder.patch;
patch.visit_body_preserves_cfg(body);

trace!("ConstProp done for {:?}", def_id);
}
}
Expand All @@ -143,8 +133,11 @@ impl ConstPropMachine<'_, '_> {

impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx> {
compile_time_machine!(<'mir, 'tcx>);

const PANIC_ON_ALLOC_FAIL: bool = true; // all allocations are small (see `MAX_ALLOC_LIMIT`)

const POST_MONO_CHECKS: bool = false; // this MIR is still generic!
Copy link
Member

Choose a reason for hiding this comment

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

Turns out this is not the entire reason, with const-generics we (try to) run regular CTFE on generic MIR as well.

It's just, we really don't need the post-mono checks here since we're not actually doing evaluation.


type MemoryKind = !;

#[inline(always)]
Expand Down Expand Up @@ -299,6 +292,7 @@ struct ConstPropagator<'mir, 'tcx> {
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
local_decls: &'mir IndexSlice<Local, LocalDecl<'tcx>>,
patch: Patch<'tcx>,
}

impl<'tcx> LayoutOfHelpers<'tcx> for ConstPropagator<'_, 'tcx> {
Expand Down Expand Up @@ -332,11 +326,7 @@ impl<'tcx> ty::layout::HasParamEnv<'tcx> for ConstPropagator<'_, 'tcx> {
}

impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
fn new(
body: &Body<'tcx>,
dummy_body: &'mir Body<'tcx>,
tcx: TyCtxt<'tcx>,
) -> ConstPropagator<'mir, 'tcx> {
fn new(body: &'mir Body<'tcx>, tcx: TyCtxt<'tcx>) -> ConstPropagator<'mir, 'tcx> {
let def_id = body.source.def_id();
let args = &GenericArgs::identity_for_item(tcx, def_id);
let param_env = tcx.param_env_reveal_all_normalized(def_id);
Expand Down Expand Up @@ -367,7 +357,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {

ecx.push_stack_frame(
Instance::new(def_id, args),
dummy_body,
body,
&ret,
StackPopCleanup::Root { cleanup: false },
)
Expand All @@ -382,7 +372,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
ecx.frame_mut().locals[local].make_live_uninit();
}

ConstPropagator { ecx, tcx, param_env, local_decls: &dummy_body.local_decls }
let patch = Patch::new(tcx);
ConstPropagator { ecx, tcx, param_env, local_decls: &body.local_decls, patch }
}

fn get_const(&self, place: Place<'tcx>) -> Option<OpTy<'tcx>> {
Expand Down Expand Up @@ -419,12 +410,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
ecx.machine.written_only_inside_own_block_locals.remove(&local);
}

fn propagate_operand(&mut self, operand: &mut Operand<'tcx>) {
if let Some(place) = operand.place() && let Some(op) = self.replace_with_const(place) {
*operand = op;
}
}

fn check_rvalue(&mut self, rvalue: &Rvalue<'tcx>) -> Option<()> {
// Perform any special handling for specific Rvalue types.
// Generally, checks here fall into one of two categories:
Expand Down Expand Up @@ -540,16 +525,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
}
}

/// Creates a new `Operand::Constant` from a `Scalar` value
fn operand_from_scalar(&self, scalar: Scalar, ty: Ty<'tcx>) -> Operand<'tcx> {
Operand::Constant(Box::new(Constant {
span: DUMMY_SP,
user_ty: None,
literal: ConstantKind::from_scalar(self.tcx, scalar, ty),
}))
}

fn replace_with_const(&mut self, place: Place<'tcx>) -> Option<Operand<'tcx>> {
fn replace_with_const(&mut self, place: Place<'tcx>) -> Option<ConstantKind<'tcx>> {
// This will return None if the above `const_prop` invocation only "wrote" a
// type whose creation requires no write. E.g. a generator whose initial state
// consists solely of uninitialized memory (so it doesn't capture any locals).
Expand All @@ -565,7 +541,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
let Right(imm) = imm else { return None };
match *imm {
Immediate::Scalar(scalar) if scalar.try_to_int().is_ok() => {
Some(self.operand_from_scalar(scalar, value.layout.ty))
Some(ConstantKind::from_scalar(self.tcx, scalar, value.layout.ty))
}
Immediate::ScalarPair(l, r) if l.try_to_int().is_ok() && r.try_to_int().is_ok() => {
let alloc = self
Expand All @@ -575,15 +551,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
})
.ok()?;

let literal = ConstantKind::Val(
Some(ConstantKind::Val(
ConstValue::ByRef { alloc, offset: Size::ZERO },
value.layout.ty,
);
Some(Operand::Constant(Box::new(Constant {
span: DUMMY_SP,
user_ty: None,
literal,
})))
))
}
// Scalars or scalar pairs that contain undef values are assumed to not have
// successfully evaluated and are thus not propagated.
Expand Down Expand Up @@ -725,40 +696,29 @@ impl<'tcx> Visitor<'tcx> for CanConstProp {
}
}

impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> {
fn tcx(&self) -> TyCtxt<'tcx> {
self.tcx
}

fn visit_operand(&mut self, operand: &mut Operand<'tcx>, location: Location) {
impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> {
fn visit_operand(&mut self, operand: &Operand<'tcx>, location: Location) {
self.super_operand(operand, location);
self.propagate_operand(operand)
if let Some(place) = operand.place() && let Some(value) = self.replace_with_const(place) {
self.patch.before_effect.insert((location, place), value);
}
}

fn process_projection_elem(
fn visit_projection_elem(
&mut self,
_: PlaceRef<'tcx>,
elem: PlaceElem<'tcx>,
_: Location,
) -> Option<PlaceElem<'tcx>> {
_: PlaceContext,
location: Location,
) {
if let PlaceElem::Index(local) = elem
&& let Some(value) = self.get_const(local.into())
&& let Some(imm) = value.as_mplace_or_imm().right()
&& let Immediate::Scalar(scalar) = *imm
&& let Ok(offset) = scalar.to_target_usize(&self.tcx)
&& let Some(min_length) = offset.checked_add(1)
&& let Some(value) = self.replace_with_const(local.into())
{
Some(PlaceElem::ConstantIndex { offset, min_length, from_end: false })
} else {
None
self.patch.before_effect.insert((location, local.into()), value);
}
}

fn visit_assign(
&mut self,
place: &mut Place<'tcx>,
rvalue: &mut Rvalue<'tcx>,
location: Location,
) {
fn visit_assign(&mut self, place: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) {
self.super_assign(place, rvalue, location);

let Some(()) = self.check_rvalue(rvalue) else { return };
Expand All @@ -775,7 +735,7 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> {
{
trace!("skipping replace of Rvalue::Use({:?} because it is already a const", c);
} else if let Some(operand) = self.replace_with_const(*place) {
*rvalue = Rvalue::Use(operand);
self.patch.assignments.insert(location, operand);
}
} else {
// Const prop failed, so erase the destination, ensuring that whatever happens
Expand All @@ -799,7 +759,7 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> {
}
}

fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Location) {
fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
trace!("visit_statement: {:?}", statement);

// We want to evaluate operands before any change to the assigned-to value,
Expand Down Expand Up @@ -843,7 +803,7 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> {
}
}

fn visit_basic_block_data(&mut self, block: BasicBlock, data: &mut BasicBlockData<'tcx>) {
fn visit_basic_block_data(&mut self, block: BasicBlock, data: &BasicBlockData<'tcx>) {
self.super_basic_block_data(block, data);

// We remove all Locals which are restricted in propagation to their containing blocks and
Expand Down
Loading
Loading