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.

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

mir opt + codegen: handle subtyping #112307

Merged
merged 5 commits into from
Jun 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#![feature(if_let_guard)]
#![feature(int_roundings)]
#![feature(let_chains)]
#![feature(negative_impls)]
#![feature(never_type)]
#![feature(strict_provenance)]
#![feature(try_blocks)]
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1729,7 +1729,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
IndirectOperand(tmp, index) => {
let op = bx.load_operand(tmp);
tmp.storage_dead(bx);
self.locals[index] = LocalRef::Operand(op);
self.overwrite_local(index, LocalRef::Operand(op));
self.debug_introduce_local(bx, index);
}
DirectOperand(index) => {
Expand All @@ -1744,7 +1744,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
} else {
OperandRef::from_immediate_or_packed_pair(bx, llval, ret_abi.layout)
};
self.locals[index] = LocalRef::Operand(op);
self.overwrite_local(index, LocalRef::Operand(op));
self.debug_introduce_local(bx, index);
}
}
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_codegen_ssa/src/mir/debuginfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}

fn spill_operand_to_stack(
operand: &OperandRef<'tcx, Bx::Value>,
operand: OperandRef<'tcx, Bx::Value>,
name: Option<String>,
bx: &mut Bx,
) -> PlaceRef<'tcx, Bx::Value> {
Expand Down Expand Up @@ -375,7 +375,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
return;
}

Self::spill_operand_to_stack(operand, name, bx)
Self::spill_operand_to_stack(*operand, name, bx)
}

LocalRef::Place(place) => *place,
Expand Down Expand Up @@ -550,7 +550,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if let Ok(operand) = self.eval_mir_constant_to_operand(bx, &c) {
self.set_debug_loc(bx, var.source_info);
let base = Self::spill_operand_to_stack(
&operand,
operand,
Some(var.name.to_string()),
bx,
);
Expand Down
75 changes: 75 additions & 0 deletions compiler/rustc_codegen_ssa/src/mir/locals.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
//! Locals are in a private module as updating `LocalRef::Operand` has to
//! be careful wrt to subtyping. To deal with this we only allow updates by using
//! `FunctionCx::overwrite_local` which handles it automatically.
use crate::mir::{FunctionCx, LocalRef};
use crate::traits::BuilderMethods;
use rustc_index::IndexVec;
use rustc_middle::mir;
use rustc_middle::ty::print::with_no_trimmed_paths;
use std::ops::{Index, IndexMut};

pub(super) struct Locals<'tcx, V> {
values: IndexVec<mir::Local, LocalRef<'tcx, V>>,
}

impl<'tcx, V> Index<mir::Local> for Locals<'tcx, V> {
type Output = LocalRef<'tcx, V>;
#[inline]
fn index(&self, index: mir::Local) -> &LocalRef<'tcx, V> {
&self.values[index]
}
}

/// To mutate locals, use `FunctionCx::overwrite_local` instead.
impl<'tcx, V, Idx: ?Sized> !IndexMut<Idx> for Locals<'tcx, V> {}

impl<'tcx, V> Locals<'tcx, V> {
pub(super) fn empty() -> Locals<'tcx, V> {
Locals { values: IndexVec::default() }
}

pub(super) fn indices(&self) -> impl DoubleEndedIterator<Item = mir::Local> + Clone + 'tcx {
self.values.indices()
}
}

impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
pub(super) fn initialize_locals(&mut self, values: Vec<LocalRef<'tcx, Bx::Value>>) {
assert!(self.locals.values.is_empty());

for (local, value) in values.into_iter().enumerate() {
match value {
LocalRef::Place(_) | LocalRef::UnsizedPlace(_) | LocalRef::PendingOperand => (),
LocalRef::Operand(op) => {
let local = mir::Local::from_usize(local);
let expected_ty = self.monomorphize(self.mir.local_decls[local].ty);
assert_eq!(expected_ty, op.layout.ty, "unexpected initial operand type");
}
}

self.locals.values.push(value);
}
}

