Skip to content

[MIR] SwitchInt Everywhere #39456

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

Merged
merged 21 commits into from
Feb 13, 2017
Merged
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
11 changes: 11 additions & 0 deletions src/librustc/middle/const_val.rs
Original file line number Diff line number Diff line change
@@ -14,6 +14,7 @@ use std::rc::Rc;
use hir::def_id::DefId;
use rustc_const_math::*;
use self::ConstVal::*;
pub use rustc_const_math::ConstInt;

use std::collections::BTreeMap;

@@ -48,4 +49,14 @@ impl ConstVal {
Char(..) => "char",
}
}

pub fn to_const_int(&self) -> Option<ConstInt> {
match *self {
ConstVal::Integral(i) => Some(i),
ConstVal::Bool(true) => Some(ConstInt::Infer(1)),
ConstVal::Bool(false) => Some(ConstInt::Infer(0)),
ConstVal::Char(ch) => Some(ConstInt::U32(ch as u32)),
_ => None
}
}
}
67 changes: 33 additions & 34 deletions src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
@@ -453,36 +453,30 @@ pub enum TerminatorKind<'tcx> {
target: BasicBlock,
},

/// jump to branch 0 if this lvalue evaluates to true
If {
cond: Operand<'tcx>,
targets: (BasicBlock, BasicBlock),
},

/// lvalue evaluates to some enum; jump depending on the branch
Switch {
discr: Lvalue<'tcx>,
adt_def: &'tcx AdtDef,
targets: Vec<BasicBlock>,
},

/// operand evaluates to an integer; jump depending on its value
/// to one of the targets, and otherwise fallback to `otherwise`
SwitchInt {
/// discriminant value being tested
discr: Lvalue<'tcx>,
discr: Operand<'tcx>,

/// type of value being tested
switch_ty: Ty<'tcx>,

/// Possible values. The locations to branch to in each case
/// are found in the corresponding indices from the `targets` vector.
values: Vec<ConstVal>,

/// Possible branch sites. The length of this vector should be
/// equal to the length of the `values` vector plus 1 -- the
/// extra item is the block to branch to if none of the values
/// fit.
values: Cow<'tcx, [ConstInt]>,

/// Possible branch sites. The last element of this vector is used
/// for the otherwise branch, so values.len() == targets.len() + 1
/// should hold.
// This invariant is quite non-obvious and also could be improved.
// One way to make this invariant is to have something like this instead:
//
// branches: Vec<(ConstInt, BasicBlock)>,
// otherwise: Option<BasicBlock> // exhaustive if None
//
// However we’ve decided to keep this as-is until we figure a case
// where some other approach seems to be strictly better than other.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the reason that I didn't do it this way is so that one could easily get a [BasicBlock] of places you might branch to. We could also just have the type-checker or other sanity checkers assert this property dynamically, which would help.

targets: Vec<BasicBlock>,
},

@@ -546,12 +540,21 @@ impl<'tcx> Terminator<'tcx> {
}

impl<'tcx> TerminatorKind<'tcx> {
pub fn if_<'a, 'gcx>(tcx: ty::TyCtxt<'a, 'gcx, 'tcx>, cond: Operand<'tcx>,
t: BasicBlock, f: BasicBlock) -> TerminatorKind<'tcx> {
static BOOL_SWITCH_FALSE: &'static [ConstInt] = &[ConstInt::Infer(0)];
TerminatorKind::SwitchInt {
discr: cond,
switch_ty: tcx.types.bool,
values: From::from(BOOL_SWITCH_FALSE),
targets: vec![f, t],
}
}

