From d747de5a927e405c7d12ae04d213bdc05add2032 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Steinbrink?= Date: Sun, 16 Mar 2014 09:29:05 +0100 Subject: [PATCH] Compile bools to i1 We currently compiled bools to i8 values, because there was a bug in LLVM that sometimes caused miscompilations when using i1 in, for example, structs. Using i8 means a lot of unnecessary zero-extend and truncate operations though, since we have to convert the value from and to i1 when using for example icmp or br instructions. Besides the unnecessary overhead caused by this, it also sometimes made LLVM miss some optimizations. Fixes #8106. --- src/librustc/middle/trans/_match.rs | 14 ++------------ src/librustc/middle/trans/base.rs | 11 ++++++++++- src/librustc/middle/trans/cabi_arm.rs | 8 +++++--- src/librustc/middle/trans/cabi_mips.rs | 12 +++++++----- src/librustc/middle/trans/cabi_x86.rs | 8 ++++++-- src/librustc/middle/trans/cabi_x86_64.rs | 9 +++++---- src/librustc/middle/trans/common.rs | 5 ----- src/librustc/middle/trans/consts.rs | 13 +------------ src/librustc/middle/trans/datum.rs | 5 +---- src/librustc/middle/trans/expr.rs | 15 ++++----------- src/librustc/middle/trans/intrinsic.rs | 10 ++-------- src/librustc/middle/trans/reflect.rs | 1 - src/librustc/middle/trans/type_.rs | 2 +- 13 files changed, 44 insertions(+), 69 deletions(-) diff --git a/src/librustc/middle/trans/_match.rs b/src/librustc/middle/trans/_match.rs index 5b9c89f6250..5fae1635bee 100644 --- a/src/librustc/middle/trans/_match.rs +++ b/src/librustc/middle/trans/_match.rs @@ -1200,8 +1200,6 @@ fn pick_col(m: &[Match]) -> uint { pub enum branch_kind { no_branch, single, switch, compare, compare_vec_len, } // Compiles a comparison between two things. -// -// NB: This must produce an i1, not a Rust bool (i8). fn compare_values<'a>( cx: &'a Block<'a>, lhs: ValueRef, @@ -1218,11 +1216,7 @@ fn compare_values<'a>( format!("comparison of `{}`", cx.ty_to_str(rhs_t)).as_slice(), StrEqFnLangItem); - let result = callee::trans_lang_call(cx, did, [lhs, rhs], None); - Result { - bcx: result.bcx, - val: bool_to_i1(result.bcx, result.val) - } + callee::trans_lang_call(cx, did, [lhs, rhs], None) } let _icx = push_ctxt("compare_values"); @@ -1243,11 +1237,7 @@ fn compare_values<'a>( format!("comparison of `{}`", cx.ty_to_str(rhs_t)).as_slice(), UniqStrEqFnLangItem); - let result = callee::trans_lang_call(cx, did, [scratch_lhs, scratch_rhs], None); - Result { - bcx: result.bcx, - val: bool_to_i1(result.bcx, result.val) - } + callee::trans_lang_call(cx, did, [scratch_lhs, scratch_rhs], None) } _ => cx.sess().bug("only strings supported in compare_values"), }, diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs index 35795cc9f23..da49c7764ce 100644 --- a/src/librustc/middle/trans/base.rs +++ b/src/librustc/middle/trans/base.rs @@ -501,7 +501,6 @@ pub fn maybe_name_value(cx: &CrateContext, v: ValueRef, s: &str) { // Used only for creating scalar comparison glue. pub enum scalar_type { nil_type, signed_int, unsigned_int, floating_point, } -// NB: This produces an i1, not a Rust bool (i8). pub fn compare_scalar_types<'a>( cx: &'a Block<'a>, lhs: ValueRef, @@ -1815,6 +1814,13 @@ pub fn get_fn_llvm_attributes(ccx: &CrateContext, fn_ty: ty::t) -> Vec<(uint, u6 } _ => {} } + + match ty::get(ret_ty).sty { + ty::ty_bool => { + attrs.push((lib::llvm::ReturnIndex as uint, lib::llvm::ZExtAttribute as u64)); + } + _ => {} + } } for (idx, &t) in fn_sig.inputs.iter().enumerate().map(|(i, v)| (i + first_arg_offset, v)) { @@ -1828,6 +1834,9 @@ pub fn get_fn_llvm_attributes(ccx: &CrateContext, fn_ty: ty::t) -> Vec<(uint, u6 attrs.push((idx, lib::llvm::NoCaptureAttribute as u64)); attrs.push((idx, lib::llvm::NonNullAttribute as u64)); } + ty::ty_bool => { + attrs.push((idx, lib::llvm::ZExtAttribute as u64)); + } // `~` pointer parameters never alias because ownership is transferred ty::ty_uniq(_) => { attrs.push((idx, lib::llvm::NoAliasAttribute as u64)); diff --git a/src/librustc/middle/trans/cabi_arm.rs b/src/librustc/middle/trans/cabi_arm.rs index ee2c6454aee..01bef64ebba 100644 --- a/src/librustc/middle/trans/cabi_arm.rs +++ b/src/librustc/middle/trans/cabi_arm.rs @@ -11,7 +11,7 @@ #![allow(non_uppercase_pattern_statics)] use lib::llvm::{llvm, Integer, Pointer, Float, Double, Struct, Array}; -use lib::llvm::StructRetAttribute; +use lib::llvm::{StructRetAttribute, ZExtAttribute}; use middle::trans::cabi::{FnType, ArgType}; use middle::trans::context::CrateContext; use middle::trans::type_::Type; @@ -85,7 +85,8 @@ fn ty_size(ty: Type) -> uint { fn classify_ret_ty(ccx: &CrateContext, ty: Type) -> ArgType { if is_reg_ty(ty) { - return ArgType::direct(ty, None, None, None); + let attr = if ty == Type::bool(ccx) { Some(ZExtAttribute) } else { None }; + return ArgType::direct(ty, None, None, attr); } let size = ty_size(ty); if size <= 4 { @@ -103,7 +104,8 @@ fn classify_ret_ty(ccx: &CrateContext, ty: Type) -> ArgType { fn classify_arg_ty(ccx: &CrateContext, ty: Type) -> ArgType { if is_reg_ty(ty) { - return ArgType::direct(ty, None, None, None); + let attr = if ty == Type::bool(ccx) { Some(ZExtAttribute) } else { None }; + return ArgType::direct(ty, None, None, attr); } let align = ty_align(ty); let size = ty_size(ty); diff --git a/src/librustc/middle/trans/cabi_mips.rs b/src/librustc/middle/trans/cabi_mips.rs index 395bc637aad..60db609e59e 100644 --- a/src/librustc/middle/trans/cabi_mips.rs +++ b/src/librustc/middle/trans/cabi_mips.rs @@ -13,7 +13,7 @@ use libc::c_uint; use std::cmp; use lib::llvm::{llvm, Integer, Pointer, Float, Double, Struct, Array}; -use lib::llvm::StructRetAttribute; +use lib::llvm::{StructRetAttribute, ZExtAttribute}; use middle::trans::context::CrateContext; use middle::trans::cabi::*; use middle::trans::type_::Type; @@ -83,9 +83,10 @@ fn ty_size(ty: Type) -> uint { } } -fn classify_ret_ty(ty: Type) -> ArgType { +fn classify_ret_ty(ccx: &CrateContext, ty: Type) -> ArgType { if is_reg_ty(ty) { - ArgType::direct(ty, None, None, None) + let attr = if ty == Type::bool(ccx) { Some(ZExtAttribute) } else { None }; + ArgType::direct(ty, None, None, attr) } else { ArgType::indirect(ty, Some(StructRetAttribute)) } @@ -101,7 +102,8 @@ fn classify_arg_ty(ccx: &CrateContext, ty: Type, offset: &mut uint) -> ArgType { *offset += align_up_to(size, align * 8) / 8; if is_reg_ty(ty) { - ArgType::direct(ty, None, None, None) + let attr = if ty == Type::bool(ccx) { Some(ZExtAttribute) } else { None }; + ArgType::direct(ty, None, None, attr) } else { ArgType::direct( ty, @@ -160,7 +162,7 @@ pub fn compute_abi_info(ccx: &CrateContext, rty: Type, ret_def: bool) -> FnType { let ret_ty = if ret_def { - classify_ret_ty(rty) + classify_ret_ty(ccx, rty) } else { ArgType::direct(Type::void(ccx), None, None, None) }; diff --git a/src/librustc/middle/trans/cabi_x86.rs b/src/librustc/middle/trans/cabi_x86.rs index d10f6b72820..5fffdf08646 100644 --- a/src/librustc/middle/trans/cabi_x86.rs +++ b/src/librustc/middle/trans/cabi_x86.rs @@ -59,7 +59,8 @@ pub fn compute_abi_info(ccx: &CrateContext, } } } else { - ret_ty = ArgType::direct(rty, None, None, None); + let attr = if rty == Type::bool(ccx) { Some(ZExtAttribute) } else { None }; + ret_ty = ArgType::direct(rty, None, None, attr); } for &t in atys.iter() { @@ -72,7 +73,10 @@ pub fn compute_abi_info(ccx: &CrateContext, ArgType::indirect(t, Some(ByValAttribute)) } } - _ => ArgType::direct(t, None, None, None), + _ => { + let attr = if t == Type::bool(ccx) { Some(ZExtAttribute) } else { None }; + ArgType::direct(t, None, None, attr) + } }; arg_tys.push(ty); } diff --git a/src/librustc/middle/trans/cabi_x86_64.rs b/src/librustc/middle/trans/cabi_x86_64.rs index 9f98cf57c09..b2cd9d256dd 100644 --- a/src/librustc/middle/trans/cabi_x86_64.rs +++ b/src/librustc/middle/trans/cabi_x86_64.rs @@ -15,7 +15,7 @@ use lib::llvm::{llvm, Integer, Pointer, Float, Double}; use lib::llvm::{Struct, Array, Attribute}; -use lib::llvm::{StructRetAttribute, ByValAttribute}; +use lib::llvm::{StructRetAttribute, ByValAttribute, ZExtAttribute}; use middle::trans::cabi::*; use middle::trans::context::CrateContext; use middle::trans::type_::Type; @@ -337,12 +337,12 @@ pub fn compute_abi_info(ccx: &CrateContext, fn x86_64_ty(ccx: &CrateContext, ty: Type, is_mem_cls: |cls: &[RegClass]| -> bool, - attr: Attribute) + ind_attr: Attribute) -> ArgType { if !ty.is_reg_ty() { let cls = classify_ty(ty); if is_mem_cls(cls.as_slice()) { - ArgType::indirect(ty, Some(attr)) + ArgType::indirect(ty, Some(ind_attr)) } else { ArgType::direct(ty, Some(llreg_ty(ccx, cls.as_slice())), @@ -350,7 +350,8 @@ pub fn compute_abi_info(ccx: &CrateContext, None) } } else { - ArgType::direct(ty, None, None, None) + let attr = if ty == Type::bool(ccx) { Some(ZExtAttribute) } else { None }; + ArgType::direct(ty, None, None, attr) } } diff --git a/src/librustc/middle/trans/common.rs b/src/librustc/middle/trans/common.rs index d7509866e08..a1923022e7b 100644 --- a/src/librustc/middle/trans/common.rs +++ b/src/librustc/middle/trans/common.rs @@ -818,11 +818,6 @@ pub fn find_vtable(tcx: &ty::ctxt, param_bounds.get(n_bound).clone() } -// Casts a Rust bool value to an i1. -pub fn bool_to_i1(bcx: &Block, llval: ValueRef) -> ValueRef { - build::ICmp(bcx, lib::llvm::IntNE, llval, C_bool(bcx.ccx(), false)) -} - pub fn langcall(bcx: &Block, span: Option, msg: &str, diff --git a/src/librustc/middle/trans/consts.rs b/src/librustc/middle/trans/consts.rs index 338821537e8..527ce5dfaae 100644 --- a/src/librustc/middle/trans/consts.rs +++ b/src/librustc/middle/trans/consts.rs @@ -399,18 +399,7 @@ fn const_expr_unadjusted(cx: &CrateContext, e: &ast::Expr, let (dv, _dt) = const_deref(cx, te, ty, true); dv } - ast::UnNot => { - match ty::get(ty).sty { - ty::ty_bool => { - // Somewhat questionable, but I believe this is - // correct. - let te = llvm::LLVMConstTrunc(te, Type::i1(cx).to_ref()); - let te = llvm::LLVMConstNot(te); - llvm::LLVMConstZExt(te, Type::bool(cx).to_ref()) - } - _ => llvm::LLVMConstNot(te), - } - } + ast::UnNot => llvm::LLVMConstNot(te), ast::UnNeg => { if is_float { llvm::LLVMConstFNeg(te) } else { llvm::LLVMConstNeg(te) } diff --git a/src/librustc/middle/trans/datum.rs b/src/librustc/middle/trans/datum.rs index 158a7d6cf7a..440aa36b28c 100644 --- a/src/librustc/middle/trans/datum.rs +++ b/src/librustc/middle/trans/datum.rs @@ -525,8 +525,6 @@ fn load<'a>(bcx: &'a Block<'a>, llptr: ValueRef, ty: ty::t) -> ValueRef { if type_is_zero_size(bcx.ccx(), ty) { C_undef(type_of::type_of(bcx.ccx(), ty)) - } else if ty::type_is_bool(ty) { - LoadRangeAssert(bcx, llptr, 0, 2, lib::llvm::False) } else if ty::type_is_char(ty) { // a char is a unicode codepoint, and so takes values from 0 // to 0x10FFFF inclusive only. @@ -652,8 +650,7 @@ impl Datum { pub fn to_llbool<'a>(self, bcx: &'a Block<'a>) -> ValueRef { assert!(ty::type_is_bool(self.ty) || ty::type_is_bot(self.ty)) - let cond_val = self.to_llscalarish(bcx); - bool_to_i1(bcx, cond_val) + self.to_llscalarish(bcx) } } diff --git a/src/librustc/middle/trans/expr.rs b/src/librustc/middle/trans/expr.rs index 9c957eec90a..9af5c7aa792 100644 --- a/src/librustc/middle/trans/expr.rs +++ b/src/librustc/middle/trans/expr.rs @@ -1147,11 +1147,7 @@ fn trans_unary<'a>(bcx: &'a Block<'a>, let datum = unpack_datum!(bcx, trans(bcx, sub_expr)); let llresult = if ty::type_is_bool(un_ty) { let val = datum.to_llscalarish(bcx); - let llcond = ICmp(bcx, - lib::llvm::IntEQ, - val, - C_bool(ccx, false)); - Select(bcx, llcond, C_bool(ccx, true), C_bool(ccx, false)) + Xor(bcx, val, C_bool(ccx, true)) } else { // Note: `Not` is bitwise, not suitable for logical not. Not(bcx, datum.to_llscalarish(bcx)) @@ -1325,9 +1321,7 @@ fn trans_eager_binop<'a>( if ty::type_is_bot(rhs_t) { C_bool(bcx.ccx(), false) } else if ty::type_is_scalar(rhs_t) { - let cmpr = base::compare_scalar_types(bcx, lhs, rhs, rhs_t, op); - bcx = cmpr.bcx; - ZExt(bcx, cmpr.val, Type::i8(bcx.ccx())) + unpack_result!(bcx, base::compare_scalar_types(bcx, lhs, rhs, rhs_t, op)) } else if is_simd { base::compare_simd_types(bcx, lhs, rhs, intype, ty::simd_size(tcx, lhs_t), op) } else { @@ -1369,10 +1363,9 @@ fn trans_lazy_binop<'a>( let join = fcx.new_id_block("join", binop_expr.id); let before_rhs = fcx.new_id_block("before_rhs", b.id); - let lhs_i1 = bool_to_i1(past_lhs, lhs); match op { - lazy_and => CondBr(past_lhs, lhs_i1, before_rhs.llbb, join.llbb), - lazy_or => CondBr(past_lhs, lhs_i1, join.llbb, before_rhs.llbb) + lazy_and => CondBr(past_lhs, lhs, before_rhs.llbb, join.llbb), + lazy_or => CondBr(past_lhs, lhs, join.llbb, before_rhs.llbb) } let DatumBlock {bcx: past_rhs, datum: rhs} = trans(before_rhs, b); diff --git a/src/librustc/middle/trans/intrinsic.rs b/src/librustc/middle/trans/intrinsic.rs index 7624a71c9dd..bc0c88ceee9 100644 --- a/src/librustc/middle/trans/intrinsic.rs +++ b/src/librustc/middle/trans/intrinsic.rs @@ -96,19 +96,13 @@ pub fn trans_intrinsic(ccx: &CrateContext, let b = get_param(bcx.fcx.llfn, first_real_arg + 1); let llfn = bcx.ccx().get_intrinsic(&name); - // convert `i1` to a `bool`, and write to the out parameter let val = Call(bcx, llfn, [a, b], []); - let result = ExtractValue(bcx, val, 0); - let overflow = ZExt(bcx, ExtractValue(bcx, val, 1), Type::bool(bcx.ccx())); - let ret = C_undef(type_of::type_of(bcx.ccx(), t)); - let ret = InsertValue(bcx, ret, result, 0); - let ret = InsertValue(bcx, ret, overflow, 1); if type_is_immediate(bcx.ccx(), t) { - Ret(bcx, ret); + Ret(bcx, val); } else { let retptr = get_param(bcx.fcx.llfn, bcx.fcx.out_arg_pos()); - Store(bcx, ret, retptr); + Store(bcx, val, retptr); RetVoid(bcx); } } diff --git a/src/librustc/middle/trans/reflect.rs b/src/librustc/middle/trans/reflect.rs index 50681762956..59903324e10 100644 --- a/src/librustc/middle/trans/reflect.rs +++ b/src/librustc/middle/trans/reflect.rs @@ -107,7 +107,6 @@ impl<'a, 'b> Reflector<'a, 'b> { mth_idx, v), ArgVals(args), None)); - let result = bool_to_i1(bcx, result); let next_bcx = fcx.new_temp_block("next"); CondBr(bcx, result, next_bcx.llbb, self.final_bcx.llbb); self.bcx = next_bcx diff --git a/src/librustc/middle/trans/type_.rs b/src/librustc/middle/trans/type_.rs index 5d58500f761..75f884aac34 100644 --- a/src/librustc/middle/trans/type_.rs +++ b/src/librustc/middle/trans/type_.rs @@ -93,7 +93,7 @@ impl Type { } pub fn bool(ccx: &CrateContext) -> Type { - Type::i8(ccx) + Type::i1(ccx) } pub fn char(ccx: &CrateContext) -> Type {