pub(super) fn overwrite_local(
&mut self,
local: mir::Local,
mut value: LocalRef<'tcx, Bx::Value>,
) {
match value {
LocalRef::Place(_) | LocalRef::UnsizedPlace(_) | LocalRef::PendingOperand => (),
LocalRef::Operand(ref mut op) => {
let local_ty = self.monomorphize(self.mir.local_decls[local].ty);
if local_ty != op.layout.ty {
// FIXME(#112651): This can be changed to an ICE afterwards.
debug!("updating type of operand due to subtyping");
with_no_trimmed_paths!(debug!(?op.layout.ty));
with_no_trimmed_paths!(debug!(?local_ty));
op.layout.ty = local_ty;
}
}
};
Copy link
Member

Choose a reason for hiding this comment

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

FWIW in the MIR interpreter we avoid the risk of this by not having the type of the local in our locals map to begin with: we store a IndexVec<mir::Local, LocalState<'tcx, Prov>>, where LocalState is basically just interpret::Operand. When we need the type we always go through body.local_decls.

So that might also be something codegen can do; storing the type in self.locals is redundant.


self.locals.values[local] = value;
}
}
43 changes: 20 additions & 23 deletions compiler/rustc_codegen_ssa/src/mir/mod.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,31 @@
use crate::base;
use crate::traits::*;
use rustc_index::bit_set::BitSet;
use rustc_index::IndexVec;
use rustc_middle::mir;
use rustc_middle::mir::interpret::ErrorHandled;
use rustc_middle::mir::traversal;
use rustc_middle::ty::layout::{FnAbiOf, HasTyCtxt, TyAndLayout};
use rustc_middle::ty::{self, Instance, Ty, TyCtxt, TypeFoldable, TypeVisitableExt};
use rustc_target::abi::call::{FnAbi, PassMode};

use std::iter;

use rustc_index::bit_set::BitSet;
use rustc_index::IndexVec;
mod analyze;
mod block;
pub mod constant;
pub mod coverageinfo;
pub mod debuginfo;
mod intrinsic;
mod locals;
pub mod operand;
pub mod place;
mod rvalue;
mod statement;

use self::debuginfo::{FunctionDebugContext, PerLocalVarDebugInfo};
use self::place::PlaceRef;
use rustc_middle::mir::traversal;

use self::operand::{OperandRef, OperandValue};
use self::place::PlaceRef;

