Pick the largest niche even if the largest niche is wrapped around

Author: oli-obk

compiler/rustc_abi/src/layout.rs

@@ -1,11 +1,12 @@
+use std::collections::BTreeSet;
 use std::fmt::{self, Write};
 use std::ops::{Bound, Deref};
 use std::{cmp, iter};
 
 use rustc_hashes::Hash64;
 use rustc_index::Idx;
 use rustc_index::bit_set::BitMatrix;
-use tracing::debug;
+use tracing::{debug, trace};
 
 use crate::{
     AbiAlign, Align, BackendRepr, FieldsShape, HasDataLayout, IndexSlice, IndexVec, Integer,
@@ -766,30 +767,42 @@ impl<Cx: HasDataLayout> LayoutCalculator<Cx> {
 
         let niche_filling_layout = calculate_niche_filling_layout();
 
-        let (mut min, mut max) = (i128::MAX, i128::MIN);
         let discr_type = repr.discr_type();
-        let bits = Integer::from_attr(dl, discr_type).size().bits();
-        for (i, mut val) in discriminants {
-            if !repr.c() && variants[i].iter().any(|f| f.is_uninhabited()) {
-                continue;
-            }
-            if discr_type.is_signed() {
-                // sign extend the raw representation to be an i128
-                val = (val << (128 - bits)) >> (128 - bits);
-            }
-            if val < min {
-                min = val;
-            }
-            if val > max {
-                max = val;
-            }
-        }
-        // We might have no inhabited variants, so pretend there's at least one.
-        if (min, max) == (i128::MAX, i128::MIN) {
-            min = 0;
-            max = 0;
-        }
-        assert!(min <= max, "discriminant range is {min}...{max}");
+        let discr_int = Integer::from_attr(dl, discr_type);
+        let bits = discr_int.size().bits();
+        // Because we can only represent one range of valid values, we'll look for the
+        // largest range of invalid values and pick everything else as the range of valid
+        // values.
+
+        // First we need to sort the possible discriminant values so that we can look for the largest gap:
+        let valid_discriminants: BTreeSet<i128> = discriminants
+            .filter(|&(i, _)| repr.c() || variants[i].iter().all(|f| !f.is_uninhabited()))
+            .map(|(_, val)| {
+                if discr_type.is_signed() {
+                    // sign extend the raw representation to be an i128
+                    (val << (128 - bits)) >> (128 - bits)
+                } else {
+                    val
+                }
+            })
+            .collect();
+        trace!(?valid_discriminants);
+        let discriminants = valid_discriminants.iter().copied();
+        let next_discriminants =
+            discriminants.clone().chain(valid_discriminants.first().copied()).skip(1);
+        // Iterate over pairs of each discriminant together with the next one.
+        // Since they were sorted, we can now compute the niche sizes and pick the largest.
+        let discriminants = discriminants.zip(next_discriminants);
+        let largest_niche = discriminants.max_by_key(|&(start, end)| {
+            if start <= end { end - start } else { discr_int.signed_max() - (start - end) }
+        });
+        trace!(?largest_niche);
+
+        // `max` is the last valid discriminant before the largest niche
+        // `min` is the first valid discriminant after the largest niche
+        let (max, min) = largest_niche
+            // We might have no inhabited variants, so pretend there's at least one.
+            .unwrap_or((0, 0));
         let (min_ity, signed) = discr_range_of_repr(min, max); //Integer::repr_discr(tcx, ty, &repr, min, max);
 
         let mut align = dl.aggregate_align;

compiler/rustc_abi/src/lib.rs

@@ -1205,6 +1205,19 @@ impl Integer {
         }
     }
 
+    /// Returns the smallest signed value that can be represented by this Integer.
+    #[inline]
+    pub fn signed_min(self) -> i128 {
+        use Integer::*;
+        match self {
+            I8 => i8::MIN as i128,
+            I16 => i16::MIN as i128,
+            I32 => i32::MIN as i128,
+            I64 => i64::MIN as i128,
+            I128 => i128::MIN,
+        }
+    }
+
     /// Finds the smallest Integer type which can represent the signed value.
     #[inline]
     pub fn fit_signed(x: i128) -> Integer {

compiler/rustc_middle/src/ty/layout.rs

@@ -107,8 +107,8 @@ impl abi::Integer {
             abi::Integer::I8
         };
 
-        // If there are no negative values, we can use the unsigned fit.
-        if min >= 0 {
+        // Pick the smallest fit.
+        if unsigned_fit <= signed_fit {
             (cmp::max(unsigned_fit, at_least), false)
         } else {
             (cmp::max(signed_fit, at_least), true)

tests/ui/enum-discriminant/wrapping_niche.stderr

@@ -9,7 +9,7 @@ error: layout_of(UnsignedAroundZero) = Layout {
                        I16,
                        false,
                    ),
-                   valid_range: 0..=65535,
+                   valid_range: (..=1) | (65535..),
                },
            ),
            fields: Arbitrary {
@@ -20,15 +20,24 @@ error: layout_of(UnsignedAroundZero) = Layout {
                    0,
                ],
            },
-           largest_niche: None,
+           largest_niche: Some(
+               Niche {
+                   offset: Size(0 bytes),
+                   value: Int(
+                       I16,
+                       false,
+                   ),
+                   valid_range: (..=1) | (65535..),
+               },
+           ),
            uninhabited: false,
            variants: Multiple {
                tag: Initialized {
                    value: Int(
                        I16,
                        false,
                    ),
-                   valid_range: 0..=65535,
+                   valid_range: (..=1) | (65535..),
                },
                tag_encoding: Direct,
                tag_field: 0,

tests/ui/transmutability/enums/niche_optimization.rs

@@ -75,8 +75,8 @@ fn one_niche() {
 
     assert::is_transmutable::<OptionLike, u8>();
     assert::is_transmutable::<V0, OptionLike>();
+    assert::is_transmutable::<V1, OptionLike>();
     assert::is_transmutable::<V254, OptionLike>();
-    assert::is_transmutable::<V255, OptionLike>();
 }
 
 fn one_niche_alt() {
@@ -97,9 +97,9 @@ fn one_niche_alt() {
     };
 
     assert::is_transmutable::<OptionLike, u8>();
-    assert::is_transmutable::<V0, OptionLike>();
+    assert::is_transmutable::<V1, OptionLike>();
+    assert::is_transmutable::<V2, OptionLike>();
     assert::is_transmutable::<V254, OptionLike>();
-    assert::is_transmutable::<V255, OptionLike>();
 }
 
 fn two_niche() {
@@ -121,9 +121,9 @@ fn two_niche() {
 
     assert::is_transmutable::<OptionLike, u8>();
     assert::is_transmutable::<V0, OptionLike>();
+    assert::is_transmutable::<V1, OptionLike>();
+    assert::is_transmutable::<V2, OptionLike>();
     assert::is_transmutable::<V253, OptionLike>();
-    assert::is_transmutable::<V254, OptionLike>();
-    assert::is_transmutable::<V255, OptionLike>();
 }
 
 fn no_niche() {
@@ -142,7 +142,7 @@ fn no_niche() {
     }
 
     const _: () = {
-        assert!(std::mem::size_of::<OptionLike>() == 2);
+        assert!(std::mem::size_of::<OptionLike>() == 1);
     };
 
     #[repr(C)]