Skip to content

Add noundef metadata for fits-in-target-pointer-size array immediate arguments #123425

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 6 commits into from
Closed
Show file tree
Hide file tree
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
33 changes: 31 additions & 2 deletions compiler/rustc_ty_utils/src/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use rustc_middle::ty::{self, InstanceDef, Ty, TyCtxt};
use rustc_session::config::OptLevel;
use rustc_span::def_id::DefId;
use rustc_target::abi::call::{
ArgAbi, ArgAttribute, ArgAttributes, ArgExtension, Conv, FnAbi, PassMode, Reg, RegKind,
RiscvInterruptKind,
ArgAbi, ArgAttribute, ArgAttributes, ArgExtension, CastTarget, Conv, FnAbi, PassMode, Reg,
RegKind, RiscvInterruptKind,
};
use rustc_target::abi::*;
use rustc_target::spec::abi::Abi as SpecAbi;
Expand Down Expand Up @@ -784,6 +784,21 @@ fn fn_abi_adjust_for_abi<'tcx>(
// an LLVM aggregate type for this leads to bad optimizations,
// so we pick an appropriately sized integer type instead.
arg.cast_to(Reg { kind: RegKind::Integer, size });

// Let's see if we can add a `noundef`. This is only legal for arrays, definitely
Copy link
Member

Choose a reason for hiding this comment

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

I think the elephant in the room here is padding, isn't it? Arrays are fine as they do not have any padding. But the word "padding" does not appear anywhere in the PR diff or description, so I am confused.

// not for unions. This is also legal for `#[repr(transparent)] struct` or
// `#[repr(transparent)] enum` containing array. Note that `#[repr(transparent)]`
// can contain other `#[repr(transparent)]` structs or enums, which can eventually
// contain an array!
if arg.layout.ty.is_array() || is_transparent_array(cx, arg.layout) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why arrays would be special here. What if this is an array of unions, or something like that?

// Fixup arg attribute with `noundef`.
let PassMode::Cast { ref mut cast, .. } = &mut arg.mode else {
bug!("this cannot fail because of the previous cast_to `Reg`");
};

let box CastTarget { ref mut attrs, .. } = cast;
attrs.set(ArgAttribute::NoUndef);
}
}

// If we deduced that this parameter was read-only, add that to the attribute list now.
Expand Down Expand Up @@ -819,6 +834,20 @@ fn fn_abi_adjust_for_abi<'tcx>(
Ok(())
}

fn is_transparent_array<'tcx>(
cx: &LayoutCx<'tcx, TyCtxt<'tcx>>,
outermost_layout: TyAndLayout<'tcx>,
) -> bool {
let mut adt_layout = outermost_layout;
// Recursively walk a layout, seeing through all `#[repr(transparent)]` layers.
while adt_layout.is_transparent::<LayoutCx<'tcx, TyCtxt<'tcx>>>()
&& let Some((_, layout)) = adt_layout.non_1zst_field(cx)
Copy link
Member

Choose a reason for hiding this comment

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

Note that unions can be transparent, so wouldn't this do the wrong thing then?

Copy link
Member

Choose a reason for hiding this comment

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

At least for structs, there's not really any need to check for is_transparent. This is an optimization so we can just make use of the fact that a struct is a wrapper (i.e., non_1zst_field returns Some) even without a repr.

{
adt_layout = layout;
}
adt_layout.ty.is_array()
}

#[tracing::instrument(level = "debug", skip(cx))]
fn make_thin_self_ptr<'tcx>(
cx: &(impl HasTyCtxt<'tcx> + HasParamEnv<'tcx>),
Expand Down
125 changes: 125 additions & 0 deletions tests/codegen/array-immediate-param-noundef.rs
Copy link
Member

@RalfJung RalfJung Apr 16, 2024

Choose a reason for hiding this comment

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

This testcase is missing negative tests with transparent unions, and arrays of unions, to ensure they do not get the attribute.

Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
// Check that small array immediates that fits in target pointer size in argument position have
// `noundef` parameter metadata. Note that the `noundef` parameter metadata is only applied if:
// - `!arg.layout.is_unsized() && size <= Pointer(AddressSpace::DATA).size(cx)`
// - optimizations are turned on.
//
//@ only-64bit (presence of noundef depends on pointer width)
//@ compile-flags: -C no-prepopulate-passes -O
#![crate_type = "lib"]

// CHECK: define noundef i64 @short_array_u64x1(i64 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u64x1(v: [u64; 1]) -> [u64; 1] {
v
}

// CHECK: define noundef i32 @short_array_u32x1(i32 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u32x1(v: [u32; 1]) -> [u32; 1] {
v
}

// CHECK: define noundef i64 @short_array_u32x2(i64 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u32x2(v: [u32; 2]) -> [u32; 2] {
v
}

// CHECK: define noundef i16 @short_array_u16x1(i16 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u16x1(v: [u16; 1]) -> [u16; 1] {
v
}

// CHECK: define noundef i32 @short_array_u16x2(i32 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u16x2(v: [u16; 2]) -> [u16; 2] {
v
}

// CHECK: define noundef i48 @short_array_u16x3(i48 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u16x3(v: [u16; 3]) -> [u16; 3] {
v
}

// CHECK: define noundef i64 @short_array_u16x4(i64 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u16x4(v: [u16; 4]) -> [u16; 4] {
v
}

// CHECK: define noundef i8 @short_array_u8x1(i8 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u8x1(v: [u8; 1]) -> [u8; 1] {
v
}

// CHECK: define noundef i16 @short_array_u8x2(i16 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u8x2(v: [u8; 2]) -> [u8; 2] {
v
}

// CHECK: define noundef i24 @short_array_u8x3(i24 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u8x3(v: [u8; 3]) -> [u8; 3] {
v
}

// CHECK: define noundef i64 @short_array_u8x8(i64 noundef %{{.*}})
#[no_mangle]
pub fn short_array_u8x8(v: [u8; 8]) -> [u8; 8] {
v
}

#[repr(transparent)]
pub struct Foo([u8; 4]);

// CHECK: define noundef i32 @repr_transparent_struct_short_array(i32 noundef %{{.*}})
#[no_mangle]
pub fn repr_transparent_struct_short_array(v: Foo) -> Foo {
v
}

#[repr(transparent)]
pub enum Bar {
Default([u8; 4]),
}

// CHECK: define noundef i32 @repr_transparent_enum_short_array(i32 noundef %{{.*}})
#[no_mangle]
pub fn repr_transparent_enum_short_array(v: Bar) -> Bar {
v
}

#[repr(transparent)]
pub struct Owo([u8; 4]);

#[repr(transparent)]
pub struct Uwu(Owo);

#[repr(transparent)]
pub struct Oowoo(Uwu);

// CHECK: define noundef i32 @repr_transparent_nested_struct_short_array(i32 noundef %{{.*}})
#[no_mangle]
pub fn repr_transparent_nested_struct_short_array(v: Oowoo) -> Oowoo {
v
}

// # Negative examples

// This inner struct is *not* `#[repr(transparent)]`, so we must not emit `noundef` for the outer
Copy link
Member

Choose a reason for hiding this comment

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

Why "must not"? With the layout we pick this would still be correct.

// struct.
pub struct NotTransparent([u8; 4]);

#[repr(transparent)]
pub struct Transparent(NotTransparent);

// CHECK-LABEL: not_all_transparent_nested_struct_short_array
// CHECK-NOT: noundef
#[no_mangle]
pub fn not_all_transparent_nested_struct_short_array(v: Transparent) -> Transparent {
v
}