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

Implement const propagation for the MIR Len operand #61081

Closed
wants to merge 1 commit into from
Closed

Implement const propagation for the MIR Len operand #61081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
93 changes: 76 additions & 17 deletions src/librustc_mir/transform/const_prop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
//! assertion failures

use rustc::hir::def::DefKind;
use rustc::hir::def_id::DefId;
use rustc::mir::{
AggregateKind, Constant, Location, Place, PlaceBase, Mir, Operand, Rvalue, Local,
NullOp, UnOp, StatementKind, Statement, LocalKind, Static, StaticKind,
Expand All @@ -17,8 +18,7 @@ use syntax_pos::{Span, DUMMY_SP};
use rustc::ty::subst::InternalSubsts;
use rustc_data_structures::indexed_vec::IndexVec;
use rustc::ty::layout::{
LayoutOf, TyLayout, LayoutError,
HasTyCtxt, TargetDataLayout, HasDataLayout,
LayoutOf, TyLayout, LayoutError, HasTyCtxt, TargetDataLayout, HasDataLayout, Size,
};

use crate::interpret::{self, InterpretCx, ScalarMaybeUndef, Immediate, OpTy, ImmTy, MemoryKind};
Expand Down Expand Up @@ -294,6 +294,26 @@ impl<'a, 'mir, 'tcx> ConstPropagator<'a, 'mir, 'tcx> {
}
}

fn get_global_id_for_static(
&self,
def_id: DefId,
promoted: Option<Promoted>,
) -> Option<GlobalId<'tcx>> {
let generics = self.tcx.generics_of(def_id);
if generics.requires_monomorphization(self.tcx) {
trace!("can't monomorphize");
// FIXME: can't handle code with generics
return None;
}
let substs = InternalSubsts::identity_for_item(self.tcx, def_id);
let instance = Instance::new(def_id, substs);

Some(GlobalId {
instance,
promoted,
})
}

