Store scalar pair bools as i8 in memory

We represent `bool` as `i1` in a `ScalarPair`, unlike other aggregates,
to optimize IR for checked operators and the like.  With this patch, we
still do so when the pair is an immediate value, but we use the `i8`
memory type when the value is loaded or stored as an LLVM aggregate.

So `(bool, bool)` looks like an `{ i1, i1 }` immediate, but `{ i8, i8 }`
in memory.  When a pair is a direct function argument, `PassMode::Pair`,
it is still passed using the immediate `i1` type, but as a return value
it will use the `i8` memory type.  Also, `bool`-like` enum tags will now
use scalar pairs when possible, where they were previously excluded due
to optimization issues.

diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs
index a32fdbb285d12cdae128c30dcf4a80077589893b..faad32a5d994ec3402fa659295787672457ce82b 100644 (file)
--- a/src/librustc/ty/layout.rs
+++ b/src/librustc/ty/layout.rs
@@ -1020,13 +1020,8 @@ enum StructKind {
                 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 {

diff --git a/src/librustc_codegen_llvm/abi.rs b/src/librustc_codegen_llvm/abi.rs
index 6b5baa402b4ab88642e0821004e65e2a6bd2efbe..4ff31ec7d1c18c364dd925ea02e15fb7ce6725df 100644 (file)
--- a/src/librustc_codegen_llvm/abi.rs
+++ b/src/librustc_codegen_llvm/abi.rs
@@ -582,8 +582,8 @@ fn llvm_type(&self, cx: &CodegenCx<'a, 'tcx>) -> Type {
                 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));
+                    llargument_tys.push(arg.layout.scalar_pair_element_llvm_type(cx, 0, true));
+                    llargument_tys.push(arg.layout.scalar_pair_element_llvm_type(cx, 1, true));
                     continue;
                 }
                 PassMode::Cast(cast) => cast.llvm_type(cx),

diff --git a/src/librustc_codegen_llvm/base.rs b/src/librustc_codegen_llvm/base.rs
index a4709739a23ddabcbb587813975a407d82fe9af3..4605b5f3f1d792b53c6d52a68e099c3c52b7bafc 100644 (file)
--- a/src/librustc_codegen_llvm/base.rs
+++ b/src/librustc_codegen_llvm/base.rs
@@ -265,8 +265,8 @@ pub fn unsize_thin_ptr<'a, 'tcx>(
             }
             let (lldata, llextra) = result.unwrap();
             // HACK(eddyb) have to bitcast pointers until LLVM removes pointee types.
-            (bx.bitcast(lldata, dst_layout.scalar_pair_element_llvm_type(bx.cx, 0)),
-             bx.bitcast(llextra, dst_layout.scalar_pair_element_llvm_type(bx.cx, 1)))
+            (bx.bitcast(lldata, dst_layout.scalar_pair_element_llvm_type(bx.cx, 0, true)),
+             bx.bitcast(llextra, dst_layout.scalar_pair_element_llvm_type(bx.cx, 1, true)))
         }
         _ => bug!("unsize_thin_ptr: called on bad types"),
     }
@@ -396,9 +396,14 @@ pub fn from_immediate(bx: &Builder, val: ValueRef) -> ValueRef {
 
 pub fn to_immediate(bx: &Builder, val: ValueRef, layout: layout::TyLayout) -> ValueRef {
     if let layout::Abi::Scalar(ref scalar) = layout.abi {
-        if scalar.is_bool() {
-            return bx.trunc(val, Type::i1(bx.cx));
-        }
+        return to_immediate_scalar(bx, val, scalar);
+    }
+    val
+}
+
+pub fn to_immediate_scalar(bx: &Builder, val: ValueRef, scalar: &layout::Scalar) -> ValueRef {
+    if scalar.is_bool() {
+        return bx.trunc(val, Type::i1(bx.cx));
     }
     val
 }

diff --git a/src/librustc_codegen_llvm/mir/operand.rs b/src/librustc_codegen_llvm/mir/operand.rs
index 3d3a4400bd8108abca11973e5693f839dbc5fc32..3069a155d1f0a76d7e30d00b6b8323d54127344e 100644 (file)
--- a/src/librustc_codegen_llvm/mir/operand.rs
+++ b/src/librustc_codegen_llvm/mir/operand.rs
@@ -128,13 +128,13 @@ pub fn from_const(bx: &Builder<'a, 'tcx>,
                     bx.cx,
                     a,
                     a_scalar,
-                    layout.scalar_pair_element_llvm_type(bx.cx, 0),
+                    layout.scalar_pair_element_llvm_type(bx.cx, 0, true),
                 );
                 let b_llval = scalar_to_llvm(
                     bx.cx,
                     b,
                     b_scalar,
-                    layout.scalar_pair_element_llvm_type(bx.cx, 1),
+                    layout.scalar_pair_element_llvm_type(bx.cx, 1, true),
                 );
                 OperandValue::Pair(a_llval, b_llval)
             },
@@ -193,8 +193,8 @@ pub fn immediate_or_packed_pair(self, bx: &Builder<'a, 'tcx>) -> ValueRef {
                    self, llty);
             // Reconstruct the immediate aggregate.
             let mut llpair = C_undef(llty);
-            llpair = bx.insert_value(llpair, a, 0);
-            llpair = bx.insert_value(llpair, b, 1);
+            llpair = bx.insert_value(llpair, base::from_immediate(bx, a), 0);
+            llpair = bx.insert_value(llpair, base::from_immediate(bx, b), 1);
             llpair
         } else {
             self.immediate()
@@ -206,13 +206,14 @@ pub fn from_immediate_or_packed_pair(bx: &Builder<'a, 'tcx>,
                                          llval: ValueRef,
                                          layout: TyLayout<'tcx>)
                                          -> OperandRef<'tcx> {
-        let val = if layout.is_llvm_scalar_pair() {
+        let val = if let layout::Abi::ScalarPair(ref a, ref b) = layout.abi {
             debug!("Operand::from_immediate_or_packed_pair: unpacking {:?} @ {:?}",
                     llval, layout);
 
             // Deconstruct the immediate aggregate.
-            OperandValue::Pair(bx.extract_value(llval, 0),
-                               bx.extract_value(llval, 1))
+            let a_llval = base::to_immediate_scalar(bx, bx.extract_value(llval, 0), a);
+            let b_llval = base::to_immediate_scalar(bx, bx.extract_value(llval, 1), b);
+            OperandValue::Pair(a_llval, b_llval)
         } else {
             OperandValue::Immediate(llval)
         };
@@ -264,8 +265,8 @@ pub fn extract_field(&self, bx: &Builder<'a, 'tcx>, i: usize) -> OperandRef<'tcx
                 *llval = bx.bitcast(*llval, field.immediate_llvm_type(bx.cx));
             }
             OperandValue::Pair(ref mut a, ref mut b) => {
-                *a = bx.bitcast(*a, field.scalar_pair_element_llvm_type(bx.cx, 0));
-                *b = bx.bitcast(*b, field.scalar_pair_element_llvm_type(bx.cx, 1));
+                *a = bx.bitcast(*a, field.scalar_pair_element_llvm_type(bx.cx, 0, true));
+                *b = bx.bitcast(*b, field.scalar_pair_element_llvm_type(bx.cx, 1, true));
             }
             OperandValue::Ref(..) => bug!()
         }
@@ -308,10 +309,10 @@ fn store_with_flags(self, bx: &Builder<'a, 'tcx>, dest: PlaceRef<'tcx>, flags: M
             }
             OperandValue::Pair(a, b) => {
                 for (i, &x) in [a, b].iter().enumerate() {
-                    let mut llptr = bx.struct_gep(dest.llval, i as u64);
+                    let 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));
+                        assert_eq!(common::val_ty(llptr), Type::i8p(bx.cx));
                     }
                     let val = base::from_immediate(bx, x);
                     bx.store_with_flags(val, llptr, dest.align, flags);

diff --git a/src/librustc_codegen_llvm/mir/place.rs b/src/librustc_codegen_llvm/mir/place.rs
index 2a1e3980adbcfaa58ee83880d10818127ce5d8cb..36dcd04b02e44847630f804c92a546081bf446df 100644 (file)
--- a/src/librustc_codegen_llvm/mir/place.rs
+++ b/src/librustc_codegen_llvm/mir/place.rs
@@ -16,7 +16,7 @@
 use rustc_data_structures::indexed_vec::Idx;
 use base;
 use builder::Builder;
-use common::{CodegenCx, C_undef, C_usize, C_u8, C_u32, C_uint, C_null, C_uint_big};
+use common::{CodegenCx, C_undef, C_usize, C_u8, C_u32, C_uint, C_null, C_uint_big, val_ty};
 use consts;
 use type_of::LayoutLlvmExt;
 use type_::Type;
@@ -127,10 +127,10 @@ pub fn load(&self, bx: &Builder<'a, 'tcx>) -> OperandRef<'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);
+                let 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));
+                    assert_eq!(val_ty(llptr), Type::i8p(bx.cx));
                 }
                 let load = bx.load(llptr, self.align);
                 scalar_load_metadata(load, scalar);

diff --git a/src/librustc_codegen_llvm/mir/rvalue.rs b/src/librustc_codegen_llvm/mir/rvalue.rs
index 0fd81c6074e48e87012ee895350119966bc5ff29..2e81fc16a58388d935024ac40d25b4ff1cb59b32 100644 (file)
--- a/src/librustc_codegen_llvm/mir/rvalue.rs
+++ b/src/librustc_codegen_llvm/mir/rvalue.rs
@@ -232,7 +232,7 @@ pub fn codegen_rvalue_operand(&mut self,
                                 // HACK(eddyb) have to bitcast pointers
                                 // until LLVM removes pointee types.
                                 let lldata = bx.pointercast(lldata,
-                                    cast.scalar_pair_element_llvm_type(bx.cx, 0));
+                                    cast.scalar_pair_element_llvm_type(bx.cx, 0, true));
                                 OperandValue::Pair(lldata, llextra)
                             }
                             OperandValue::Immediate(lldata) => {
@@ -251,7 +251,7 @@ pub fn codegen_rvalue_operand(&mut self,
                         if let OperandValue::Pair(data_ptr, meta) = operand.val {
                             if cast.is_llvm_scalar_pair() {
                                 let data_cast = bx.pointercast(data_ptr,
-                                    cast.scalar_pair_element_llvm_type(bx.cx, 0));
+                                    cast.scalar_pair_element_llvm_type(bx.cx, 0, true));
                                 OperandValue::Pair(data_cast, meta)
                             } else { // cast to thin-ptr
                                 // Cast of fat-ptr to thin-ptr is an extraction of data-ptr and

diff --git a/src/librustc_codegen_llvm/type_of.rs b/src/librustc_codegen_llvm/type_of.rs
index 88b75ff9c09439cbfa68c1241024ae1c06db99b2..195f1c3b85aea379590ad8f2362394d79898426e 100644 (file)
--- a/src/librustc_codegen_llvm/type_of.rs
+++ b/src/librustc_codegen_llvm/type_of.rs
@@ -47,8 +47,8 @@ fn uncached_llvm_type<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>,
         }
         layout::Abi::ScalarPair(..) => {
             return Type::struct_(cx, &[
-                layout.scalar_pair_element_llvm_type(cx, 0),
-                layout.scalar_pair_element_llvm_type(cx, 1),
+                layout.scalar_pair_element_llvm_type(cx, 0, false),
+                layout.scalar_pair_element_llvm_type(cx, 1, false),
             ], false);
         }
         layout::Abi::Uninhabited |
@@ -206,7 +206,7 @@ pub trait LayoutLlvmExt<'tcx> {
     fn scalar_llvm_type_at<'a>(&self, cx: &CodegenCx<'a, 'tcx>,
                                scalar: &layout::Scalar, offset: Size) -> Type;
     fn scalar_pair_element_llvm_type<'a>(&self, cx: &CodegenCx<'a, 'tcx>,
-                                         index: usize) -> Type;
+                                         index: usize, immediate: bool) -> Type;
     fn llvm_field_index(&self, index: usize) -> u64;
     fn pointee_info_at<'a>(&self, cx: &CodegenCx<'a, 'tcx>, offset: Size)
                            -> Option<PointeeInfo>;
@@ -340,7 +340,7 @@ fn scalar_llvm_type_at<'a>(&self, cx: &CodegenCx<'a, 'tcx>,
     }
 
     fn scalar_pair_element_llvm_type<'a>(&self, cx: &CodegenCx<'a, 'tcx>,
-                                         index: usize) -> Type {
+                                         index: usize, immediate: bool) -> Type {
         // HACK(eddyb) special-case fat pointers until LLVM removes
         // pointee types, to avoid bitcasting every `OperandRef::deref`.
         match self.ty.sty {
@@ -350,7 +350,7 @@ fn scalar_pair_element_llvm_type<'a>(&self, cx: &CodegenCx<'a, 'tcx>,
             }
             ty::TyAdt(def, _) if def.is_box() => {
                 let ptr_ty = cx.tcx.mk_mut_ptr(self.ty.boxed_ty());
-                return cx.layout_of(ptr_ty).scalar_pair_element_llvm_type(cx, index);
+                return cx.layout_of(ptr_ty).scalar_pair_element_llvm_type(cx, index, immediate);
             }
             _ => {}
         }
@@ -361,14 +361,13 @@ fn scalar_pair_element_llvm_type<'a>(&self, cx: &CodegenCx<'a, '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() {
+        // Make sure to return the same type `immediate_llvm_type` would when
+        // dealing with an immediate pair.  This means that `(bool, bool)` is
+        // effectively represented as `{i8, i8}` in memory and `{i1, i1}` as an
+        // immediate, just like `bool` is typically `i8` in memory and only `i1`
+        // when immediate.  We need to load/store `bool` as `i8` to avoid
+        // crippling LLVM optimizations or triggering other LLVM bugs with `i1`.
+        if immediate && scalar.is_bool() {
             return Type::i1(cx);
         }
 

diff --git a/src/test/codegen/function-arguments.rs b/src/test/codegen/function-arguments.rs
index e3fa7a7db39a254bd2736fdd8ced347ad4a0157c..c027dece01414255ea136847fe7d82c519f6268d 100644 (file)
--- a/src/test/codegen/function-arguments.rs
+++ b/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

diff --git a/src/test/codegen/scalar-pair-bool.rs b/src/test/codegen/scalar-pair-bool.rs
new file mode 100644 (file)
index 0000000..2078a24
--- /dev/null
+++ b/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)
+}