Skip to content
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.

Sign up for GitHub

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

Fix a couple ICEs in the new CastKind::Transmute code #110021

Merged
merged 2 commits into from
Apr 10, 2023
Merged

Fix a couple ICEs in the new CastKind::Transmute code #110021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
454bca5
Check `CastKind::Transmute` sizes in a better way
scottmcm Apr 6, 2023
d757c4b
Handle not all immediates having `abi::Scalar`s
scottmcm Apr 6, 2023
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
25 changes: 25 additions & 0 deletions compiler/rustc_codegen_ssa/src/mir/operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,31 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
}

impl<'a, 'tcx, V: CodegenObject> OperandValue<V> {
/// Returns an `OperandValue` that's generally UB to use in any way.
///
/// Depending on the `layout`, returns an `Immediate` or `Pair` containing
/// poison value(s), or a `Ref` containing a poison pointer.
///
/// Supports sized types only.
pub fn poison<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
bx: &mut Bx,
layout: TyAndLayout<'tcx>,
) -> OperandValue<V> {
assert!(layout.is_sized());
if bx.cx().is_backend_immediate(layout) {
let ibty = bx.cx().immediate_backend_type(layout);
OperandValue::Immediate(bx.const_poison(ibty))
} else if bx.cx().is_backend_scalar_pair(layout) {
let ibty0 = bx.cx().scalar_pair_element_backend_type(layout, 0, true);
let ibty1 = bx.cx().scalar_pair_element_backend_type(layout, 1, true);
OperandValue::Pair(bx.const_poison(ibty0), bx.const_poison(ibty1))
} else {
let bty = bx.cx().backend_type(layout);
let ptr_bty = bx.cx().type_ptr_to(bty);
OperandValue::Ref(bx.const_poison(ptr_bty), None, layout.align.abi)
}
}

pub fn store<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
self,
bx: &mut Bx,
Expand Down
107 changes: 71 additions & 36 deletions compiler/rustc_codegen_ssa/src/mir/rvalue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,17 +158,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
debug_assert!(src.layout.is_sized());
debug_assert!(dst.layout.is_sized());

if src.layout.size != dst.layout.size
|| src.layout.abi.is_uninhabited()
|| dst.layout.abi.is_uninhabited()
{
// In all of these cases it's UB to run this transmute, but that's
// known statically so might as well trap for it, rather than just
// making it unreachable.
bx.abort();
return;
}

