Store bools as i8 in scalar pairs

src/librustc/ty/layout.rs

@@ -1020,13 +1020,8 @@ impl<'a, 'tcx> LayoutCx<'tcx, TyCtxt<'a, 'tcx, 'tcx>> {
                 let mut abi = Abi::Aggregate { sized: true };
                 if tag.value.size(dl) == size {
                     abi = Abi::Scalar(tag.clone());
-                } else if !tag.is_bool() {
-                    // HACK(nox): Blindly using ScalarPair for all tagged enums
-                    // where applicable leads to Option<u8> being handled as {i1, i8},
-                    // which later confuses SROA and some loop optimisations,
-                    // ultimately leading to the repeat-trusted-len test
-                    // failing. We make the trade-off of using ScalarPair only
-                    // for types where the tag isn't a boolean.
+                } else {
+                    // Try to use a ScalarPair for all tagged enums.
                     let mut common_prim = None;
                     for (field_layouts, layout_variant) in variants.iter().zip(&layout_variants) {
                         let offsets = match layout_variant.fields {

src/librustc_codegen_llvm/abi.rs

@@ -582,9 +582,21 @@ impl<'a, 'tcx> FnTypeExt<'a, 'tcx> for FnType<'tcx, Ty<'tcx>> {
                 PassMode::Ignore => continue,
                 PassMode::Direct(_) => arg.layout.immediate_llvm_type(cx),
                 PassMode::Pair(..) => {
-                    llargument_tys.push(arg.layout.scalar_pair_element_llvm_type(cx, 0));
-                    llargument_tys.push(arg.layout.scalar_pair_element_llvm_type(cx, 1));
-                    continue;
+                    let imm_type_of = |i, scalar: &layout::Scalar| {
+                        if scalar.is_bool() {
+                            Type::i1(cx)
+                        } else {
+                            arg.layout.scalar_pair_element_llvm_type(cx, i)
+                        }
+                    };
+                    match &arg.layout.abi {
+                        layout::Abi::ScalarPair(a, b) => {
+                            llargument_tys.push(imm_type_of(0, a));
+                            llargument_tys.push(imm_type_of(1, b));
+                            continue;
+                        }
+                        _ => bug!("FnType::llvm_type: {:?} invalid for pair arugment", arg.layout)
+                    }
                 }
                 PassMode::Cast(cast) => cast.llvm_type(cx),
                 PassMode::Indirect(_) => arg.memory_ty(cx).ptr_to(),

src/librustc_codegen_llvm/mir/block.rs

@@ -615,10 +615,17 @@ impl<'a, 'tcx> FunctionCx<'a, 'tcx> {
         }
 
         if let PassMode::Pair(..) = arg.mode {
-            match op.val {
-                Pair(a, b) => {
-                    llargs.push(a);
-                    llargs.push(b);
+            let imm = |val, scalar: &layout::Scalar| {
+                if scalar.is_bool() {
+                    bx.trunc(val, Type::i1(bx.cx))
+                } else {
+                    val
+                }
+            };
+            match (op.val, &arg.layout.abi) {
+                (Pair(a_llval, b_llval), layout::Abi::ScalarPair(a, b)) => {
+                    llargs.push(imm(a_llval, a));
+                    llargs.push(imm(b_llval, b));
                     return;
                 }
                 _ => bug!("codegen_argument: {:?} invalid for pair arugment", op)

src/librustc_codegen_llvm/mir/mod.rs

@@ -516,6 +516,8 @@ fn arg_local_refs<'a, 'tcx>(bx: &Builder<'a, 'tcx>,
                     bx.set_value_name(b, &(name + ".1"));
                     llarg_idx += 1;
 
+                    let a = base::from_immediate(bx, a);
+                    let b = base::from_immediate(bx, b);
                     return local(OperandRef {
                         val: OperandValue::Pair(a, b),
                         layout: arg.layout

src/librustc_codegen_llvm/mir/operand.rs

@@ -237,15 +237,22 @@ impl<'a, 'tcx> OperandRef<'tcx> {
 
             // Extract a scalar component from a pair.
             (OperandValue::Pair(a_llval, b_llval), &layout::Abi::ScalarPair(ref a, ref b)) => {
-                if offset.bytes() == 0 {
+                let (mut llval, scalar) = if offset.bytes() == 0 {
                     assert_eq!(field.size, a.value.size(bx.cx));
-                    OperandValue::Immediate(a_llval)
+                    (a_llval, a)
                 } else {
                     assert_eq!(offset, a.value.size(bx.cx)
                         .abi_align(b.value.align(bx.cx)));
                     assert_eq!(field.size, b.value.size(bx.cx));
-                    OperandValue::Immediate(b_llval)
+                    (b_llval, b)
+                };
+                if scalar.is_bool() {
+                    // Scalar pairs store `bool` in its `i8` memory storage, like any other
+                    // aggregate, so we have to truncate to an `i1` immediate value.
+                    assert_eq!(common::val_ty(llval), Type::i8(bx.cx));
+                    llval = bx.trunc(llval, Type::i1(bx.cx));
                 }
+                OperandValue::Immediate(llval)
             }
 
             // `#[repr(simd)]` types are also immediate.
@@ -307,11 +314,9 @@ impl<'a, 'tcx> OperandValue {
             }
             OperandValue::Pair(a, b) => {
                 for (i, &x) in [a, b].iter().enumerate() {
-                    let mut llptr = bx.struct_gep(dest.llval, i as u64);
-                    // Make sure to always store i1 as i8.
-                    if common::val_ty(x) == Type::i1(bx.cx) {
-                        llptr = bx.pointercast(llptr, Type::i8p(bx.cx));
-                    }
+                    let llptr = bx.struct_gep(dest.llval, i as u64);
+                    // Pairs should always contain the memory type, particularly `bool` as `i8`.
+                    assert_ne!(common::val_ty(llptr), Type::i1(bx.cx).ptr_to());
                     let val = base::from_immediate(bx, x);
                     bx.store_with_flags(val, llptr, dest.align, flags);
                 }

src/librustc_codegen_llvm/mir/place.rs

@@ -127,18 +127,12 @@ impl<'a, 'tcx> PlaceRef<'tcx> {
             OperandValue::Immediate(base::to_immediate(bx, llval, self.layout))
         } else if let layout::Abi::ScalarPair(ref a, ref b) = self.layout.abi {
             let load = |i, scalar: &layout::Scalar| {
-                let mut llptr = bx.struct_gep(self.llval, i as u64);
-                // Make sure to always load i1 as i8.
-                if scalar.is_bool() {
-                    llptr = bx.pointercast(llptr, Type::i8p(bx.cx));
-                }
+                let llptr = bx.struct_gep(self.llval, i as u64);
+                // Pairs should always contain the memory type, particularly `bool` as `i8`.
+                assert_ne!(::common::val_ty(llptr), Type::i1(bx.cx).ptr_to());
                 let load = bx.load(llptr, self.align);
                 scalar_load_metadata(load, scalar);
-                if scalar.is_bool() {
-                    bx.trunc(load, Type::i1(bx.cx))
-                } else {
-                    load
-                }
+                load
             };
             OperandValue::Pair(load(0, a), load(1, b))
         } else {

src/librustc_codegen_llvm/mir/rvalue.rs

@@ -651,7 +651,7 @@ impl<'a, 'tcx> FunctionCx<'a, 'tcx> {
         // while the current crate doesn't use overflow checks.
         if !bx.cx.check_overflow {
             let val = self.codegen_scalar_binop(bx, op, lhs, rhs, input_ty);
-            return OperandValue::Pair(val, C_bool(bx.cx, false));
+            return OperandValue::Pair(val, C_u8(bx.cx, 0));
         }
 
         let (val, of) = match op {
@@ -685,7 +685,9 @@ impl<'a, 'tcx> FunctionCx<'a, 'tcx> {
             }
         };
 
-        OperandValue::Pair(val, of)
+        // Scalar pairs store `bool` in its `i8` memory storage, so
+        // we need to zero-extend `of` from `i1`.
+        OperandValue::Pair(val, bx.zext(of, Type::i8(bx.cx)))
     }
 
     pub fn rvalue_creates_operand(&self, rvalue: &mir::Rvalue<'tcx>) -> bool {

src/librustc_codegen_llvm/type_of.rs

@@ -361,16 +361,8 @@ impl<'tcx> LayoutLlvmExt<'tcx> for TyLayout<'tcx> {
         };
         let scalar = [a, b][index];
 
-        // Make sure to return the same type `immediate_llvm_type` would,
-        // to avoid dealing with two types and the associated conversions.
-        // This means that `(bool, bool)` is represented as `{i1, i1}`,
-        // both in memory and as an immediate, while `bool` is typically
-        // `i8` in memory and only `i1` when immediate. While we need to
-        // load/store `bool` as `i8` to avoid crippling LLVM optimizations,
-        // `i1` in a LLVM aggregate is valid and mostly equivalent to `i8`.
-        if scalar.is_bool() {
-            return Type::i1(cx);
-        }
+        // Note: We used to define `bool` as `i1` in a scalar pair, but now we
+        // always use `i8` to match the memory storage, more like LLVM expects.
 
         let offset = if index == 0 {
             Size::ZERO

src/test/codegen/function-arguments.rs

@@ -149,7 +149,7 @@ pub fn enum_id_1(x: Option<Result<u16, u16>>) -> Option<Result<u16, u16>> {
   x
 }
 
-// CHECK: i16 @enum_id_2(i16)
+// CHECK: { i8, i8 } @enum_id_2(i1 zeroext %x.0, i8 %x.1)
 #[no_mangle]
 pub fn enum_id_2(x: Option<u8>) -> Option<u8> {
   x

src/test/codegen/scalar-pair-bool.rs

@@ -0,0 +1,40 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// compile-flags: -O
+
+#![crate_type = "lib"]
+
+// CHECK: define { i8, i8 } @pair_bool_bool(i1 zeroext %pair.0, i1 zeroext %pair.1)
+#[no_mangle]
+pub fn pair_bool_bool(pair: (bool, bool)) -> (bool, bool) {
+    pair
+}
+
+// CHECK: define { i8, i32 } @pair_bool_i32(i1 zeroext %pair.0, i32 %pair.1)
+#[no_mangle]
+pub fn pair_bool_i32(pair: (bool, i32)) -> (bool, i32) {
+    pair
+}
+
+// CHECK: define { i32, i8 } @pair_i32_bool(i32 %pair.0, i1 zeroext %pair.1)
+#[no_mangle]
+pub fn pair_i32_bool(pair: (i32, bool)) -> (i32, bool) {
+    pair
+}
+
+// CHECK: define { i8, i8 } @pair_and_or(i1 zeroext %arg0.0, i1 zeroext %arg0.1)
+#[no_mangle]
+pub fn pair_and_or((a, b): (bool, bool)) -> (bool, bool) {
+    // Make sure it can operate directly on the unpacked args
+    // CHECK: and i1 %arg0.0, %arg0.1
+    // CHECK: or i1 %arg0.0, %arg0.1
+    (a && b, a || b)
+}