diff --git a/naga/src/arena.rs b/naga/src/arena.rs index fc1283524..1bd6007c1 100644 --- a/naga/src/arena.rs +++ b/naga/src/arena.rs @@ -1,9 +1,11 @@ -use std::{cmp::Ordering, fmt, hash, marker::PhantomData, num::NonZeroU32, ops}; +use std::{cmp::Ordering, fmt, hash, marker::PhantomData, ops}; + +use crate::non_max_u32::NonMaxU32; /// An unique index in the arena array that a handle points to. -/// The "non-zero" part ensures that an `Option>` has +/// The "non-max" part ensures that an `Option>` has /// the same size and representation as `Handle`. -type Index = NonZeroU32; +type Index = NonMaxU32; use crate::{FastIndexSet, Span}; @@ -89,13 +91,12 @@ impl Handle { /// Returns the zero-based index of this handle. pub const fn index(self) -> usize { - let index = self.index.get() - 1; - index as usize + self.index.get() as usize } /// Convert a `usize` index into a `Handle`. fn from_usize(index: usize) -> Self { - let handle_index = u32::try_from(index + 1) + let handle_index = u32::try_from(index) .ok() .and_then(Index::new) .expect("Failed to insert into arena. Handle overflows"); @@ -104,7 +105,7 @@ impl Handle { /// Convert a `usize` index into a `Handle`, without range checks. const unsafe fn from_usize_unchecked(index: usize) -> Self { - Handle::new(Index::new_unchecked((index + 1) as u32)) + Handle::new(Index::new_unchecked(index as u32)) } /// Write this handle's index to `formatter`, preceded by `prefix`. @@ -174,7 +175,7 @@ impl Clone for Range { impl fmt::Debug for Range { fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - write!(formatter, "[{}..{}]", self.inner.start + 1, self.inner.end) + write!(formatter, "[{}..{}]", self.inner.start, self.inner.end - 1) } } @@ -182,9 +183,10 @@ impl Iterator for Range { type Item = Handle; fn next(&mut self) -> Option { if self.inner.start < self.inner.end { + let next = self.inner.start; self.inner.start += 1; Some(Handle { - index: NonZeroU32::new(self.inner.start).unwrap(), + index: NonMaxU32::new(next).unwrap(), marker: self.marker, }) } else { @@ -209,11 +211,10 @@ impl Range { pub fn first_and_last(&self) -> Option<(Handle, Handle)> { if self.inner.start < self.inner.end { Some(( - // `Range::new_from_bounds` expects a 1-based, start- and - // end-inclusive range, but `self.inner` is a zero-based, - // end-exclusive range. - Handle::new(Index::new(self.inner.start + 1).unwrap()), - Handle::new(Index::new(self.inner.end).unwrap()), + // `Range::new_from_bounds` expects a start- and end-inclusive + // range, but `self.inner` is an end-exclusive range. + Handle::new(Index::new(self.inner.start).unwrap()), + Handle::new(Index::new(self.inner.end - 1).unwrap()), )) } else { None @@ -417,10 +418,7 @@ impl Arena { return Ok(()); } - // `range.inner` is zero-based, but end-exclusive, so `range.inner.end` - // is actually the right one-based index for the last handle within the - // range. - let last_handle = Handle::new(range.inner.end.try_into().unwrap()); + let last_handle = Handle::new(Index::new(range.inner.end - 1).unwrap()); if self.check_contains_handle(last_handle).is_err() { return Err(BadRangeError::new(range.clone())); } @@ -436,7 +434,7 @@ impl Arena { let mut index = 0; let mut retained = 0; self.data.retain_mut(|elt| { - let handle = Handle::new(Index::new(index as u32 + 1).unwrap()); + let handle = Handle::from_usize(index); let keep = predicate(handle, elt); // Since `predicate` needs mutable access to each element, @@ -602,7 +600,7 @@ impl UniqueArena { UniqueArenaDrain { inner_elts: self.set.drain(..), inner_spans: self.span_info.drain(..), - index: Index::new(1).unwrap(), + index: Index::new(0).unwrap(), } } } @@ -636,8 +634,7 @@ impl UniqueArena { /// the item's handle and a reference to it. pub fn iter(&self) -> impl DoubleEndedIterator, &T)> { self.set.iter().enumerate().map(|(i, v)| { - let position = i + 1; - let index = unsafe { Index::new_unchecked(position as u32) }; + let index = unsafe { Index::new_unchecked(i as u32) }; (Handle::new(index), v) }) } diff --git a/naga/src/compact/handle_set_map.rs b/naga/src/compact/handle_set_map.rs index c716ca829..b0d6c2e6a 100644 --- a/naga/src/compact/handle_set_map.rs +++ b/naga/src/compact/handle_set_map.rs @@ -1,14 +1,13 @@ use crate::arena::{Arena, Handle, Range, UniqueArena}; -type Index = std::num::NonZeroU32; +type Index = crate::non_max_u32::NonMaxU32; /// A set of `Handle` values. pub struct HandleSet { - /// Bound on zero-based indexes of handles stored in this set. + /// Bound on indexes of handles stored in this set. len: usize, - /// `members[i]` is true if the handle with zero-based index `i` - /// is a member. + /// `members[i]` is true if the handle with index `i` is a member. members: bit_set::BitSet, /// This type is indexed by values of type `T`. @@ -27,8 +26,6 @@ impl HandleSet { /// Add `handle` to the set. pub fn insert(&mut self, handle: Handle) { - // Note that, oddly, `Handle::index` does not return a 1-based - // `Index`, but rather a zero-based `usize`. self.members.insert(handle.index()); } @@ -40,8 +37,6 @@ impl HandleSet { } pub fn contains(&self, handle: Handle) -> bool { - // Note that, oddly, `Handle::index` does not return a 1-based - // `Index`, but rather a zero-based `usize`. self.members.contains(handle.index()) } } @@ -66,10 +61,9 @@ impl ArenaType for UniqueArena { pub struct HandleMap { /// The indices assigned to handles in the compacted module. /// - /// If `new_index[i]` is `Some(n)`, then `n` is the 1-based - /// `Index` of the compacted `Handle` corresponding to the - /// pre-compacted `Handle` whose zero-based index is `i`. ("Clear - /// as mud.") + /// If `new_index[i]` is `Some(n)`, then `n` is the `Index` of the + /// compacted `Handle` corresponding to the pre-compacted `Handle` + /// whose index is `i`. new_index: Vec>, /// This type is indexed by values of type `T`. @@ -78,11 +72,11 @@ pub struct HandleMap { impl HandleMap { pub fn from_set(set: HandleSet) -> Self { - let mut next_index = Index::new(1).unwrap(); + let mut next_index = Index::new(0).unwrap(); Self { new_index: (0..set.len) - .map(|zero_based_index| { - if set.members.contains(zero_based_index) { + .map(|index| { + if set.members.contains(index) { // This handle will be retained in the compacted version, // so assign it a new index. let this = next_index; @@ -111,11 +105,9 @@ impl HandleMap { log::trace!( "adjusting {} handle [{}] -> [{:?}]", std::any::type_name::(), - old.index() + 1, + old.index(), self.new_index[old.index()] ); - // Note that `Handle::index` returns a zero-based index, - // but `Handle::new` accepts a 1-based `Index`. self.new_index[old.index()].map(Handle::new) } @@ -147,20 +139,18 @@ impl HandleMap { pub fn adjust_range(&self, range: &mut Range, compacted_arena: &Arena) { let mut index_range = range.zero_based_index_range(); let compacted; - // Remember that the indices we retrieve from `new_index` are 1-based - // compacted indices, but the index range we're computing is zero-based - // compacted indices. - if let Some(first1) = index_range.find_map(|i| self.new_index[i as usize]) { + if let Some(first) = index_range.find_map(|i| self.new_index[i as usize]) { // The first call to `find_map` mutated `index_range` to hold the // remainder of original range, which is exactly the range we need // to search for the new last handle. - if let Some(last1) = index_range.rev().find_map(|i| self.new_index[i as usize]) { - // Build a zero-based end-exclusive range, given one-based handle indices. - compacted = first1.get() - 1..last1.get(); + if let Some(last) = index_range.rev().find_map(|i| self.new_index[i as usize]) { + // Build an end-exclusive range, given the two included indices + // `first` and `last`. + compacted = first.get()..last.get() + 1; } else { // The range contains only a single live handle, which // we identified with the first `find_map` call. - compacted = first1.get() - 1..first1.get(); + compacted = first.get()..first.get() + 1; } } else { compacted = 0..0; diff --git a/naga/src/lib.rs b/naga/src/lib.rs index 9ececf558..5696f4445 100644 --- a/naga/src/lib.rs +++ b/naga/src/lib.rs @@ -277,6 +277,7 @@ pub mod compact; pub mod error; pub mod front; pub mod keywords; +mod non_max_u32; pub mod proc; mod span; pub mod valid; diff --git a/naga/src/non_max_u32.rs b/naga/src/non_max_u32.rs new file mode 100644 index 000000000..6c4e067e3 --- /dev/null +++ b/naga/src/non_max_u32.rs @@ -0,0 +1,111 @@ +//! [`NonMaxU32`], a 32-bit type that can represent any value except [`u32::MAX`]. +//! +//! Naga would like `Option>` to be a 32-bit value, which means we +//! need to exclude some index value for use in representing [`None`]. We could +//! have [`Handle`] store a [`NonZeroU32`], but zero is a very useful value for +//! indexing. We could have a [`Handle`] store a value one greater than its index, +//! but it turns out that it's not uncommon to want to work with [`Handle`]s' +//! indices, so that bias of 1 becomes more visible than one would like. +//! +//! This module defines the type [`NonMaxU32`], for which `Option` is +//! still a 32-bit value, but which is directly usable as a [`Handle`] index +//! type. It still uses a bias of 1 under the hood, but that fact is isolated +//! within the implementation. +//! +//! [`Handle`]: crate::arena::Handle +//! [`NonZeroU32`]: std::num::NonZeroU32 +#![allow(dead_code)] + +use std::num::NonZeroU32; + +/// An unsigned 32-bit value known not to be [`u32::MAX`]. +/// +/// A `NonMaxU32` value can represent any value in the range `0 .. u32::MAX - +/// 1`, and an `Option` is still a 32-bit value. In other words, +/// `NonMaxU32` is just like [`NonZeroU32`], except that a different value is +/// missing from the full `u32` range. +/// +/// Since zero is a very useful value in indexing, `NonMaxU32` is more useful +/// for representing indices than [`NonZeroU32`]. +/// +/// # Implementation +/// +/// A `NonMaxU32` whose value is `n` is a newtype around a [`NonZeroU32`] whose +/// value is `n + 1`. This way, the range of values `NonMaxU32` can represent, +/// `0..=u32::MAX - 1`, is mapped to the range `1..=u32::MAX`, which is the +/// range that [`NonZeroU32`] can represent. (And conversely, since [`u32`] +/// addition wraps around, the unrepresentable value [`u32::MAX`] becomes the +/// unrepresentable value `0`.) +/// +/// [`NonZeroU32`]: std::num::NonZeroU32 +#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] +#[cfg_attr(feature = "serialize", derive(serde::Serialize))] +#[cfg_attr(feature = "deserialize", derive(serde::Deserialize))] +#[cfg_attr( + any(feature = "serialize", feature = "deserialize"), + serde(transparent) +)] +#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] +pub struct NonMaxU32(NonZeroU32); + +impl NonMaxU32 { + /// Construct a [`NonMaxU32`] whose value is `n`, if possible. + pub const fn new(n: u32) -> Option { + // If `n` is `u32::MAX`, then `n.wrapping_add(1)` is `0`, + // so `NonZeroU32::new` returns `None` in exactly the case + // where we must return `None`. + match NonZeroU32::new(n.wrapping_add(1)) { + Some(non_zero) => Some(NonMaxU32(non_zero)), + None => None, + } + } + + /// Return the value of `self` as a [`u32`]. + pub const fn get(self) -> u32 { + self.0.get() - 1 + } + + /// Construct a [`NonMaxU32`] whose value is `n`. + /// + /// # Safety + /// + /// The value of `n` must not be [`u32::MAX`]. + pub const unsafe fn new_unchecked(n: u32) -> NonMaxU32 { + NonMaxU32(unsafe { NonZeroU32::new_unchecked(n + 1) }) + } + + /// Construct a [`NonMaxU32`] whose value is `index`. + /// + /// # Safety + /// + /// - The value of `index` must be strictly less than [`u32::MAX`]. + pub const unsafe fn from_usize_unchecked(index: usize) -> Self { + NonMaxU32(unsafe { NonZeroU32::new_unchecked(index as u32 + 1) }) + } + + pub fn checked_add(self, n: u32) -> Option { + // Adding `n` to `self` produces `u32::MAX` if and only if + // adding `n` to `self.0` produces `0`. So we can simply + // call `NonZeroU32::checked_add` and let its check for zero + // determine whether our add would have produced `u32::MAX`. + Some(NonMaxU32(self.0.checked_add(n)?)) + } +} + +impl std::fmt::Debug for NonMaxU32 { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.get().fmt(f) + } +} + +impl std::fmt::Display for NonMaxU32 { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.get().fmt(f) + } +} + +#[test] +fn size() { + use core::mem::size_of; + assert_eq!(size_of::>(), size_of::()); +} diff --git a/naga/src/valid/function.rs b/naga/src/valid/function.rs index ee80e4d8e..d8c479128 100644 --- a/naga/src/valid/function.rs +++ b/naga/src/valid/function.rs @@ -1474,7 +1474,7 @@ impl super::Validator { if self.flags.contains(super::ValidationFlags::EXPRESSIONS) { if let Some(unvisited) = self.needs_visit.iter().next() { - let index = std::num::NonZeroU32::new(unvisited as u32 + 1).unwrap(); + let index = crate::non_max_u32::NonMaxU32::new(unvisited as u32).unwrap(); let handle = Handle::new(index); return Err(FunctionError::UnvisitedExpression(handle) .with_span_handle(handle, &fun.expressions)); diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index 297b67dff..4d46776a7 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -5,11 +5,12 @@ use crate::{ Handle, }; +use crate::non_max_u32::NonMaxU32; use crate::{Arena, UniqueArena}; use super::ValidationError; -use std::{convert::TryInto, hash::Hash, num::NonZeroU32}; +use std::{convert::TryInto, hash::Hash}; impl super::Validator { /// Validates that all handles within `module` are: @@ -688,7 +689,7 @@ impl Handle { Ok(self) } else { let erase_handle_type = |handle: Handle<_>| { - Handle::new(NonZeroU32::new((handle.index() + 1).try_into().unwrap()).unwrap()) + Handle::new(NonMaxU32::new((handle.index()).try_into().unwrap()).unwrap()) }; Err(FwdDepError { subject: erase_handle_type(self), diff --git a/naga/tests/out/dot/quad.dot b/naga/tests/out/dot/quad.dot index 986408978..6c4a4a7e4 100644 --- a/naga/tests/out/dot/quad.dot +++ b/naga/tests/out/dot/quad.dot @@ -1,23 +1,23 @@ digraph Module { subgraph cluster_globals { label="Globals" - g0 [ shape=hexagon label="[1] Handle/'u_texture'" ] - g1 [ shape=hexagon label="[2] Handle/'u_sampler'" ] + g0 [ shape=hexagon label="[0] Handle/'u_texture'" ] + g1 [ shape=hexagon label="[1] Handle/'u_sampler'" ] } subgraph cluster_ep0 { label="Vertex/'vert_main'" node [ style=filled ] - ep0_e0 [ color="#8dd3c7" label="[1] Argument[0]" ] - ep0_e1 [ color="#8dd3c7" label="[2] Argument[1]" ] - ep0_e2 [ fillcolor="#ffffb3" label="[3] Constant" ] - ep0_e3 [ color="#fdb462" label="[4] Multiply" ] + ep0_e0 [ color="#8dd3c7" label="[0] Argument[0]" ] + ep0_e1 [ color="#8dd3c7" label="[1] Argument[1]" ] + ep0_e2 [ fillcolor="#ffffb3" label="[2] Constant" ] + ep0_e3 [ color="#fdb462" label="[3] Multiply" ] ep0_e0 -> ep0_e3 [ label="right" ] ep0_e2 -> ep0_e3 [ label="left" ] - ep0_e4 [ fillcolor="#ffffb3" label="[5] Literal" ] - ep0_e5 [ fillcolor="#ffffb3" label="[6] Literal" ] - ep0_e6 [ color="#bebada" label="[7] Compose" ] + ep0_e4 [ fillcolor="#ffffb3" label="[4] Literal" ] + ep0_e5 [ fillcolor="#ffffb3" label="[5] Literal" ] + ep0_e6 [ color="#bebada" label="[6] Compose" ] { ep0_e3 ep0_e4 ep0_e5 } -> ep0_e6 - ep0_e7 [ color="#bebada" label="[8] Compose" ] + ep0_e7 [ color="#bebada" label="[7] Compose" ] { ep0_e1 ep0_e6 } -> ep0_e7 ep0_s0 [ shape=square label="Root" ] ep0_s1 [ shape=square label="Emit" ] @@ -34,24 +34,24 @@ digraph Module { subgraph cluster_ep1 { label="Fragment/'frag_main'" node [ style=filled ] - ep1_e0 [ color="#8dd3c7" label="[1] Argument[0]" ] - ep1_e1 [ color="#ffffb3" label="[2] Global" ] + ep1_e0 [ color="#8dd3c7" label="[0] Argument[0]" ] + ep1_e1 [ color="#ffffb3" label="[1] Global" ] g0 -> ep1_e1 [fillcolor=gray] - ep1_e2 [ color="#ffffb3" label="[3] Global" ] + ep1_e2 [ color="#ffffb3" label="[2] Global" ] g1 -> ep1_e2 [fillcolor=gray] - ep1_e3 [ color="#80b1d3" label="[4] ImageSample" ] + ep1_e3 [ color="#80b1d3" label="[3] ImageSample" ] ep1_e2 -> ep1_e3 [ label="sampler" ] ep1_e1 -> ep1_e3 [ label="image" ] ep1_e0 -> ep1_e3 [ label="coordinate" ] - ep1_e4 [ color="#8dd3c7" label="[5] AccessIndex[3]" ] + ep1_e4 [ color="#8dd3c7" label="[4] AccessIndex[3]" ] ep1_e3 -> ep1_e4 [ label="base" ] - ep1_e5 [ fillcolor="#ffffb3" label="[6] Literal" ] - ep1_e6 [ color="#fdb462" label="[7] Equal" ] + ep1_e5 [ fillcolor="#ffffb3" label="[5] Literal" ] + ep1_e6 [ color="#fdb462" label="[6] Equal" ] ep1_e5 -> ep1_e6 [ label="right" ] ep1_e4 -> ep1_e6 [ label="left" ] - ep1_e7 [ color="#8dd3c7" label="[8] AccessIndex[3]" ] + ep1_e7 [ color="#8dd3c7" label="[7] AccessIndex[3]" ] ep1_e3 -> ep1_e7 [ label="base" ] - ep1_e8 [ color="#fdb462" label="[9] Multiply" ] + ep1_e8 [ color="#fdb462" label="[8] Multiply" ] ep1_e3 -> ep1_e8 [ label="right" ] ep1_e7 -> ep1_e8 [ label="left" ] ep1_s0 [ shape=square label="Root" ] @@ -87,11 +87,11 @@ digraph Module { subgraph cluster_ep2 { label="Fragment/'fs_extra'" node [ style=filled ] - ep2_e0 [ fillcolor="#ffffb3" label="[1] Literal" ] - ep2_e1 [ fillcolor="#ffffb3" label="[2] Literal" ] - ep2_e2 [ fillcolor="#ffffb3" label="[3] Literal" ] - ep2_e3 [ fillcolor="#ffffb3" label="[4] Literal" ] - ep2_e4 [ fillcolor="#bebada" label="[5] Compose" ] + ep2_e0 [ fillcolor="#ffffb3" label="[0] Literal" ] + ep2_e1 [ fillcolor="#ffffb3" label="[1] Literal" ] + ep2_e2 [ fillcolor="#ffffb3" label="[2] Literal" ] + ep2_e3 [ fillcolor="#ffffb3" label="[3] Literal" ] + ep2_e4 [ fillcolor="#bebada" label="[4] Compose" ] { ep2_e0 ep2_e1 ep2_e2 ep2_e3 } -> ep2_e4 ep2_s0 [ shape=square label="Root" ] ep2_s1 [ shape=square label="Emit" ]