if let Some(val) = self.codegen_transmute_operand(bx, src, dst.layout) {
val.store(bx, dst);
return;
Expand Down Expand Up @@ -202,8 +191,21 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
operand: OperandRef<'tcx, Bx::Value>,
cast: TyAndLayout<'tcx>,
) -> Option<OperandValue<Bx::Value>> {
// Callers already checked that the layout sizes match
debug_assert_eq!(operand.layout.size, cast.size);
// Check for transmutes that are always UB.
if operand.layout.size != cast.size
|| operand.layout.abi.is_uninhabited()
|| cast.abi.is_uninhabited()
{
if !operand.layout.abi.is_uninhabited() {
// Since this is known statically and the input could have existed
// without already having hit UB, might as well trap for it.
bx.abort();
}

// Because this transmute is UB, return something easy to generate,
// since it's fine that later uses of the value are probably UB.
return Some(OperandValue::poison(bx, cast));
}

let operand_kind = self.value_kind(operand.layout);
let cast_kind = self.value_kind(cast);
Expand All @@ -222,10 +224,20 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
bug!("Found {operand_kind:?} for operand {operand:?}");
};
if let OperandValueKind::Immediate(out_scalar) = cast_kind {
let cast_bty = bx.backend_type(cast);
Some(OperandValue::Immediate(Self::transmute_immediate(
bx, imm, in_scalar, out_scalar, cast_bty,
)))
match (in_scalar, out_scalar) {
(ScalarOrZst::Zst, ScalarOrZst::Zst) => {
Some(OperandRef::new_zst(bx, cast).val)
}
(ScalarOrZst::Scalar(in_scalar), ScalarOrZst::Scalar(out_scalar))
if in_scalar.size(self.cx) == out_scalar.size(self.cx) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this check necessary?

For pairs you need to check the inner size because matching size of the whole pair doesn't necessarily imply matching sizes of elements... But surely the operand.layout.size != cast.size check is sufficient for scalars? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good call. Let me give that a shot -- might allow deleting more code too...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this check, but ScalarOrZst::size is still needed elsewhere, so I couldn't remove more than that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no, vectors. Without this check <4 x i64> to <8 x f32> trips the later debug-assert.

I suppose I could hypothetically bitcast that anyway, but I'm not doing that this PR.

Added some codegen tests for vectors so that when future me breaks this while running --keep-stage-std again, I'll notice without CI 😬

{
let cast_bty = bx.backend_type(cast);
Some(OperandValue::Immediate(
self.transmute_immediate(bx, imm, in_scalar, out_scalar, cast_bty),
))
}
_ => None,
}
} else {
None
}
Expand All @@ -234,12 +246,15 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let OperandValueKind::Pair(in_a, in_b) = operand_kind else {
bug!("Found {operand_kind:?} for operand {operand:?}");
};
if let OperandValueKind::Pair(out_a, out_b) = cast_kind {
if let OperandValueKind::Pair(out_a, out_b) = cast_kind
&& in_a.size(self.cx) == out_a.size(self.cx)
&& in_b.size(self.cx) == out_b.size(self.cx)
{
let out_a_ibty = bx.scalar_pair_element_backend_type(cast, 0, false);
let out_b_ibty = bx.scalar_pair_element_backend_type(cast, 1, false);
Some(OperandValue::Pair(
Self::transmute_immediate(bx, imm_a, in_a, out_a, out_a_ibty),
Self::transmute_immediate(bx, imm_b, in_b, out_b, out_b_ibty),
self.transmute_immediate(bx, imm_a, in_a, out_a, out_a_ibty),
self.transmute_immediate(bx, imm_b, in_b, out_b, out_b_ibty),
))
} else {
None
Expand All @@ -254,12 +269,15 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
/// `to_backend_ty` must be the *non*-immediate backend type (so it will be
/// `i8`, not `i1`, for `bool`-like types.)
fn transmute_immediate(
&self,
bx: &mut Bx,
mut imm: Bx::Value,
from_scalar: abi::Scalar,
to_scalar: abi::Scalar,
to_backend_ty: Bx::Type,
) -> Bx::Value {
debug_assert_eq!(from_scalar.size(self.cx), to_scalar.size(self.cx));

use abi::Primitive::*;
imm = bx.from_immediate(imm);
imm = match (from_scalar.primitive(), to_scalar.primitive()) {
Expand Down Expand Up @@ -831,14 +849,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let operand_ty = operand.ty(self.mir, self.cx.tcx());
let cast_layout = self.cx.layout_of(self.monomorphize(cast_ty));
let operand_layout = self.cx.layout_of(self.monomorphize(operand_ty));
if operand_layout.size != cast_layout.size
|| operand_layout.abi.is_uninhabited()
|| cast_layout.abi.is_uninhabited()
{
// Send UB cases to the full form so the operand version can
// `bitcast` without worrying about malformed IR.
return false;
}

match (self.value_kind(operand_layout), self.value_kind(cast_layout)) {
// Can always load from a pointer as needed
Expand All @@ -847,9 +857,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
// Need to generate an `alloc` to get a pointer from an immediate
(OperandValueKind::Immediate(..) | OperandValueKind::Pair(..), OperandValueKind::Ref) => false,

// When we have scalar immediates, we can convert them as needed
(OperandValueKind::Immediate(..), OperandValueKind::Immediate(..)) |
(OperandValueKind::Pair(..), OperandValueKind::Pair(..)) => true,
// When we have scalar immediates, we can only convert things
// where the sizes match, to avoid endianness questions.
(OperandValueKind::Immediate(a), OperandValueKind::Immediate(b)) =>
a.size(self.cx) == b.size(self.cx),
(OperandValueKind::Pair(a0, a1), OperandValueKind::Pair(b0, b1)) =>
a0.size(self.cx) == b0.size(self.cx) && a1.size(self.cx) == b1.size(self.cx),

// Send mixings between scalars and pairs through the memory route
// FIXME: Maybe this could use insertvalue/extractvalue instead?
Expand Down Expand Up @@ -887,13 +900,18 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if self.cx.is_backend_immediate(layout) {
debug_assert!(!self.cx.is_backend_scalar_pair(layout));
OperandValueKind::Immediate(match layout.abi {
abi::Abi::Scalar(s) => s,
abi::Abi::Vector { element, .. } => element,
x => bug!("Couldn't translate {x:?} as backend immediate"),
abi::Abi::Scalar(s) => ScalarOrZst::Scalar(s),
abi::Abi::Vector { element, .. } => ScalarOrZst::Scalar(element),
_ if layout.is_zst() => ScalarOrZst::Zst,
x => span_bug!(self.mir.span, "Couldn't translate {x:?} as backend immediate"),
})
} else if self.cx.is_backend_scalar_pair(layout) {
let abi::Abi::ScalarPair(s1, s2) = layout.abi else {
bug!("Couldn't translate {:?} as backend scalar pair", layout.abi)
span_bug!(
self.mir.span,
"Couldn't translate {:?} as backend scalar pair",
layout.abi,
);
};
OperandValueKind::Pair(s1, s2)
} else {
Expand All @@ -902,9 +920,26 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
}

/// The variants of this match [`OperandValue`], giving details about the
/// backend values that will be held in that other type.
#[derive(Debug, Copy, Clone)]
enum OperandValueKind {
Ref,
Immediate(abi::Scalar),
Immediate(ScalarOrZst),
Pair(abi::Scalar, abi::Scalar),
}

#[derive(Debug, Copy, Clone)]
enum ScalarOrZst {
Zst,
Scalar(abi::Scalar),
}

impl ScalarOrZst {
pub fn size(self, cx: &impl abi::HasDataLayout) -> abi::Size {
match self {
ScalarOrZst::Zst => abi::Size::ZERO,
ScalarOrZst::Scalar(s) => s.size(cx),
}
}
}
35 changes: 35 additions & 0 deletions tests/codegen/intrinsics/transmute-x64.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// compile-flags: -O -C no-prepopulate-passes
// only-x86_64 (it's using arch-specific types)
// min-llvm-version: 15.0 # this test assumes `ptr`s

#![crate_type = "lib"]

use std::arch::x86_64::{__m128, __m128i, __m256i};
use std::mem::transmute;

// CHECK-LABEL: @check_sse_float_to_int(
#[no_mangle]
pub unsafe fn check_sse_float_to_int(x: __m128) -> __m128i {
// CHECK-NOT: alloca
// CHECK: %1 = load <4 x float>, ptr %x, align 16
// CHECK: store <4 x float> %1, ptr %0, align 16
transmute(x)
}

// CHECK-LABEL: @check_sse_pair_to_avx(
#[no_mangle]
pub unsafe fn check_sse_pair_to_avx(x: (__m128i, __m128i)) -> __m256i {
// CHECK-NOT: alloca
// CHECK: %1 = load <4 x i64>, ptr %x, align 16
// CHECK: store <4 x i64> %1, ptr %0, align 32
transmute(x)
}

// CHECK-LABEL: @check_sse_pair_from_avx(
#[no_mangle]
pub unsafe fn check_sse_pair_from_avx(x: __m256i) -> (__m128i, __m128i) {
// CHECK-NOT: alloca
// CHECK: %1 = load <4 x i64>, ptr %x, align 32
// CHECK: store <4 x i64> %1, ptr %0, align 16
transmute(x)
}
Loading