fn eval_place(&mut self, place: &Place<'tcx>, source_info: SourceInfo) -> Option<Const<'tcx>> {
match *place {
Place::Base(PlaceBase::Local(loc)) => self.places[loc].clone(),
Expand All @@ -306,6 +326,14 @@ impl<'a, 'mir, 'tcx> ConstPropagator<'a, 'mir, 'tcx> {
})?;
Some(res)
},
ProjectionElem::Deref => {
let base = self.eval_place(&proj.base, source_info)?;
let res = self.use_ecx(source_info, |this| {
this.ecx.deref_operand(base)
})?;

Some(res.into())
},
// We could get more projections by using e.g., `operand_projection`,
// but we do not even have the stack frame set up properly so
// an `Index` projection would throw us off-track.
Expand All @@ -314,17 +342,7 @@ impl<'a, 'mir, 'tcx> ConstPropagator<'a, 'mir, 'tcx> {
Place::Base(
PlaceBase::Static(box Static {kind: StaticKind::Promoted(promoted), ..})
) => {
let generics = self.tcx.generics_of(self.source.def_id());
if generics.requires_monomorphization(self.tcx) {
// FIXME: can't handle code with generics
return None;
}
let substs = InternalSubsts::identity_for_item(self.tcx, self.source.def_id());
let instance = Instance::new(self.source.def_id(), substs);
let cid = GlobalId {
instance,
promoted: Some(promoted),
};
let cid = self.get_global_id_for_static(self.source.def_id(), Some(promoted))?;
// cannot use `const_eval` here, because that would require having the MIR
// for the current function available, but we're producing said MIR right now
let res = self.use_ecx(source_info, |this| {
Expand All @@ -334,7 +352,24 @@ impl<'a, 'mir, 'tcx> ConstPropagator<'a, 'mir, 'tcx> {
trace!("evaluated promoted {:?} to {:?}", promoted, res);
Some(res.into())
},
_ => None,
Place::Base(
PlaceBase::Static(box Static {kind: StaticKind::Static(def_id), .. })
) => {
// mutable statics can change value at any time so we can't const
// propagate their values
if self.tcx.is_mutable_static(def_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this also need to take internal mutability into account?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's even worse, the moment you encounter a union or raw pointer you need to give up. Is this change needed for this PR or could it be done in an extra PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

could it be done in an extra PR?

Possibly? I had some trouble getting a Len operand to appear in the MIR and this was necessary to get the current test case I came up with to pass. The obvious test case

let x= &[1, 2, 3];
x[1]

doesn't seem to generate a Len operand at all.

I'll look into this tonight but this might not be necessary to get this test case to work:

const TEST: &[u32] = &[1, 2, 3];

fn test() -> u32 {
    TEST[1]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You get one if you use let x: &[u8] = &[1, 2, 3];. Arrays don't generate Len Rvalues

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow. I didn't realize &[1, 2, 3] got inferred as a &[{integer}; 3] instead of a slice. That makes sense why my original test case didn't work then. A lot of these changes are probably not necessary then.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm... The original test wasn't ((&[1, 2, 3]) as &[u32])[1], because I didn't what the MIR after the &[1, 2, 3] to know about the actual array size. But that's not really useful I guess. Feel free to just change it to one big expression and scrap the rest of the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, my previous comment was kind of vague. So far, I haven't been able to find a test case that contains a Len operand that we can const propagate without the changes to handle statics. Here's what I've tried:

let x: &[u32] = &[1, 2, 3];
x[1];

This style test doesn't work at all because x is a local and we don't const propagate locals.

(&[1, 2, 3] as &[u32])[1];

This doesn't compile because it's a non-primitive cast.

static X: &[u32] = &[u32];

X[1];

This could maybe work but would require additional changes like those included in this PR to handle statics.

const X: &[u32] = &[1, 2, 3];

X[1];

This I believe will work once we have support in replace_with_const() for Operand::Indirect.

So, as it stands currently, I don't think there's anyway to write a test which will exercise this part of the const propagator.

Copy link
Contributor

Choose a reason for hiding this comment

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

The let binding case we can't do until we get some sort of dataflow analysis going

The static case scares me. I'm not sure we should be propagating statics, like ever. A static is something with a name. Reading from it seems like something we shouldn't optimize out, not sure how aggressive we can or want to be there. It's probably alright if the value being read is not from a static mut and the type has no interior mutability, but if we read beyond that (like from things with UnsafeCell or behind raw pointers) we have no guarantees. The problem is, once we read from a static, the propagated value has no notion of "I'm from a static" anymore. So we don't know anymore that we can't just read further values. Most likely not a problem for now, as propagating raw pointer derefs is its very own crazy topic that probably needs UGC discussions. But for now I'd rather not open this box.

The other two cases seem reasonable to me, pick either I guess? Though you may end up needing Indirect support for the cast case, too, so just go with the constant, which requires the least changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

The static case scares me. I'm not sure we should be propagating statics, like ever.

Yeah, I agree. There's so many edge cases here that it would be very complicated to ensure this was done correctly.

The let binding case we can't do until we get some sort of dataflow analysis going

Do we need full dataflow analysis or is there a simpler thing we could do now and eventually replace with dataflow? For example, I have a change I've been playing with that allows propagating locals which were themselves propagated into. This only is only at the in the ConstPropagator and not the CanConstProp pass so it doesn't affect error reporting. (See e699063 for my code)

Copy link
Contributor

@oli-obk oli-obk May 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

hmm... we'd also need to make sure to propagate neither from nor into locals that have more than one mutable usage. You don't want any const prop for let x = if y { 5 } else { 6 }; or for let mut x = 42; if y { x = 9; }

trace!("can't const-prop mutable static");
return None;
}

let cid = self.get_global_id_for_static(def_id, None)?;

let res = self.use_ecx(source_info, |this| {
this.ecx.const_eval_raw(cid)
})?;
trace!("evaluated static {:?} to {:?}", def_id, res);
Some(res.into())
},
}
}

Expand Down Expand Up @@ -370,10 +405,34 @@ impl<'a, 'mir, 'tcx> ConstPropagator<'a, 'mir, 'tcx> {
this.ecx.cast(op, kind, dest.into())?;
Ok(dest.into())
})
}
},
Rvalue::Len(ref place) => {
let place = self.eval_place(&place, source_info)?;
let mplace = place.to_mem_place();

// FIXME(oli-obk): evaluate static/constant slice lengths
Rvalue::Len(_) => None,
let is_slice =
if let ty::Slice(_) = mplace.layout.ty.sty { true }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is scary. Do we really encounter both &[T] and [T]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, after *&[T] is evaluated, we're left with a bare [T].

else { mplace.layout.ty.is_slice() };
Copy link
Contributor

Choose a reason for hiding this comment

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

this will also return true for *const [T], which we should probably not const prop for now


if is_slice {
let len = mplace.meta.unwrap().to_usize(&self.ecx).unwrap();

Some(ImmTy {
imm: Immediate::Scalar(
Scalar::from_uint(
len,
Size::from_bits(
self.tcx.sess.target.usize_ty.bit_width().unwrap() as u64
)
).into(),
),
layout: self.tcx.layout_of(self.param_env.and(self.tcx.types.usize)).ok()?,
}.into())
} else {
trace!("not slice: {:?}", mplace.layout.ty.sty);
None
}
},
Rvalue::NullaryOp(NullOp::SizeOf, ty) => {
type_size_of(self.tcx, self.param_env, ty).and_then(|n| Some(
ImmTy {
Expand Down
32 changes: 15 additions & 17 deletions src/test/mir-opt/const_prop/slice_len.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,34 @@
fn test() -> &'static [u32] {
&[1, 2]
}
static X: &[u32] = &[0, 1, 2];

fn main() {
let x = test()[0];
let x = X[1];
}

// END RUST SOURCE
// START rustc.main.ConstProp.before.mir
// bb1: {
// bb0: {
// ...
// _3 = const 0usize;
// _4 = Len((*_2));
// _5 = Lt(_3, _4);
// assert(move _5, "index out of bounds: the len is move _4 but the index is _3") -> bb2;
// _2 = const 1usize;
// _3 = Len((*(X: &[u32])));
// _4 = Lt(_2, _3);
// assert(move _4, "index out of bounds: the len is move _3 but the index is _2") -> bb1;
// }
// bb2: {
// _1 = (*_2)[_3];
// bb1: {
// _1 = (*(X: &[u32]))[_2];
// ...
// return;
// }
// END rustc.main.ConstProp.before.mir
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _3 = const 0usize;
// _4 = Len((*_2));
// _5 = Lt(_3, _4);
// assert(move _5, "index out of bounds: the len is move _4 but the index is _3") -> bb2;
// _2 = const 1usize;
// _3 = const 3usize;
// _4 = const true;
// assert(const true, "index out of bounds: the len is move _3 but the index is _2") -> bb1;
// }
// bb2: {
// _1 = (*_2)[_3];
// bb1: {
// _1 = (*(X: &[u32]))[_2];
// ...
// return;
// }
Expand Down