auto merge of #15005 : dotdash/rust/i1_bool, r=alexcrichton

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.

First, we have to fix some bugs concerning the handling of
attributes in foreign function declarations and calls. These
are required because the i1 type needs the ZExt attribute when
used as a function parameter or return type.

Then we have to update LLVM to get a bugfix without which LLVM
sometimes generates broken code when using i1.

And then, finally, we can switch bools over to i1.

Author: bors

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"),
         },

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));

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);

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)
     };

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);
     }

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,20 +337,21 @@ 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())),
                                 None,
                                 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)
         }
     }
 

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<Span>,
                 msg: &str,

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) }

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<K:KindOps> Datum<K> {
 
     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)
     }
 }
 

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);

src/librustc/middle/trans/foreign.rs

@@ -11,7 +11,7 @@
 
 use back::{link};
 use lib::llvm::llvm;
-use lib::llvm::{ValueRef, CallConv, StructRetAttribute, Linkage};
+use lib::llvm::{ValueRef, CallConv, Linkage};
 use lib;
 use middle::weak_lang_items;
 use middle::trans::base::push_ctxt;
@@ -373,18 +373,41 @@ pub fn trans_native_call<'a>(
     };
 
     // A function pointer is called without the declaration available, so we have to apply
-    // any attributes with ABI implications directly to the call instruction. Right now, the
-    // only attribute we need to worry about is `sret`.
+    // any attributes with ABI implications directly to the call instruction.
     let mut attrs = Vec::new();
-    if fn_type.ret_ty.is_indirect() {
-        attrs.push((1, lib::llvm::StructRetAttribute as u64));
 
+    // Add attributes that are always applicable, independent of the concrete foreign ABI
+    if fn_type.ret_ty.is_indirect() {
         // The outptr can be noalias and nocapture because it's entirely
         // invisible to the program. We can also mark it as nonnull
         attrs.push((1, lib::llvm::NoAliasAttribute as u64));
         attrs.push((1, lib::llvm::NoCaptureAttribute as u64));
         attrs.push((1, lib::llvm::NonNullAttribute as u64));
     };
+
+    // Add attributes that depend on the concrete foreign ABI
+    let mut arg_idx = if fn_type.ret_ty.is_indirect() { 1 } else { 0 };
+    match fn_type.ret_ty.attr {
+        Some(attr) => attrs.push((arg_idx, attr as u64)),
+        _ => ()
+    }
+
+    arg_idx += 1;
+    for arg_ty in fn_type.arg_tys.iter() {
+        if arg_ty.is_ignore() {
+            continue;
+        }
+        // skip padding
+        if arg_ty.pad.is_some() { arg_idx += 1; }
+
+        match arg_ty.attr {
+            Some(attr) => attrs.push((arg_idx, attr as u64)),
+            _ => {}
+        }
+
+        arg_idx += 1;
+    }
+
     let llforeign_retval = CallWithConv(bcx,
                                         llfn,
                                         llargs_foreign.as_slice(),
@@ -934,22 +957,17 @@ pub fn lltype_for_foreign_fn(ccx: &CrateContext, ty: ty::t) -> Type {
 
 fn add_argument_attributes(tys: &ForeignTypes,
                            llfn: ValueRef) {
-    let mut i = 0;
-
-    if tys.fn_ty.ret_ty.is_indirect() {
-        match tys.fn_ty.ret_ty.attr {
-            Some(attr) => {
-                let llarg = get_param(llfn, i);
-                unsafe {
-                    llvm::LLVMAddAttribute(llarg, attr as c_uint);
-                }
-            }
-            None => {}
-        }
+    let mut i = if tys.fn_ty.ret_ty.is_indirect() { 1 } else { 0 };
 
-        i += 1;
+    match tys.fn_ty.ret_ty.attr {
+        Some(attr) => unsafe {
+            llvm::LLVMAddFunctionAttribute(llfn, i as c_uint, attr as u64);
+        },
+        None => {}
     }
 
+    i += 1;
+
     for &arg_ty in tys.fn_ty.arg_tys.iter() {
         if arg_ty.is_ignore() {
             continue;
@@ -958,12 +976,9 @@ fn add_argument_attributes(tys: &ForeignTypes,
         if arg_ty.pad.is_some() { i += 1; }
 
         match arg_ty.attr {
-            Some(attr) => {
-                let llarg = get_param(llfn, i);
-                unsafe {
-                    llvm::LLVMAddAttribute(llarg, attr as c_uint);
-                }
-            }
+            Some(attr) => unsafe {
+                llvm::LLVMAddFunctionAttribute(llfn, i as c_uint, attr as u64);
+            },
             None => ()
         }