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

Apply noundef attribute to all scalar types which do not permit raw init #94157

Merged
merged 2 commits into from
Feb 28, 2022
Merged

Apply noundef attribute to all scalar types which do not permit raw init #94157

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
5979b68
Apply noundef attribute to all scalar types which do not permit raw init
erikdesjardins Feb 12, 2022
45ee3fc
make pgo-branch-weights test not dependent on argument attributes
erikdesjardins Feb 27, 2022
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
5 changes: 5 additions & 0 deletions compiler/rustc_middle/src/ty/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3053,6 +3053,11 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
return;
}

// Scalars which have invalid values cannot be undef.
if !scalar.is_always_valid(self) {
attrs.set(ArgAttribute::NoUndef);
Copy link
Member

Choose a reason for hiding this comment

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

With this logic, can't we remove some of the logic from #93670 since those types also are "not always valid"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially thought the same, but then we don't put noundef on Option<&T/&mut T/Box<T>>>, since they have no invalid values.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't realize those would get the attribute.

OTOH that still means we are missing Option<NonZero*>, so maybe there's a more general way to handle this by special-casing enums? Like, if the scalar has Multiple layout, then that one scalar contains its discriminant and hence we can definitely set NoUndef? (This might miss newtypes around such enums, not sure if that's a problem.)

Copy link
Contributor Author

@erikdesjardins erikdesjardins Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You're right.

I'm a bit hesistant to add more complex logic here. I think the missing cases are fairly rare, just Option<NonZero*/NonNull> (and equivalent types), and enums with exactly 256 (or 65536, etc.) variants. In the long run we can switch to the uninit tracking added by #94527 and handle these cases without having to inspect the shape of the layout at all.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. :) I just hope we won't forget to clean up that duplication of logic in two different places ;)

}

// Only pointer types handled below.
if scalar.value != Pointer {
return;
Expand Down
4 changes: 3 additions & 1 deletion src/test/codegen/abi-repr-ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// compile-flags: -O

#![crate_type="lib"]

#[repr(i8)]
Expand All @@ -6,7 +8,7 @@ pub enum Type {
Type2 = 1
}

// CHECK: define{{( dso_local)?}} signext i8 @test()
// CHECK: define{{( dso_local)?}} noundef signext i8 @test()
#[no_mangle]
pub extern "C" fn test() -> Type {
Type::Type1
Expand Down
4 changes: 2 additions & 2 deletions src/test/codegen/call-metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
// Checks that range metadata gets emitted on calls to functions returning a
// scalar value.

// compile-flags: -C no-prepopulate-passes
// compile-flags: -O -C no-prepopulate-passes

#![crate_type = "lib"]

pub fn test() {
// CHECK: call i8 @some_true(), !range [[R0:![0-9]+]]
// CHECK: call noundef i8 @some_true(), !range [[R0:![0-9]+]]
nikic marked this conversation as resolved.
Show resolved Hide resolved
// CHECK: [[R0]] = !{i8 0, i8 3}
some_true();
}
Expand Down
50 changes: 49 additions & 1 deletion src/test/codegen/function-arguments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#![feature(rustc_attrs)]

use std::mem::MaybeUninit;
use std::num::NonZeroU64;

pub struct S {
_field: [i32; 8],
Expand All @@ -13,6 +14,11 @@ pub struct UnsafeInner {
_field: std::cell::UnsafeCell<i16>,
}

pub enum MyBool {
True,
False,
}

// CHECK: noundef zeroext i1 @boolean(i1 noundef zeroext %x)
#[no_mangle]
pub fn boolean(x: bool) -> bool {
Expand All @@ -25,6 +31,48 @@ pub fn maybeuninit_boolean(x: MaybeUninit<bool>) -> MaybeUninit<bool> {
x
}

// CHECK: noundef zeroext i1 @enum_bool(i1 noundef zeroext %x)
#[no_mangle]
pub fn enum_bool(x: MyBool) -> MyBool {
x
}

// CHECK: i8 @maybeuninit_enum_bool(i8 %x)
#[no_mangle]
pub fn maybeuninit_enum_bool(x: MaybeUninit<MyBool>) -> MaybeUninit<MyBool> {
x
}

// CHECK: noundef i32 @char(i32 noundef %x)
#[no_mangle]
pub fn char(x: char) -> char {
x
}

// CHECK: i32 @maybeuninit_char(i32 %x)
#[no_mangle]
pub fn maybeuninit_char(x: MaybeUninit<char>) -> MaybeUninit<char> {
x
}

// CHECK: i64 @int(i64 %x)
#[no_mangle]
pub fn int(x: u64) -> u64 {
x
}

// CHECK: noundef i64 @nonzero_int(i64 noundef %x)
#[no_mangle]
pub fn nonzero_int(x: NonZeroU64) -> NonZeroU64 {
x
}

// CHECK: i64 @option_nonzero_int(i64 %x)
#[no_mangle]
pub fn option_nonzero_int(x: Option<NonZeroU64>) -> Option<NonZeroU64> {
x
}

// CHECK: @readonly_borrow(i32* noalias noundef readonly align 4 dereferenceable(4) %_1)
// FIXME #25759 This should also have `nocapture`
#[no_mangle]
Expand Down Expand Up @@ -156,7 +204,7 @@ pub fn return_slice(x: &[u16]) -> &[u16] {
x
}

// CHECK: { i16, i16 } @enum_id_1(i16 %x.0, i16 %x.1)
// CHECK: { i16, i16 } @enum_id_1(i16 noundef %x.0, i16 %x.1)
#[no_mangle]
pub fn enum_id_1(x: Option<Result<u16, u16>>) -> Option<Result<u16, u16>> {
x
Expand Down
4 changes: 2 additions & 2 deletions src/test/codegen/repr-transparent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// compile-flags: -C no-prepopulate-passes
// compile-flags: -O -C no-prepopulate-passes

// ignore-riscv64 riscv64 has an i128 type used with test_Vector
// see codegen/riscv-abi for riscv functiona call tests
Expand Down Expand Up @@ -56,7 +56,7 @@ pub struct GenericPlusZst<T>(T, Zst2);
#[repr(u8)]
pub enum Bool { True, False, FileNotFound }

// CHECK: define{{( dso_local)?}}{{( zeroext)?}} i8 @test_Gpz(i8{{( zeroext)?}} %_1)
// CHECK: define{{( dso_local)?}} noundef{{( zeroext)?}} i8 @test_Gpz(i8 noundef{{( zeroext)?}} %_1)
#[no_mangle]
pub extern "C" fn test_Gpz(_: GenericPlusZst<Bool>) -> GenericPlusZst<Bool> { loop {} }

Expand Down
4 changes: 2 additions & 2 deletions src/test/run-make-fulldeps/pgo-branch-weights/filecheck-patterns.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
# First, establish that certain !prof labels are attached to the expected
# functions and branching instructions.

CHECK: define void @function_called_twice(i32 %c) {{.*}} !prof [[function_called_twice_id:![0-9]+]] {
CHECK: define void @function_called_twice(i32 {{.*}} !prof [[function_called_twice_id:![0-9]+]] {
CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !prof [[branch_weights0:![0-9]+]]

CHECK: define void @function_called_42_times(i32 %c) {{.*}} !prof [[function_called_42_times_id:![0-9]+]] {
CHECK: define void @function_called_42_times(i32{{.*}} %c) {{.*}} !prof [[function_called_42_times_id:![0-9]+]] {
CHECK: switch i32 %c, label {{.*}} [
CHECK-NEXT: i32 97, label {{.*}}
CHECK-NEXT: i32 98, label {{.*}}
Expand Down