From bd38095ba1cb8700362bdcf48d365807caa8d9fa Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Thu, 14 Jun 2018 16:47:56 -0700 Subject: [PATCH] Store bools as i8 in scalar pairs Rust `bool` mostly uses the LLVM `i1` type for immediate values, and `i8` for memory storage, including most aggregates. Scalar pairs were an exception, also using `i1`, but this can cause problems when it is accessed through a pointer to the pair. This patch changes to use the memory type for scalar pairs too. Function argument pairs (`PassMode::Pair`) pass their pieces like distinct direct arguments, so for this case we explicitly convert `bool` to an immediate `i1` type again. This way the function can still optimize knowing the valid `0..2` range of `bool` values. Previously, `bool`-like enum tags were excluded from scalar pairs, because the `i1` didn't optimize well. Now that we're using `i8`, optimization seems fine with this, at least for the repeat-trusted-len test that was reportedly problematic before. --- src/librustc/ty/layout.rs | 9 ++---- src/librustc_codegen_llvm/abi.rs | 18 +++++++++-- src/librustc_codegen_llvm/mir/block.rs | 15 ++++++--- src/librustc_codegen_llvm/mir/mod.rs | 2 ++ src/librustc_codegen_llvm/mir/operand.rs | 21 ++++++++----- src/librustc_codegen_llvm/mir/place.rs | 14 +++------ src/librustc_codegen_llvm/mir/rvalue.rs | 6 ++-- src/librustc_codegen_llvm/type_of.rs | 12 ++----- src/test/codegen/function-arguments.rs | 2 +- src/test/codegen/scalar-pair-bool.rs | 40 ++++++++++++++++++++++++ 10 files changed, 94 insertions(+), 45 deletions(-) create mode 100644 src/test/codegen/scalar-pair-bool.rs diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 499398abcf996..da40d935fee6e 100644 --- a/src/librustc/ty/layout.rs +++ b/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 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 6b5baa402b4ab..fa81cec0d7962 100644 --- a/src/librustc_codegen_llvm/abi.rs +++ b/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(), diff --git a/src/librustc_codegen_llvm/mir/block.rs b/src/librustc_codegen_llvm/mir/block.rs index 14d20b6dbe297..fa01d5676ebc6 100644 --- a/src/librustc_codegen_llvm/mir/block.rs +++ b/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) diff --git a/src/librustc_codegen_llvm/mir/mod.rs b/src/librustc_codegen_llvm/mir/mod.rs index f9be91b4f3f13..1f5748c849676 100644 --- a/src/librustc_codegen_llvm/mir/mod.rs +++ b/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 diff --git a/src/librustc_codegen_llvm/mir/operand.rs b/src/librustc_codegen_llvm/mir/operand.rs index 9f32b41cb13e7..92c6e10836c6f 100644 --- a/src/librustc_codegen_llvm/mir/operand.rs +++ b/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); } diff --git a/src/librustc_codegen_llvm/mir/place.rs b/src/librustc_codegen_llvm/mir/place.rs index bda8c75875051..0174cb03dc3d7 100644 --- a/src/librustc_codegen_llvm/mir/place.rs +++ b/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 { diff --git a/src/librustc_codegen_llvm/mir/rvalue.rs b/src/librustc_codegen_llvm/mir/rvalue.rs index d1b949d4f7351..4e396808a4166 100644 --- a/src/librustc_codegen_llvm/mir/rvalue.rs +++ b/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 { diff --git a/src/librustc_codegen_llvm/type_of.rs b/src/librustc_codegen_llvm/type_of.rs index 88b75ff9c0943..772edbdb431d3 100644 --- a/src/librustc_codegen_llvm/type_of.rs +++ b/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 diff --git a/src/test/codegen/function-arguments.rs b/src/test/codegen/function-arguments.rs index e3fa7a7db39a2..c027dece01414 100644 --- 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>) -> Option> { 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) -> Option { x diff --git a/src/test/codegen/scalar-pair-bool.rs b/src/test/codegen/scalar-pair-bool.rs new file mode 100644 index 0000000000000..2078a2450853f --- /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 or the MIT license +// , 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) +}