From b9b45a0e96ad86d35aa3be45b7ef9080679abdd3 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Fri, 13 Nov 2015 00:12:50 +0200 Subject: [PATCH] address review comments --- src/librustc_mir/tcx/mod.rs | 48 ++++++++++++++++ src/librustc_trans/trans/mir/constant.rs | 26 +++++---- src/librustc_trans/trans/mir/operand.rs | 28 ++++++---- src/librustc_trans/trans/mir/rvalue.rs | 70 ++++++------------------ 4 files changed, 98 insertions(+), 74 deletions(-) diff --git a/src/librustc_mir/tcx/mod.rs b/src/librustc_mir/tcx/mod.rs index 69240e9f9953..15a49fc9d857 100644 --- a/src/librustc_mir/tcx/mod.rs +++ b/src/librustc_mir/tcx/mod.rs @@ -103,6 +103,31 @@ impl<'tcx> Mir<'tcx> { } } + pub fn binop_ty(&self, + tcx: &ty::ctxt<'tcx>, + op: BinOp, + lhs_ty: Ty<'tcx>, + rhs_ty: Ty<'tcx>) + -> Ty<'tcx> + { + // FIXME: handle SIMD correctly + match op { + BinOp::Add | BinOp::Sub | BinOp::Mul | BinOp::Div | BinOp::Rem | + BinOp::BitXor | BinOp::BitAnd | BinOp::BitOr => { + // these should be integers or floats of the same size. + assert_eq!(lhs_ty, rhs_ty); + lhs_ty + } + BinOp::Shl | BinOp::Shr => { + lhs_ty // lhs_ty can be != rhs_ty + } + BinOp::Eq | BinOp::Lt | BinOp::Le | + BinOp::Ne | BinOp::Ge | BinOp::Gt => { + tcx.types.bool + } + } + } + pub fn lvalue_ty(&self, tcx: &ty::ctxt<'tcx>, lvalue: &Lvalue<'tcx>) @@ -138,3 +163,26 @@ impl BorrowKind { } } } + +impl BinOp { + pub fn to_hir_binop(self) -> hir::BinOp_ { + match self { + BinOp::Add => hir::BinOp_::BiAdd, + BinOp::Sub => hir::BinOp_::BiSub, + BinOp::Mul => hir::BinOp_::BiMul, + BinOp::Div => hir::BinOp_::BiDiv, + BinOp::Rem => hir::BinOp_::BiRem, + BinOp::BitXor => hir::BinOp_::BiBitXor, + BinOp::BitAnd => hir::BinOp_::BiBitAnd, + BinOp::BitOr => hir::BinOp_::BiBitOr, + BinOp::Shl => hir::BinOp_::BiShl, + BinOp::Shr => hir::BinOp_::BiShr, + BinOp::Eq => hir::BinOp_::BiEq, + BinOp::Ne => hir::BinOp_::BiNe, + BinOp::Lt => hir::BinOp_::BiLt, + BinOp::Gt => hir::BinOp_::BiGt, + BinOp::Le => hir::BinOp_::BiLe, + BinOp::Ge => hir::BinOp_::BiGe + } + } +} diff --git a/src/librustc_trans/trans/mir/constant.rs b/src/librustc_trans/trans/mir/constant.rs index 923baf0dcfe1..8c0d8b10bfe4 100644 --- a/src/librustc_trans/trans/mir/constant.rs +++ b/src/librustc_trans/trans/mir/constant.rs @@ -16,7 +16,7 @@ use trans::common::{self, Block}; use trans::common::{C_bool, C_bytes, C_floating_f64, C_integral, C_str_slice}; use trans::type_of; -use super::operand::{OperandRef, OperandValue}; +use super::operand::OperandRef; use super::MirContext; impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { @@ -26,19 +26,21 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { ty: Ty<'tcx>) -> OperandRef<'tcx> { + use super::operand::OperandValue::{Ref, Immediate}; + let ccx = bcx.ccx(); let llty = type_of::type_of(ccx, ty); let val = match *cv { - ConstVal::Float(v) => OperandValue::Imm(C_floating_f64(v, llty)), - ConstVal::Bool(v) => OperandValue::Imm(C_bool(ccx, v)), - ConstVal::Int(v) => OperandValue::Imm(C_integral(llty, v as u64, true)), - ConstVal::Uint(v) => OperandValue::Imm(C_integral(llty, v, false)), - ConstVal::Str(ref v) => OperandValue::Imm(C_str_slice(ccx, v.clone())), + ConstVal::Float(v) => Immediate(C_floating_f64(v, llty)), + ConstVal::Bool(v) => Immediate(C_bool(ccx, v)), + ConstVal::Int(v) => Immediate(C_integral(llty, v as u64, true)), + ConstVal::Uint(v) => Immediate(C_integral(llty, v, false)), + ConstVal::Str(ref v) => Immediate(C_str_slice(ccx, v.clone())), ConstVal::ByteStr(ref v) => { - OperandValue::Imm(consts::addr_of(ccx, - C_bytes(ccx, v), - 1, - "byte_str")) + Immediate(consts::addr_of(ccx, + C_bytes(ccx, v), + 1, + "byte_str")) } ConstVal::Struct(id) | ConstVal::Tuple(id) => { @@ -52,9 +54,9 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { Err(_) => panic!("constant eval failure"), }; if common::type_is_immediate(bcx.ccx(), ty) { - OperandValue::Imm(llval) + Immediate(llval) } else { - OperandValue::Ref(llval) + Ref(llval) } } ConstVal::Function(_) => { diff --git a/src/librustc_trans/trans/mir/operand.rs b/src/librustc_trans/trans/mir/operand.rs index 7daf76a8d31e..63abdfe2dd91 100644 --- a/src/librustc_trans/trans/mir/operand.rs +++ b/src/librustc_trans/trans/mir/operand.rs @@ -17,8 +17,8 @@ use trans::datum; use super::{MirContext, TempRef}; -/// The Rust representation of an operand's value. This is uniquely -/// determined by the operand type, but is kept as an enum as a +/// The representation of a Rust value. The enum variant is in fact +/// uniquely determined by the value's type, but is kept as a /// safety check. #[derive(Copy, Clone)] pub enum OperandValue { @@ -26,16 +26,22 @@ pub enum OperandValue { /// to be valid for the operand's lifetime. Ref(ValueRef), /// A single LLVM value. - Imm(ValueRef), + Immediate(ValueRef), /// A fat pointer. The first ValueRef is the data and the second /// is the extra. FatPtr(ValueRef, ValueRef) } +/// An `OperandRef` is an "SSA" reference to a Rust value, along with +/// its type. +/// +/// NOTE: unless you know a value's type exactly, you should not +/// generate LLVM opcodes acting on it and instead act via methods, +/// to avoid nasty edge cases. In particular, using `build::Store` +/// directly is sure to cause problems - use `store_operand` instead. #[derive(Copy, Clone)] pub struct OperandRef<'tcx> { - // This will be "indirect" if `appropriate_rvalue_mode` returns - // ByRef, and otherwise ByValue. + // The value. pub val: OperandValue, // The type of value being returned. @@ -43,9 +49,11 @@ pub struct OperandRef<'tcx> { } impl<'tcx> OperandRef<'tcx> { + /// Asserts that this operand refers to a scalar and returns + /// a reference to its value. pub fn immediate(self) -> ValueRef { match self.val { - OperandValue::Imm(s) => s, + OperandValue::Immediate(s) => s, _ => unreachable!() } } @@ -56,8 +64,8 @@ impl<'tcx> OperandRef<'tcx> { format!("OperandRef(Ref({}) @ {:?})", bcx.val_to_string(r), self.ty) } - OperandValue::Imm(i) => { - format!("OperandRef(Imm({}) @ {:?})", + OperandValue::Immediate(i) => { + format!("OperandRef(Immediate({}) @ {:?})", bcx.val_to_string(i), self.ty) } OperandValue::FatPtr(a, d) => { @@ -106,7 +114,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { ty); let val = match datum::appropriate_rvalue_mode(bcx.ccx(), ty) { datum::ByValue => { - OperandValue::Imm(base::load_ty(bcx, tr_lvalue.llval, ty)) + OperandValue::Immediate(base::load_ty(bcx, tr_lvalue.llval, ty)) } datum::ByRef if common::type_is_fat_ptr(bcx.tcx(), ty) => { let (lldata, llextra) = base::load_fat_ptr(bcx, tr_lvalue.llval, ty); @@ -150,7 +158,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { debug!("store_operand: operand={}", operand.repr(bcx)); match operand.val { OperandValue::Ref(r) => base::memcpy_ty(bcx, lldest, r, operand.ty), - OperandValue::Imm(s) => base::store_ty(bcx, s, lldest, operand.ty), + OperandValue::Immediate(s) => base::store_ty(bcx, s, lldest, operand.ty), OperandValue::FatPtr(data, extra) => { base::store_fat_ptr(bcx, data, extra, lldest, operand.ty); } diff --git a/src/librustc_trans/trans/mir/rvalue.rs b/src/librustc_trans/trans/mir/rvalue.rs index 8f5496929c2b..cce71b257026 100644 --- a/src/librustc_trans/trans/mir/rvalue.rs +++ b/src/librustc_trans/trans/mir/rvalue.rs @@ -10,7 +10,6 @@ use llvm::ValueRef; use rustc::middle::ty::{self, Ty}; -use rustc_front::hir; use rustc_mir::repr as mir; use trans::asm; @@ -47,6 +46,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { mir::Rvalue::Cast(mir::CastKind::Unsize, ref operand, cast_ty) => { if common::type_is_fat_ptr(bcx.tcx(), cast_ty) { + // into-coerce of a thin pointer to a fat pointer - just + // use the operand path. let (bcx, temp) = self.trans_rvalue_operand(bcx, rvalue); self.store_operand(bcx, lldest, temp); return bcx; @@ -59,8 +60,13 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { let operand = self.trans_operand(bcx, operand); match operand.val { OperandValue::FatPtr(..) => unreachable!(), - OperandValue::Imm(llval) => { - // ugly alloca. + OperandValue::Immediate(llval) => { + // unsize from an immediate structure. We don't + // really need a temporary alloca here, but + // avoiding it would require us to have + // `coerce_unsized_into` use extractvalue to + // index into the struct, and this case isn't + // important enough for it. debug!("trans_rvalue: creating ugly alloca"); let lltemp = base::alloc_ty(bcx, operand.ty, "__unsize_temp"); base::store_ty(bcx, llval, lltemp, operand.ty); @@ -165,7 +171,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { // and is a no-op at the LLVM level operand.val } - OperandValue::Imm(lldata) => { + OperandValue::Immediate(lldata) => { // "standard" unsize let (lldata, llextra) = base::unsize_thin_ptr(bcx, lldata, @@ -200,7 +206,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { // destination effectively creates a reference. if common::type_is_sized(bcx.tcx(), ty) { (bcx, OperandRef { - val: OperandValue::Imm(tr_lvalue.llval), + val: OperandValue::Immediate(tr_lvalue.llval), ty: ref_ty, }) } else { @@ -215,7 +221,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { mir::Rvalue::Len(ref lvalue) => { let tr_lvalue = self.trans_lvalue(bcx, lvalue); (bcx, OperandRef { - val: OperandValue::Imm(self.lvalue_len(bcx, tr_lvalue)), + val: OperandValue::Immediate(self.lvalue_len(bcx, tr_lvalue)), ty: bcx.tcx().types.usize, }) } @@ -230,7 +236,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { base::compare_fat_ptrs(bcx, lhs_addr, lhs_extra, rhs_addr, rhs_extra, - lhs.ty, cmp_to_hir_cmp(op), + lhs.ty, op.to_hir_binop(), DebugLoc::None) } _ => unreachable!() @@ -242,8 +248,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { lhs.ty, DebugLoc::None) }; (bcx, OperandRef { - val: OperandValue::Imm(llresult), - ty: type_of_binop(bcx.tcx(), op, lhs.ty, rhs.ty), + val: OperandValue::Immediate(llresult), + ty: self.mir.binop_ty(bcx.tcx(), op, lhs.ty, rhs.ty), }) } @@ -261,7 +267,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { } }; (bcx, OperandRef { - val: OperandValue::Imm(llval), + val: OperandValue::Immediate(llval), ty: operand.ty, }) } @@ -281,7 +287,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { llalign, DebugLoc::None); (bcx, OperandRef { - val: OperandValue::Imm(llval), + val: OperandValue::Immediate(llval), ty: box_ty, }) } @@ -388,7 +394,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { mir::BinOp::Eq | mir::BinOp::Lt | mir::BinOp::Gt | mir::BinOp::Ne | mir::BinOp::Le | mir::BinOp::Ge => { base::compare_scalar_types(bcx, lhs, rhs, input_ty, - cmp_to_hir_cmp(op), debug_loc) + op.to_hir_binop(), debug_loc) } } } @@ -413,43 +419,3 @@ pub fn rvalue_creates_operand<'tcx>(rvalue: &mir::Rvalue<'tcx>) -> bool { // (*) this is only true if the type is suitable } - -fn cmp_to_hir_cmp(op: mir::BinOp) -> hir::BinOp_ { - match op { - mir::BinOp::Eq => hir::BiEq, - mir::BinOp::Ne => hir::BiNe, - mir::BinOp::Lt => hir::BiLt, - mir::BinOp::Le => hir::BiLe, - mir::BinOp::Gt => hir::BiGt, - mir::BinOp::Ge => hir::BiGe, - _ => unreachable!() - } -} - -/// FIXME(nikomatsakis): I don't think this function should go here -fn type_of_binop<'tcx>( - tcx: &ty::ctxt<'tcx>, - op: mir::BinOp, - lhs_ty: Ty<'tcx>, - rhs_ty: Ty<'tcx>) - -> Ty<'tcx> -{ - match op { - mir::BinOp::Add | mir::BinOp::Sub | - mir::BinOp::Mul | mir::BinOp::Div | mir::BinOp::Rem | - mir::BinOp::BitXor | mir::BinOp::BitAnd | mir::BinOp::BitOr => { - // these should be integers or floats of the same size. We - // probably want to dump all ops in some intrinsics framework - // someday. - assert_eq!(lhs_ty, rhs_ty); - lhs_ty - } - mir::BinOp::Shl | mir::BinOp::Shr => { - lhs_ty // lhs_ty can be != rhs_ty - } - mir::BinOp::Eq | mir::BinOp::Lt | mir::BinOp::Le | - mir::BinOp::Ne | mir::BinOp::Ge | mir::BinOp::Gt => { - tcx.types.bool - } - } -}