// Used for tracking the state of generated basic blocks.
enum CachedLlbb<T> {
Expand Down Expand Up @@ -91,7 +101,7 @@ pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> {
///
/// Avoiding allocs can also be important for certain intrinsics,
/// notably `expect`.
locals: IndexVec<mir::Local, LocalRef<'tcx, Bx::Value>>,
locals: locals::Locals<'tcx, Bx::Value>,

/// All `VarDebugInfo` from the MIR body, partitioned by `Local`.
/// This is `None` if no var`#[non_exhaustive]`iable debuginfo/names are needed.
Expand Down Expand Up @@ -192,7 +202,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
cleanup_kinds,
landing_pads: IndexVec::from_elem(None, &mir.basic_blocks),
funclets: IndexVec::from_fn_n(|_| None, mir.basic_blocks.len()),
locals: IndexVec::new(),
locals: locals::Locals::empty(),
debug_context,
per_local_var_debug_info: None,
caller_location: None,
Expand Down Expand Up @@ -223,7 +233,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
let memory_locals = analyze::non_ssa_locals(&fx);

// Allocate variable and temp allocas
fx.locals = {
let local_values = {
let args = arg_local_refs(&mut start_bx, &mut fx, &memory_locals);

let mut allocate_local = |local| {
Expand Down Expand Up @@ -256,6 +266,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
.chain(mir.vars_and_temps_iter().map(allocate_local))
.collect()
};
fx.initialize_locals(local_values);

// Apply debuginfo to the newly allocated locals.
fx.debug_introduce_locals(&mut start_bx);
Expand Down Expand Up @@ -289,14 +300,13 @@ fn arg_local_refs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
.enumerate()
.map(|(arg_index, local)| {
let arg_decl = &mir.local_decls[local];
let arg_ty = fx.monomorphize(arg_decl.ty);

if Some(local) == mir.spread_arg {
// This argument (e.g., the last argument in the "rust-call" ABI)
// is a tuple that was spread at the ABI level and now we have
// to reconstruct it into a tuple local variable, from multiple
// individual LLVM function arguments.

let arg_ty = fx.monomorphize(arg_decl.ty);
let ty::Tuple(tupled_arg_tys) = arg_ty.kind() else {
bug!("spread argument isn't a tuple?!");
};
Expand Down Expand Up @@ -331,8 +341,6 @@ fn arg_local_refs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
}

if fx.fn_abi.c_variadic && arg_index == fx.fn_abi.args.len() {
let arg_ty = fx.monomorphize(arg_decl.ty);

let va_list = PlaceRef::alloca(bx, bx.layout_of(arg_ty));
bx.va_start(va_list.llval);

Expand Down Expand Up @@ -429,14 +437,3 @@ fn arg_local_refs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(

args
}

mod analyze;
mod block;
pub mod constant;
pub mod coverageinfo;
pub mod debuginfo;
mod intrinsic;
pub mod operand;
pub mod place;
mod rvalue;
mod statement;
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/mir/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
LocalRef::PendingOperand => {
let operand = self.codegen_rvalue_operand(bx, rvalue);
self.locals[index] = LocalRef::Operand(operand);
self.overwrite_local(index, LocalRef::Operand(operand));
self.debug_introduce_local(bx, index);
}
LocalRef::Operand(op) => {
Expand Down
19 changes: 17 additions & 2 deletions compiler/rustc_mir_transform/src/dest_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@
//! if they do not consistently refer to the same place in memory. This is satisfied if they do
//! not contain any indirection through a pointer or any indexing projections.
//!
//! * `p` and `q` must have the **same type**. If we replace a local with a subtype or supertype,
//! we may end up with a differnet vtable for that local. See the `subtyping-impacts-selection`
//! tests for an example where that causes issues.
//!
//! * We need to make sure that the goal of "merging the memory" is actually structurally possible
//! in MIR. For example, even if all the other conditions are satisfied, there is no way to
//! "merge" `_5.foo` and `_6.bar`. For now, we ensure this by requiring that both `p` and `q` are
Expand Down Expand Up @@ -134,6 +138,7 @@ use crate::MirPass;
use rustc_data_structures::fx::FxHashMap;
use rustc_index::bit_set::BitSet;
use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
use rustc_middle::mir::HasLocalDecls;
use rustc_middle::mir::{dump_mir, PassWhere};
use rustc_middle::mir::{
traversal, Body, InlineAsmOperand, Local, LocalKind, Location, Operand, Place, Rvalue,
Expand Down Expand Up @@ -763,12 +768,22 @@ impl<'tcx> Visitor<'tcx> for FindAssignments<'_, '_, 'tcx> {
return;
};

// As described at the top of the file, we do not go near things that have their address
// taken.
// As described at the top of the file, we do not go near things that have
// their address taken.
if self.borrowed.contains(src) || self.borrowed.contains(dest) {
return;
}

// As described at the top of this file, we do not touch locals which have
// different types.
let src_ty = self.body.local_decls()[src].ty;
let dest_ty = self.body.local_decls()[dest].ty;
if src_ty != dest_ty {
// FIXME(#112651): This can be removed afterwards. Also update the module description.
trace!("skipped `{src:?} = {dest:?}` due to subtyping: {src_ty} != {dest_ty}");
return;
}

// Also, we need to make sure that MIR actually allows the `src` to be removed
if is_local_required(src, self.body) {
return;
Expand Down
30 changes: 23 additions & 7 deletions compiler/rustc_mir_transform/src/shim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,15 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<'
let mut result = match instance {
ty::InstanceDef::Item(..) => bug!("item {:?} passed to make_shim", instance),
ty::InstanceDef::VTableShim(def_id) => {
build_call_shim(tcx, instance, Some(Adjustment::Deref), CallKind::Direct(def_id))
let adjustment = Adjustment::Deref { source: DerefSource::MutPtr };
build_call_shim(tcx, instance, Some(adjustment), CallKind::Direct(def_id))
}
ty::InstanceDef::FnPtrShim(def_id, ty) => {
let trait_ = tcx.trait_of_item(def_id).unwrap();
let adjustment = match tcx.fn_trait_kind_from_def_id(trait_) {
Some(ty::ClosureKind::FnOnce) => Adjustment::Identity,
Some(ty::ClosureKind::FnMut | ty::ClosureKind::Fn) => Adjustment::Deref,
Some(ty::ClosureKind::Fn) => Adjustment::Deref { source: DerefSource::ImmRef },
Some(ty::ClosureKind::FnMut) => Adjustment::Deref { source: DerefSource::MutRef },
None => bug!("fn pointer {:?} is not an fn", ty),
};

Expand Down Expand Up @@ -107,16 +109,26 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<'
result
}

#[derive(Copy, Clone, Debug, PartialEq)]
enum DerefSource {
/// `fn shim(&self) { inner(*self )}`.
ImmRef,
/// `fn shim(&mut self) { inner(*self )}`.
MutRef,
/// `fn shim(*mut self) { inner(*self )}`.
MutPtr,
}

#[derive(Copy, Clone, Debug, PartialEq)]
enum Adjustment {
/// Pass the receiver as-is.
Identity,

/// We get passed `&[mut] self` and call the target with `*self`.
/// We get passed a reference or a raw pointer to `self` and call the target with `*self`.
///
/// This either copies `self` (if `Self: Copy`, eg. for function items), or moves out of it
/// (for `VTableShim`, which effectively is passed `&own Self`).
Deref,
Deref { source: DerefSource },

/// We get passed `self: Self` and call the target with `&mut self`.
///
Expand Down Expand Up @@ -667,8 +679,12 @@ fn build_call_shim<'tcx>(
let self_arg = &mut inputs_and_output[0];
*self_arg = match rcvr_adjustment.unwrap() {
Adjustment::Identity => fnty,
Adjustment::Deref => tcx.mk_imm_ptr(fnty),
Adjustment::RefMut => tcx.mk_mut_ptr(fnty),
Adjustment::Deref { source } => match source {
DerefSource::ImmRef => tcx.mk_imm_ref(tcx.lifetimes.re_erased, fnty),
DerefSource::MutRef => tcx.mk_mut_ref(tcx.lifetimes.re_erased, fnty),
DerefSource::MutPtr => tcx.mk_mut_ptr(fnty),
},
Adjustment::RefMut => bug!("`RefMut` is never used with indirect calls: {instance:?}"),
};
sig.inputs_and_output = tcx.mk_type_list(&inputs_and_output);
}
Expand Down Expand Up @@ -699,7 +715,7 @@ fn build_call_shim<'tcx>(

let rcvr = rcvr_adjustment.map(|rcvr_adjustment| match rcvr_adjustment {
Adjustment::Identity => Operand::Move(rcvr_place()),
Adjustment::Deref => Operand::Move(tcx.mk_place_deref(rcvr_place())),
Adjustment::Deref { source: _ } => Operand::Move(tcx.mk_place_deref(rcvr_place())),
Adjustment::RefMut => {
// let rcvr = &mut rcvr;
let ref_rcvr = local_decls.push(
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_mir_transform/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,14 @@ fn compute_copy_classes(ssa: &mut SsaLocals, body: &Body<'_>) {
else { continue };

let Some(rhs) = place.as_local() else { continue };
let local_ty = body.local_decls()[local].ty;
let rhs_ty = body.local_decls()[rhs].ty;
if local_ty != rhs_ty {
// FIXME(#112651): This can be removed afterwards.
trace!("skipped `{local:?} = {rhs:?}` due to subtyping: {local_ty} != {rhs_ty}");
continue;
}

if !ssa.is_ssa(rhs) {
continue;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// MIR for `std::ops::Fn::call` before AddMovesForPackedDrops

fn std::ops::Fn::call(_1: *const fn(), _2: ()) -> <fn() as FnOnce<()>>::Output {
fn std::ops::Fn::call(_1: &fn(), _2: ()) -> <fn() as FnOnce<()>>::Output {
let mut _0: <fn() as std::ops::FnOnce<()>>::Output;

bb0: {
Expand Down
Loading