Skip to content

Store bools as i8 in scalar pairs #51566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions src/librustc/ty/layout.rs
Original file line number Diff line number Diff line change
@@ -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 {
18 changes: 15 additions & 3 deletions src/librustc_codegen_llvm/abi.rs
Original file line number Diff line number Diff line change
@@ -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(),
15 changes: 11 additions & 4 deletions src/librustc_codegen_llvm/mir/block.rs
Original file line number Diff line number Diff line change
@@ -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)
2 changes: 2 additions & 0 deletions src/librustc_codegen_llvm/mir/mod.rs
Original file line number Diff line number Diff line change
@@ -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
21 changes: 13 additions & 8 deletions src/librustc_codegen_llvm/mir/operand.rs
Original file line number Diff line number Diff line change
@@ -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);
}
14 changes: 4 additions & 10 deletions src/librustc_codegen_llvm/mir/place.rs
Original file line number Diff line number Diff line change
@@ -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 {
6 changes: 4 additions & 2 deletions src/librustc_codegen_llvm/mir/rvalue.rs
Original file line number Diff line number Diff line change
@@ -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 {
12 changes: 2 additions & 10 deletions src/librustc_codegen_llvm/type_of.rs
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion src/test/codegen/function-arguments.rs
Original file line number Diff line number Diff line change
@@ -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
40 changes: 40 additions & 0 deletions src/test/codegen/scalar-pair-bool.rs
Original file line number Diff line number Diff line change
@@ -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)
}