Skip to content

Commit d4f7856

Browse files
borsbors
committed
Auto merge of #155473 - scottmcm:tweak_niche_assignment, r=chenyukang,mati865
Prefer `-1` for `None` Currently we pick "weird" numbers like `1114112` for `None::<char>`. While that's not *wrong*, it's kinda *unnatural* -- a human wouldn't make that choice. This PR instead picks `-1` for thinge like `None::<char>` -- like [clang's `WEOF`](https://github.com/llvm/llvm-project/blob/63ae74b78a11f6c61136dbc445652929389eb9ab/libc/include/llvm-libc-macros/wchar-macros.h#L15) -- and `None::<bool>` and such. Any enums with more than one niched value (so not `Result` nor `Option`) remain as they were before. Also we continue to use `0` when that's possible -- `-1` is only preferred when zero *isn't* possible. --- Inspired when someone in discord posted an example like this <https://rust.godbolt.org/z/W94s9qdYW> and I thought it was odd that we're currently picking `-9223372036854775808` to be the value to store to mark an `Option<Vec<_>>` as `None`. (Especially since that needs an 8-byte immediate on x64, and writing `-1` is only a 4-byte immediate.)
2 parents c7fe5e9 + ba1a33e commit d4f7856

10 files changed

Lines changed: 296 additions & 18 deletions

File tree

‎compiler/rustc_abi/src/lib.rs‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2054,8 +2054,7 @@ impl Niche {
20542054
assert!(size.bits() <= 128);
20552055
let max_value = size.unsigned_int_max();
20562056

2057-
let niche = v.end.wrapping_add(1)..v.start;
2058-
let available = niche.end.wrapping_sub(niche.start) & max_value;
2057+
let available = v.start.wrapping_sub(v.end).wrapping_sub(1) & max_value;
20592058
if count > available {
20602059
return None;
20612060
}
@@ -2083,7 +2082,17 @@ impl Niche {
20832082
Some((start, Scalar::Initialized { value, valid_range: v.with_end(end) }))
20842083
};
20852084
let distance_end_zero = max_value - v.end;
2086-
if v.start > v.end {
2085+
// FIXME: this ought to work for `bool` too, but that seems to be hitting a miscompilation
2086+
// <https://github.com/rust-lang/rust/pull/155473#issuecomment-4302036343>
2087+
if count == 1 && v != (WrappingRange { start: 0, end: 1 }) {
2088+
// We only need one, so just pick the one closest to zero.
2089+
// Not only does that obviously use zero if it's possible, but it also
2090+
// simplifies testing things like `Option<char>`, since looking for `-1`
2091+
// is easier than looking for `1114112` (and matches clang's `WEOF`).
2092+
let next_up = size.sign_extend(v.end.wrapping_add(1)).unsigned_abs();
2093+
let next_down = size.sign_extend(v.start.wrapping_sub(1)).unsigned_abs();
2094+
if next_down <= next_up { move_start(v) } else { move_end(v) }
2095+
} else if v.start > v.end {
20872096
// zero is unavailable because wrapping occurs
20882097
move_end(v)
20892098
} else if v.start <= distance_end_zero {

‎tests/codegen-llvm/enum/enum-aggregate.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
//@ only-64bit
33

44
#![crate_type = "lib"]
5+
#![feature(pattern_types, pattern_type_macro)]
56

67
use std::cmp::Ordering;
78
use std::num::NonZero;
@@ -123,3 +124,14 @@ fn make_fully_uninhabited_result(v: u32, n: Never) -> Result<(u32, Never), (Neve
123124
}
124125

125126
enum Never {}
127+
128+
#[repr(transparent)]
129+
struct NewtypeIndex(std::pat::pattern_type!(u32 is 0..=0xFFFFFF00));
130+
131+
#[no_mangle]
132+
pub fn make_none_newtype_index() -> Option<NewtypeIndex> {
133+
// CHECK-LABEL: @make_none_newtype_index
134+
// CHECK-NEXT: start:
135+
// CHECK-NEXT: ret i32 -1
136+
None
137+
}

‎tests/codegen-llvm/enum/enum-discriminant-eq.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ pub fn opt_bool_eq_discr(a: Option<bool>, b: Option<bool>) -> bool {
3636
#[unsafe(no_mangle)]
3737
pub fn opt_ord_eq_discr(a: Option<Ordering>, b: Option<Ordering>) -> bool {
3838
// CHECK-LABEL: @opt_ord_eq_discr(
39-
// CHECK: %[[A:.+]] = icmp ne i8 %a, 2
40-
// CHECK: %[[B:.+]] = icmp eq i8 %b, 2
39+
// CHECK: %[[A:.+]] = icmp ne i8 %a, -2
40+
// CHECK: %[[B:.+]] = icmp eq i8 %b, -2
4141
// CHECK: %[[R:.+]] = xor i1 %[[A]], %[[B]]
4242
// CHECK: ret i1 %[[R]]
4343

@@ -58,8 +58,8 @@ pub fn opt_nz32_eq_discr(a: Option<NonZero<u32>>, b: Option<NonZero<u32>>) -> bo
5858
#[unsafe(no_mangle)]
5959
pub fn opt_ac_eq_discr(a: Option<AC>, b: Option<AC>) -> bool {
6060
// CHECK-LABEL: @opt_ac_eq_discr(
61-
// CHECK: %[[A:.+]] = icmp ne i8 %a, -128
62-
// CHECK: %[[B:.+]] = icmp eq i8 %b, -128
61+
// CHECK: %[[A:.+]] = icmp ne i8 %a, -1
62+
// CHECK: %[[B:.+]] = icmp eq i8 %b, -1
6363
// CHECK: %[[R:.+]] = xor i1 %[[A]], %[[B]]
6464
// CHECK: ret i1 %[[R]]
6565

‎tests/codegen-llvm/enum/enum-two-variants-match.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ use std::cmp::Ordering::{self, *};
8181
#[no_mangle]
8282
pub fn option_ordering_match(x: Option<Ordering>) -> char {
8383
// CHECK: %[[RAW:.+]] = load i8, ptr %x
84-
// CHECK: %[[IS_NONE:.+]] = icmp eq i8 %[[RAW]], 2
84+
// CHECK: %[[IS_NONE:.+]] = icmp eq i8 %[[RAW]], -2
8585
// CHECK: %[[OPT_DISCR:.+]] = select i1 %[[IS_NONE]], i64 0, i64 1
8686
// CHECK: %[[OPT_DISCR_T:.+]] = trunc nuw i64 %[[OPT_DISCR]] to i1
8787
// CHECK: br i1 %[[OPT_DISCR_T]], label %[[BB_SOME:.+]], label %[[BB_NONE:.+]]

‎tests/codegen-llvm/option-niche-eq.rs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,3 +82,14 @@ pub fn bool_eq(l: Option<bool>, r: Option<bool>) -> bool {
8282
// CHECK-NEXT: ret i1
8383
l == r
8484
}
85+
86+
// CHECK-LABEL: @bool_ref_eq
87+
#[no_mangle]
88+
pub fn bool_ref_eq(l: &Option<bool>, r: &Option<bool>) -> bool {
89+
// CHECK: start:
90+
// CHECK-NEXT: load i8
91+
// CHECK-NEXT: load i8
92+
// CHECK-NEXT: icmp eq i8
93+
// CHECK-NEXT: ret i1
94+
l == r
95+
}

‎tests/ui/layout/debug.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,3 +88,9 @@ union EmptyUnion {} //~ ERROR: has an unknown layout
8888
// (this error is never emitted to users).
8989
#[rustc_dump_layout(debug)]
9090
type TooGeneric<T> = T; //~ ERROR: does not have a fixed layout
91+
92+
#[rustc_dump_layout(debug)]
93+
type OptBool = Option<bool>; //~ ERROR: layout_of
94+
95+
#[rustc_dump_layout(debug)]
96+
type OptChar = Option<char>; //~ ERROR: layout_of

‎tests/ui/layout/debug.stderr‎

Lines changed: 241 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -614,6 +614,246 @@ error: the type `T` does not have a fixed layout
614614
LL | type TooGeneric<T> = T;
615615
| ^^^^^^^^^^^^^^^^^^
616616

617-
error: aborting due to 20 previous errors
617+
error: layout_of(Option<bool>) = Layout {
618+
size: Size(1 bytes),
619+
align: AbiAlign {
620+
abi: Align(1 bytes),
621+
},
622+
backend_repr: Scalar(
623+
Initialized {
624+
value: Int(
625+
I8,
626+
false,
627+
),
628+
valid_range: 0..=2,
629+
},
630+
),
631+
fields: Arbitrary {
632+
offsets: [
633+
Size(0 bytes),
634+
],
635+
in_memory_order: [
636+
0,
637+
],
638+
},
639+
largest_niche: Some(
640+
Niche {
641+
offset: Size(0 bytes),
642+
value: Int(
643+
I8,
644+
false,
645+
),
646+
valid_range: 0..=2,
647+
},
648+
),
649+
uninhabited: false,
650+
variants: Multiple {
651+
tag: Initialized {
652+
value: Int(
653+
I8,
654+
false,
655+
),
656+
valid_range: 0..=2,
657+
},
658+
tag_encoding: Niche {
659+
untagged_variant: 1,
660+
niche_variants: 0..=0,
661+
niche_start: 2,
662+
},
663+
tag_field: 0,
664+
variants: [
665+
Layout {
666+
size: Size(0 bytes),
667+
align: AbiAlign {
668+
abi: Align(1 bytes),
669+
},
670+
backend_repr: Memory {
671+
sized: true,
672+
},
673+
fields: Arbitrary {
674+
offsets: [],
675+
in_memory_order: [],
676+
},
677+
largest_niche: None,
678+
uninhabited: false,
679+
variants: Single {
680+
index: 0,
681+
},
682+
max_repr_align: None,
683+
unadjusted_abi_align: Align(1 bytes),
684+
randomization_seed: $SEED,
685+
},
686+
Layout {
687+
size: Size(1 bytes),
688+
align: AbiAlign {
689+
abi: Align(1 bytes),
690+
},
691+
backend_repr: Scalar(
692+
Initialized {
693+
value: Int(
694+
I8,
695+
false,
696+
),
697+
valid_range: 0..=1,
698+
},
699+
),
700+
fields: Arbitrary {
701+
offsets: [
702+
Size(0 bytes),
703+
],
704+
in_memory_order: [
705+
0,
706+
],
707+
},
708+
largest_niche: Some(
709+
Niche {
710+
offset: Size(0 bytes),
711+
value: Int(
712+
I8,
713+
false,
714+
),
715+
valid_range: 0..=1,
716+
},
717+
),
718+
uninhabited: false,
719+
variants: Single {
720+
index: 1,
721+
},
722+
max_repr_align: None,
723+
unadjusted_abi_align: Align(1 bytes),
724+
randomization_seed: $SEED,
725+
},
726+
],
727+
},
728+
max_repr_align: None,
729+
unadjusted_abi_align: Align(1 bytes),
730+
randomization_seed: $SEED,
731+
}
732+
--> $DIR/debug.rs:93:1
733+
|
734+
LL | type OptBool = Option<bool>;
735+
| ^^^^^^^^^^^^
736+
737+
error: layout_of(Option<char>) = Layout {
738+
size: Size(4 bytes),
739+
align: AbiAlign {
740+
abi: Align(4 bytes),
741+
},
742+
backend_repr: Scalar(
743+
Initialized {
744+
value: Int(
745+
I32,
746+
false,
747+
),
748+
valid_range: (..=1114111) | (4294967295..),
749+
},
750+
),
751+
fields: Arbitrary {
752+
offsets: [
753+
Size(0 bytes),
754+
],
755+
in_memory_order: [
756+
0,
757+
],
758+
},
759+
largest_niche: Some(
760+
Niche {
761+
offset: Size(0 bytes),
762+
value: Int(
763+
I32,
764+
false,
765+
),
766+
valid_range: (..=1114111) | (4294967295..),
767+
},
768+
),
769+
uninhabited: false,
770+
variants: Multiple {
771+
tag: Initialized {
772+
value: Int(
773+
I32,
774+
false,
775+
),
776+
valid_range: (..=1114111) | (4294967295..),
777+
},
778+
tag_encoding: Niche {
779+
untagged_variant: 1,
780+
niche_variants: 0..=0,
781+
niche_start: 4294967295,
782+
},
783+
tag_field: 0,
784+
variants: [
785+
Layout {
786+
size: Size(0 bytes),
787+
align: AbiAlign {
788+
abi: Align(1 bytes),
789+
},
790+
backend_repr: Memory {
791+
sized: true,
792+
},
793+
fields: Arbitrary {
794+
offsets: [],
795+
in_memory_order: [],
796+
},
797+
largest_niche: None,
798+
uninhabited: false,
799+
variants: Single {
800+
index: 0,
801+
},
802+
max_repr_align: None,
803+
unadjusted_abi_align: Align(1 bytes),
804+
randomization_seed: $SEED,
805+
},
806+
Layout {
807+
size: Size(4 bytes),
808+
align: AbiAlign {
809+
abi: Align(4 bytes),
810+
},
811+
backend_repr: Scalar(
812+
Initialized {
813+
value: Int(
814+
I32,
815+
false,
816+
),
817+
valid_range: 0..=1114111,
818+
},
819+
),
820+
fields: Arbitrary {
821+
offsets: [
822+
Size(0 bytes),
823+
],
824+
in_memory_order: [
825+
0,
826+
],
827+
},
828+
largest_niche: Some(
829+
Niche {
830+
offset: Size(0 bytes),
831+
value: Int(
832+
I32,
833+
false,
834+
),
835+
valid_range: 0..=1114111,
836+
},
837+
),
838+
uninhabited: false,
839+
variants: Single {
840+
index: 1,
841+
},
842+
max_repr_align: None,
843+
unadjusted_abi_align: Align(4 bytes),
844+
randomization_seed: $SEED,
845+
},
846+
],
847+
},
848+
max_repr_align: None,
849+
unadjusted_abi_align: Align(4 bytes),
850+
randomization_seed: $SEED,
851+
}
852+
--> $DIR/debug.rs:96:1
853+
|
854+
LL | type OptChar = Option<char>;
855+
| ^^^^^^^^^^^^
856+
857+
error: aborting due to 22 previous errors
618858

619859
For more information about this error, try `rustc --explain E0277`.

‎tests/ui/layout/zero-sized-array-enum-niche.stderr‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,7 @@ error: layout_of(Result<[u32; 0], Packed<U16IsZero>>) = Layout {
370370
I16,
371371
false,
372372
),
373-
valid_range: 0..=1,
373+
valid_range: (..=0) | (65535..),
374374
},
375375
),
376376
uninhabited: false,
@@ -380,12 +380,12 @@ error: layout_of(Result<[u32; 0], Packed<U16IsZero>>) = Layout {
380380
I16,
381381
false,
382382
),
383-
valid_range: 0..=1,
383+
valid_range: (..=0) | (65535..),
384384
},
385385
tag_encoding: Niche {
386386
untagged_variant: 1,
387387
niche_variants: 0..=0,
388-
niche_start: 1,
388+
niche_start: 65535,
389389
},
390390
tag_field: 0,
391391
variants: [

‎tests/ui/type/pattern_types/non_null.stderr‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ error: layout_of(Option<(*const ()) is !null>) = Layout {
5757
0,
5858
),
5959
),
60-
valid_range: (..=0) | (1..),
60+
valid_range: 0..=18446744073709551615,
6161
},
6262
),
6363
fields: Arbitrary {
@@ -77,7 +77,7 @@ error: layout_of(Option<(*const ()) is !null>) = Layout {
7777
0,
7878
),
7979
),
80-
valid_range: (..=0) | (1..),
80+
valid_range: 0..=18446744073709551615,
8181
},
8282
tag_encoding: Niche {
8383
untagged_variant: 1,

0 commit comments

Comments
 (0)