pub fn successors(&self) -> Cow<[BasicBlock]> {
use self::TerminatorKind::*;
match *self {
Goto { target: ref b } => slice::ref_slice(b).into_cow(),
If { targets: (b1, b2), .. } => vec![b1, b2].into_cow(),
Switch { targets: ref b, .. } => b[..].into_cow(),
SwitchInt { targets: ref b, .. } => b[..].into_cow(),
Resume => (&[]).into_cow(),
Return => (&[]).into_cow(),
@@ -580,8 +583,6 @@ impl<'tcx> TerminatorKind<'tcx> {
use self::TerminatorKind::*;
match *self {
Goto { target: ref mut b } => vec![b],
If { targets: (ref mut b1, ref mut b2), .. } => vec![b1, b2],
Switch { targets: ref mut b, .. } => b.iter_mut().collect(),
SwitchInt { targets: ref mut b, .. } => b.iter_mut().collect(),
Resume => Vec::new(),
Return => Vec::new(),
@@ -659,8 +660,6 @@ impl<'tcx> TerminatorKind<'tcx> {
use self::TerminatorKind::*;
match *self {
Goto { .. } => write!(fmt, "goto"),
If { cond: ref lv, .. } => write!(fmt, "if({:?})", lv),
Switch { discr: ref lv, .. } => write!(fmt, "switch({:?})", lv),
SwitchInt { discr: ref lv, .. } => write!(fmt, "switchInt({:?})", lv),
Return => write!(fmt, "return"),
Resume => write!(fmt, "resume"),
@@ -710,18 +709,11 @@ impl<'tcx> TerminatorKind<'tcx> {
match *self {
Return | Resume | Unreachable => vec![],
Goto { .. } => vec!["".into()],
If { .. } => vec!["true".into(), "false".into()],
Switch { ref adt_def, .. } => {
adt_def.variants
.iter()
.map(|variant| variant.name.to_string().into())
.collect()
}
SwitchInt { ref values, .. } => {
values.iter()
.map(|const_val| {
let mut buf = String::new();
fmt_const_val(&mut buf, const_val).unwrap();
fmt_const_val(&mut buf, &ConstVal::Integral(*const_val)).unwrap();
buf.into()
})
.chain(iter::once(String::from("otherwise").into()))
@@ -997,6 +989,12 @@ pub enum Rvalue<'tcx> {

UnaryOp(UnOp, Operand<'tcx>),

/// Read the discriminant of an ADT.
///
/// Undefined (i.e. no effort is made to make it defined, but there’s no reason why it cannot
/// be defined to return, say, a 0) if ADT is not an enum.
Discriminant(Lvalue<'tcx>),

/// Creates an *uninitialized* Box
Box(Ty<'tcx>),

@@ -1111,6 +1109,7 @@ impl<'tcx> Debug for Rvalue<'tcx> {
write!(fmt, "Checked{:?}({:?}, {:?})", op, a, b)
}
UnaryOp(ref op, ref a) => write!(fmt, "{:?}({:?})", op, a),
Discriminant(ref lval) => write!(fmt, "discriminant({:?})", lval),
Box(ref t) => write!(fmt, "Box({:?})", t),
InlineAsm { ref asm, ref outputs, ref inputs } => {
write!(fmt, "asm!({:?} : {:?} : {:?})", asm, outputs, inputs)
35 changes: 23 additions & 12 deletions src/librustc/mir/tcx.rs
Original file line number Diff line number Diff line change
@@ -18,6 +18,7 @@ use ty::subst::{Subst, Substs};
use ty::{self, AdtDef, Ty, TyCtxt};
use ty::fold::{TypeFoldable, TypeFolder, TypeVisitor};
use hir;
use ty::util::IntTypeExt;

#[derive(Copy, Clone, Debug)]
pub enum LvalueTy<'tcx> {
@@ -135,15 +136,15 @@ impl<'tcx> Lvalue<'tcx> {
impl<'tcx> Rvalue<'tcx> {
pub fn ty<'a, 'gcx>(&self, mir: &Mir<'tcx>, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> Option<Ty<'tcx>>
{
match self {
&Rvalue::Use(ref operand) => Some(operand.ty(mir, tcx)),
&Rvalue::Repeat(ref operand, ref count) => {
match *self {
Rvalue::Use(ref operand) => Some(operand.ty(mir, tcx)),
Rvalue::Repeat(ref operand, ref count) => {
let op_ty = operand.ty(mir, tcx);
let count = count.value.as_u64(tcx.sess.target.uint_type);
assert_eq!(count as usize as u64, count);
Some(tcx.mk_array(op_ty, count as usize))
}
&Rvalue::Ref(reg, bk, ref lv) => {
Rvalue::Ref(reg, bk, ref lv) => {
let lv_ty = lv.ty(mir, tcx).to_ty(tcx);
Some(tcx.mk_ref(reg,
ty::TypeAndMut {
@@ -152,27 +153,37 @@ impl<'tcx> Rvalue<'tcx> {
}
))
}
&Rvalue::Len(..) => Some(tcx.types.usize),
&Rvalue::Cast(.., ty) => Some(ty),
&Rvalue::BinaryOp(op, ref lhs, ref rhs) => {
Rvalue::Len(..) => Some(tcx.types.usize),
Rvalue::Cast(.., ty) => Some(ty),
Rvalue::BinaryOp(op, ref lhs, ref rhs) => {
let lhs_ty = lhs.ty(mir, tcx);
let rhs_ty = rhs.ty(mir, tcx);
Some(op.ty(tcx, lhs_ty, rhs_ty))
}
&Rvalue::CheckedBinaryOp(op, ref lhs, ref rhs) => {
Rvalue::CheckedBinaryOp(op, ref lhs, ref rhs) => {
let lhs_ty = lhs.ty(mir, tcx);
let rhs_ty = rhs.ty(mir, tcx);
let ty = op.ty(tcx, lhs_ty, rhs_ty);
let ty = tcx.intern_tup(&[ty, tcx.types.bool], false);
Some(ty)
}
&Rvalue::UnaryOp(_, ref operand) => {
Rvalue::UnaryOp(_, ref operand) => {
Some(operand.ty(mir, tcx))
}
&Rvalue::Box(t) => {
Rvalue::Discriminant(ref lval) => {
let ty = lval.ty(mir, tcx).to_ty(tcx);
if let ty::TyAdt(adt_def, _) = ty.sty {
Some(adt_def.discr_ty.to_ty(tcx))
} else {
// Undefined behaviour, bug for now; may want to return something for
// the `discriminant` intrinsic later.
bug!("Rvalue::Discriminant on Lvalue of type {:?}", ty);
}
}
Rvalue::Box(t) => {
Some(tcx.mk_box(t))
}
&Rvalue::Aggregate(ref ak, ref ops) => {
Rvalue::Aggregate(ref ak, ref ops) => {
match *ak {
AggregateKind::Array => {
if let Some(operand) = ops.get(0) {
@@ -196,7 +207,7 @@ impl<'tcx> Rvalue<'tcx> {
}
}
}
&Rvalue::InlineAsm { .. } => None
Rvalue::InlineAsm { .. } => None
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, this is pre-existing and not a problem with this PR per se, but I think that every rvalue should have a type. The cases where we see None here are, I think, basically bugs. For example, inline-asm should probably be a statement, if it has no result, and AggregateKind::Array should store the type as part of its variant, to handle the empty case. Is there a strong reason for it not to be this way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a strong reason for it not to be this way?

IIRC it has been this way all the way back to when first MIR patches landed. I have no idea why is it this way at the moment and I do not see any reason why it should stay an Rvalue.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, seems fine (but independent from this PR)

}
}
}
43 changes: 19 additions & 24 deletions src/librustc/mir/visit.rs
Original file line number Diff line number Diff line change
@@ -14,7 +14,6 @@ use ty::subst::Substs;
use ty::{ClosureSubsts, Region, Ty};
use mir::*;
use rustc_const_math::ConstUsize;
use rustc_data_structures::tuple_slice::TupleSlice;
use rustc_data_structures::indexed_vec::Idx;
use syntax_pos::Span;

@@ -224,6 +223,12 @@ macro_rules! make_mir_visitor {
self.super_const_val(const_val);
}

fn visit_const_int(&mut self,
const_int: &ConstInt,
_: Location) {
self.super_const_int(const_int);
}

fn visit_const_usize(&mut self,
const_usize: & $($mutability)* ConstUsize,
_: Location) {
@@ -363,31 +368,14 @@ macro_rules! make_mir_visitor {
self.visit_branch(block, target);
}

TerminatorKind::If { ref $($mutability)* cond,
ref $($mutability)* targets } => {
self.visit_operand(cond, source_location);
for &target in targets.as_slice() {
self.visit_branch(block, target);
}
}

TerminatorKind::Switch { ref $($mutability)* discr,
adt_def: _,
ref targets } => {
self.visit_lvalue(discr, LvalueContext::Inspect, source_location);
for &target in targets {
self.visit_branch(block, target);
}
}

TerminatorKind::SwitchInt { ref $($mutability)* discr,
ref $($mutability)* switch_ty,
ref $($mutability)* values,
ref values,
ref targets } => {
self.visit_lvalue(discr, LvalueContext::Inspect, source_location);
self.visit_operand(discr, source_location);
self.visit_ty(switch_ty);
for value in values {
self.visit_const_val(value, source_location);
for value in &values[..] {
self.visit_const_int(value, source_location);
}
for &target in targets {
self.visit_branch(block, target);
@@ -506,6 +494,10 @@ macro_rules! make_mir_visitor {
self.visit_operand(op, location);
}

Rvalue::Discriminant(ref $($mutability)* lvalue) => {
self.visit_lvalue(lvalue, LvalueContext::Inspect, location);
}

Rvalue::Box(ref $($mutability)* ty) => {
self.visit_ty(ty);
}
@@ -712,10 +704,13 @@ macro_rules! make_mir_visitor {
_substs: & $($mutability)* ClosureSubsts<'tcx>) {
}

fn super_const_val(&mut self, _substs: & $($mutability)* ConstVal) {
fn super_const_val(&mut self, _const_val: & $($mutability)* ConstVal) {
}

fn super_const_int(&mut self, _const_int: &ConstInt) {
}

fn super_const_usize(&mut self, _substs: & $($mutability)* ConstUsize) {
fn super_const_usize(&mut self, _const_usize: & $($mutability)* ConstUsize) {
}

// Convenience methods
4 changes: 3 additions & 1 deletion src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
@@ -673,10 +673,12 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
pub fn alloc_adt_def(self,
did: DefId,
kind: AdtKind,
discr_ty: Option<attr::IntType>,
variants: Vec<ty::VariantDef>,
repr: ReprOptions)
-> &'gcx ty::AdtDef {
let def = ty::AdtDef::new(self, did, kind, variants, repr);
let discr_ty = discr_ty.unwrap_or(attr::UnsignedInt(ast::UintTy::U8));
let def = ty::AdtDef::new(self, did, kind, discr_ty, variants, repr);
self.global_arenas.adt_def.alloc(def)
}

Loading