diff --git a/blas-tests/tests/oper.rs b/blas-tests/tests/oper.rs index 2741123f9..b7c2d769d 100644 --- a/blas-tests/tests/oper.rs +++ b/blas-tests/tests/oper.rs @@ -173,7 +173,7 @@ where S2: Data, { let ((m, _), k) = (lhs.dim(), rhs.dim()); - reference_mat_mul(lhs, &rhs.to_owned().into_shape((k, 1)).unwrap()) + reference_mat_mul(lhs, &rhs.as_standard_layout().into_shape((k, 1)).unwrap()) .into_shape(m) .unwrap() } @@ -186,7 +186,7 @@ where S2: Data, { let (m, (_, n)) = (lhs.dim(), rhs.dim()); - reference_mat_mul(&lhs.to_owned().into_shape((1, m)).unwrap(), rhs) + reference_mat_mul(&lhs.as_standard_layout().into_shape((1, m)).unwrap(), rhs) .into_shape(n) .unwrap() } diff --git a/src/dimension/dimension_trait.rs b/src/dimension/dimension_trait.rs index b2ef92e43..4223e9ed0 100644 --- a/src/dimension/dimension_trait.rs +++ b/src/dimension/dimension_trait.rs @@ -286,17 +286,16 @@ pub trait Dimension: return true; } if dim.ndim() == 1 { - return false; + return strides[0] as isize == -1; } let order = strides._fastest_varying_stride_order(); let strides = strides.slice(); - // FIXME: Negative strides let dim_slice = dim.slice(); let mut cstride = 1; for &i in order.slice() { // a dimension of length 1 can have unequal strides - if dim_slice[i] != 1 && strides[i] != cstride { + if dim_slice[i] != 1 && (strides[i] as isize).abs() as usize != cstride { return false; } cstride *= dim_slice[i]; @@ -307,8 +306,7 @@ pub trait Dimension: /// Return the axis ordering corresponding to the fastest variation /// (in ascending order). /// - /// Assumes that no stride value appears twice. This cannot yield the correct - /// result the strides are not positive. + /// Assumes that no stride value appears twice. #[doc(hidden)] fn _fastest_varying_stride_order(&self) -> Self { let mut indices = self.clone(); @@ -316,7 +314,9 @@ pub trait Dimension: *elt = i; } let strides = self.slice(); - indices.slice_mut().sort_by_key(|&i| strides[i]); + indices + .slice_mut() + .sort_by_key(|&i| (strides[i] as isize).abs()); indices } @@ -645,7 +645,7 @@ impl Dimension for Dim<[Ix; 2]> { #[inline] fn _fastest_varying_stride_order(&self) -> Self { - if get!(self, 0) as Ixs <= get!(self, 1) as Ixs { + if (get!(self, 0) as Ixs).abs() <= (get!(self, 1) as Ixs).abs() { Ix2(0, 1) } else { Ix2(1, 0) @@ -805,7 +805,7 @@ impl Dimension for Dim<[Ix; 3]> { let mut order = Ix3(0, 1, 2); macro_rules! swap { ($stride:expr, $order:expr, $x:expr, $y:expr) => { - if $stride[$x] > $stride[$y] { + if ($stride[$x] as isize).abs() > ($stride[$y] as isize).abs() { $stride.swap($x, $y); $order.ixm().swap($x, $y); } diff --git a/src/dimension/mod.rs b/src/dimension/mod.rs index 9ee1f44d6..f52ba889b 100644 --- a/src/dimension/mod.rs +++ b/src/dimension/mod.rs @@ -46,15 +46,12 @@ pub fn stride_offset(n: Ix, stride: Ix) -> isize { /// There is overlap if, when iterating through the dimensions in order of /// increasing stride, the current stride is less than or equal to the maximum /// possible offset along the preceding axes. (Axes of length ≤1 are ignored.) -/// -/// The current implementation assumes that strides of axes with length > 1 are -/// nonnegative. Additionally, it does not check for overflow. pub fn dim_stride_overlap(dim: &D, strides: &D) -> bool { let order = strides._fastest_varying_stride_order(); let mut sum_prev_offsets = 0; for &index in order.slice() { let d = dim[index]; - let s = strides[index] as isize; + let s = (strides[index] as isize).abs(); match d { 0 => return false, 1 => {} @@ -210,11 +207,7 @@ where /// /// 2. The product of non-zero axis lengths must not exceed `isize::MAX`. /// -/// 3. For axes with length > 1, the stride must be nonnegative. This is -/// necessary to make sure the pointer cannot move backwards outside the -/// slice. For axes with length ≤ 1, the stride can be anything. -/// -/// 4. If the array will be empty (any axes are zero-length), the difference +/// 3. If the array will be empty (any axes are zero-length), the difference /// between the least address and greatest address accessible by moving /// along all axes must be ≤ `data.len()`. (It's fine in this case to move /// one byte past the end of the slice since the pointers will be offset but @@ -225,13 +218,19 @@ where /// `data.len()`. This and #3 ensure that all dereferenceable pointers point /// to elements within the slice. /// -/// 5. The strides must not allow any element to be referenced by two different +/// 4. The strides must not allow any element to be referenced by two different /// indices. /// /// Note that since slices cannot contain more than `isize::MAX` bytes, /// condition 4 is sufficient to guarantee that the absolute difference in /// units of `A` and in units of bytes between the least address and greatest /// address accessible by moving along all axes does not exceed `isize::MAX`. +/// +/// Warning: This function is sufficient to check the invariants of ArrayBase only +/// if the pointer to the first element of the array is chosen such that the element +/// with the smallest memory address is at the start of data. (In other words, the +/// pointer to the first element of the array must be computed using offset_from_ptr_to_memory +/// so that negative strides are correctly handled.) pub(crate) fn can_index_slice( data: &[A], dim: &D, @@ -248,7 +247,7 @@ fn can_index_slice_impl( dim: &D, strides: &D, ) -> Result<(), ShapeError> { - // Check condition 4. + // Check condition 3. let is_empty = dim.slice().iter().any(|&d| d == 0); if is_empty && max_offset > data_len { return Err(from_kind(ErrorKind::OutOfBounds)); @@ -257,15 +256,7 @@ fn can_index_slice_impl( return Err(from_kind(ErrorKind::OutOfBounds)); } - // Check condition 3. - for (&d, &s) in izip!(dim.slice(), strides.slice()) { - let s = s as isize; - if d > 1 && s < 0 { - return Err(from_kind(ErrorKind::Unsupported)); - } - } - - // Check condition 5. + // Check condition 4. if !is_empty && dim_stride_overlap(dim, strides) { return Err(from_kind(ErrorKind::Unsupported)); } @@ -289,6 +280,19 @@ pub fn stride_offset_checked(dim: &[Ix], strides: &[Ix], index: &[Ix]) -> Option Some(offset) } +/// Checks if strides are non-negative. +pub fn strides_non_negative(strides: &D) -> Result<(), ShapeError> +where + D: Dimension, +{ + for &stride in strides.slice() { + if (stride as isize) < 0 { + return Err(from_kind(ErrorKind::Unsupported)); + } + } + Ok(()) +} + /// Implementation-specific extensions to `Dimension` pub trait DimensionExt { // note: many extensions go in the main trait if they need to be special- @@ -394,6 +398,19 @@ fn to_abs_slice(axis_len: usize, slice: Slice) -> (usize, usize, isize) { (start, end, step) } +/// This function computes the offset from the logically first element to the first element in +/// memory of the array. The result is always <= 0. +pub fn offset_from_ptr_to_memory(dim: &D, strides: &D) -> isize { + let offset = izip!(dim.slice(), strides.slice()).fold(0, |_offset, (d, s)| { + if (*s as isize) < 0 { + _offset + *s as isize * (*d as isize - 1) + } else { + _offset + } + }); + offset +} + /// Modify dimension, stride and return data pointer offset /// /// **Panics** if stride is 0 or if any index is out of bounds. @@ -693,13 +710,21 @@ mod test { let dim = (2, 3, 2).into_dimension(); let strides = (5, 2, 1).into_dimension(); assert!(super::dim_stride_overlap(&dim, &strides)); + let strides = (-5isize as usize, 2, -1isize as usize).into_dimension(); + assert!(super::dim_stride_overlap(&dim, &strides)); let strides = (6, 2, 1).into_dimension(); assert!(!super::dim_stride_overlap(&dim, &strides)); + let strides = (6, -2isize as usize, 1).into_dimension(); + assert!(!super::dim_stride_overlap(&dim, &strides)); let strides = (6, 0, 1).into_dimension(); assert!(super::dim_stride_overlap(&dim, &strides)); + let strides = (-6isize as usize, 0, 1).into_dimension(); + assert!(super::dim_stride_overlap(&dim, &strides)); let dim = (2, 2).into_dimension(); let strides = (3, 2).into_dimension(); assert!(!super::dim_stride_overlap(&dim, &strides)); + let strides = (3, -2isize as usize).into_dimension(); + assert!(!super::dim_stride_overlap(&dim, &strides)); } #[test] @@ -736,7 +761,7 @@ mod test { can_index_slice::(&[1], &Ix1(2), &Ix1(1)).unwrap_err(); can_index_slice::(&[1, 2], &Ix1(2), &Ix1(0)).unwrap_err(); can_index_slice::(&[1, 2], &Ix1(2), &Ix1(1)).unwrap(); - can_index_slice::(&[1, 2], &Ix1(2), &Ix1(-1isize as usize)).unwrap_err(); + can_index_slice::(&[1, 2], &Ix1(2), &Ix1(-1isize as usize)).unwrap(); } #[test] diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 4353a827e..56948caec 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -16,6 +16,7 @@ use num_traits::{Float, One, Zero}; use std::mem::MaybeUninit; use crate::dimension; +use crate::dimension::offset_from_ptr_to_memory; use crate::error::{self, ShapeError}; use crate::extension::nonnull::nonnull_from_vec_data; use crate::imp_prelude::*; @@ -24,6 +25,7 @@ use crate::indices; use crate::iterators::{to_vec, to_vec_mapped}; use crate::StrideShape; use crate::{geomspace, linspace, logspace}; +use rawpointer::PointerExt; /// # Constructor Methods for Owned Arrays /// @@ -431,7 +433,8 @@ where /// /// 2. The product of non-zero axis lengths must not exceed `isize::MAX`. /// - /// 3. For axes with length > 1, the stride must be nonnegative. + /// 3. For axes with length > 1, the pointer cannot move outside the + /// slice. /// /// 4. If the array will be empty (any axes are zero-length), the /// difference between the least address and greatest address accessible @@ -457,7 +460,7 @@ where // debug check for issues that indicates wrong use of this constructor debug_assert!(dimension::can_index_slice(&v, &dim, &strides).is_ok()); ArrayBase { - ptr: nonnull_from_vec_data(&mut v), + ptr: nonnull_from_vec_data(&mut v).offset(-offset_from_ptr_to_memory(&dim, &strides)), data: DataOwned::new(v), strides, dim, @@ -483,7 +486,7 @@ where /// /// This constructor is limited to elements where `A: Copy` (no destructors) /// to avoid users shooting themselves too hard in the foot. - /// + /// /// (Also note that the constructors `from_shape_vec` and /// `from_shape_vec_unchecked` allow the user yet more control, in the sense /// that Arrays can be created from arbitrary vectors.) diff --git a/src/impl_methods.rs b/src/impl_methods.rs index cccc195a1..3921dcbfc 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -17,7 +17,8 @@ use crate::arraytraits; use crate::dimension; use crate::dimension::IntoDimension; use crate::dimension::{ - abs_index, axes_of, do_slice, merge_axes, size_of_shape_checked, stride_offset, Axes, + abs_index, axes_of, do_slice, merge_axes, offset_from_ptr_to_memory, size_of_shape_checked, + stride_offset, Axes, }; use crate::error::{self, ErrorKind, ShapeError}; use crate::itertools::zip; @@ -153,12 +154,12 @@ where /// Return an uniquely owned copy of the array. /// - /// If the input array is contiguous and its strides are positive, then the - /// output array will have the same memory layout. Otherwise, the layout of - /// the output array is unspecified. If you need a particular layout, you - /// can allocate a new array with the desired memory layout and - /// [`.assign()`](#method.assign) the data. Alternatively, you can collect - /// an iterator, like this for a result in standard layout: + /// If the input array is contiguous, then the output array will have the same + /// memory layout. Otherwise, the layout of the output array is unspecified. + /// If you need a particular layout, you can allocate a new array with the + /// desired memory layout and [`.assign()`](#method.assign) the data. + /// Alternatively, you can collectan iterator, like this for a result in + /// standard layout: /// /// ``` /// # use ndarray::prelude::*; @@ -1280,9 +1281,6 @@ where } /// Return true if the array is known to be contiguous. - /// - /// Will detect c- and f-contig arrays correctly, but otherwise - /// There are some false negatives. pub(crate) fn is_contiguous(&self) -> bool { D::is_contiguous(&self.dim, &self.strides) } @@ -1404,14 +1402,18 @@ where /// /// If this function returns `Some(_)`, then the elements in the slice /// have whatever order the elements have in memory. - /// - /// Implementation notes: Does not yet support negatively strided arrays. pub fn as_slice_memory_order(&self) -> Option<&[A]> where S: Data, { if self.is_contiguous() { - unsafe { Some(slice::from_raw_parts(self.ptr.as_ptr(), self.len())) } + let offset = offset_from_ptr_to_memory(&self.dim, &self.strides); + unsafe { + Some(slice::from_raw_parts( + self.ptr.offset(offset).as_ptr(), + self.len(), + )) + } } else { None } @@ -1425,7 +1427,13 @@ where { if self.is_contiguous() { self.ensure_unique(); - unsafe { Some(slice::from_raw_parts_mut(self.ptr.as_ptr(), self.len())) } + let offset = offset_from_ptr_to_memory(&self.dim, &self.strides); + unsafe { + Some(slice::from_raw_parts_mut( + self.ptr.offset(offset).as_ptr(), + self.len(), + )) + } } else { None } diff --git a/src/impl_raw_views.rs b/src/impl_raw_views.rs index 8377dc93c..e3447cac5 100644 --- a/src/impl_raw_views.rs +++ b/src/impl_raw_views.rs @@ -62,6 +62,11 @@ where /// [`.offset()`] regardless of the starting point due to past offsets. /// /// * The product of non-zero axis lengths must not exceed `isize::MAX`. + /// + /// * Strides must be non-negative. + /// + /// This function can use debug assertions to check some of these requirements, + /// but it's not a complete check. /// /// [`.offset()`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.offset pub unsafe fn from_shape_ptr(shape: Sh, ptr: *const A) -> Self @@ -73,6 +78,7 @@ where if cfg!(debug_assertions) { assert!(!ptr.is_null(), "The pointer must be non-null."); if let Strides::Custom(strides) = &shape.strides { + dimension::strides_non_negative(strides).unwrap(); dimension::max_abs_offset_check_overflow::(&dim, &strides).unwrap(); } else { dimension::size_of_shape_checked(&dim).unwrap(); @@ -202,6 +208,11 @@ where /// [`.offset()`] regardless of the starting point due to past offsets. /// /// * The product of non-zero axis lengths must not exceed `isize::MAX`. + /// + /// * Strides must be non-negative. + /// + /// This function can use debug assertions to check some of these requirements, + /// but it's not a complete check. /// /// [`.offset()`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.offset pub unsafe fn from_shape_ptr(shape: Sh, ptr: *mut A) -> Self @@ -213,6 +224,7 @@ where if cfg!(debug_assertions) { assert!(!ptr.is_null(), "The pointer must be non-null."); if let Strides::Custom(strides) = &shape.strides { + dimension::strides_non_negative(strides).unwrap(); dimension::max_abs_offset_check_overflow::(&dim, &strides).unwrap(); } else { dimension::size_of_shape_checked(&dim).unwrap(); diff --git a/src/impl_views/constructors.rs b/src/impl_views/constructors.rs index bc1602af4..c6e5f9988 100644 --- a/src/impl_views/constructors.rs +++ b/src/impl_views/constructors.rs @@ -96,6 +96,11 @@ where /// /// * The product of non-zero axis lengths must not exceed `isize::MAX`. /// + /// * Strides must be non-negative. + /// + /// This function can use debug assertions to check some of these requirements, + /// but it's not a complete check. + /// /// [`.offset()`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.offset pub unsafe fn from_shape_ptr(shape: Sh, ptr: *const A) -> Self where @@ -188,6 +193,11 @@ where /// /// * The product of non-zero axis lengths must not exceed `isize::MAX`. /// + /// * Strides must be non-negative. + /// + /// This function can use debug assertions to check some of these requirements, + /// but it's not a complete check. + /// /// [`.offset()`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.offset pub unsafe fn from_shape_ptr(shape: Sh, ptr: *mut A) -> Self where diff --git a/tests/array.rs b/tests/array.rs index 3d917f72b..9523a8d21 100644 --- a/tests/array.rs +++ b/tests/array.rs @@ -1717,10 +1717,18 @@ fn to_owned_memory_order() { // input. let c = arr2(&[[1, 2, 3], [4, 5, 6]]); let mut f = c.view(); + + // transposed array f.swap_axes(0, 1); let fo = f.to_owned(); assert_eq!(f, fo); assert_eq!(f.strides(), fo.strides()); + + // negated stride axis + f.invert_axis(Axis(1)); + let fo2 = f.to_owned(); + assert_eq!(f, fo2); + assert_eq!(f.strides(), fo2.strides()); } #[test] @@ -1729,6 +1737,7 @@ fn to_owned_neg_stride() { c.slice_collapse(s![.., ..;-1]); let co = c.to_owned(); assert_eq!(c, co); + assert_eq!(c.strides(), co.strides()); } #[test] @@ -1786,6 +1795,64 @@ fn test_contiguous() { assert!(b.as_slice_memory_order().is_some()); } +#[test] +fn test_contiguous_neg_strides() { + let s = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]; + let mut a = ArrayView::from_shape((2, 3, 2).strides((1, 4, 2)), &s).unwrap(); + assert_eq!( + a, + arr3(&[[[0, 2], [4, 6], [8, 10]], [[1, 3], [5, 7], [9, 11]]]) + ); + assert!(a.as_slice_memory_order().is_some()); + + let mut b = a.slice(s![..;1, ..;-1, ..;-1]); + assert_eq!( + b, + arr3(&[[[10, 8], [6, 4], [2, 0]], [[11, 9], [7, 5], [3, 1]]]) + ); + assert!(b.as_slice_memory_order().is_some()); + + b.swap_axes(1, 2); + assert_eq!(b, arr3(&[[[10, 6, 2], [8, 4, 0]], [[11, 7, 3], [9, 5, 1]]])); + assert!(b.as_slice_memory_order().is_some()); + + b.invert_axis(Axis(0)); + assert_eq!(b, arr3(&[[[11, 7, 3], [9, 5, 1]], [[10, 6, 2], [8, 4, 0]]])); + assert!(b.as_slice_memory_order().is_some()); + + let mut c = b.reversed_axes(); + assert_eq!( + c, + arr3(&[[[11, 10], [9, 8]], [[7, 6], [5, 4]], [[3, 2], [1, 0]]]) + ); + assert!(c.as_slice_memory_order().is_some()); + + c.merge_axes(Axis(1), Axis(2)); + assert_eq!(c, arr3(&[[[11, 10, 9, 8]], [[7, 6, 5, 4]], [[3, 2, 1, 0]]])); + assert!(c.as_slice_memory_order().is_some()); + + let d = b.remove_axis(Axis(1)); + assert_eq!(d, arr2(&[[11, 7, 3], [10, 6, 2]])); + assert!(d.as_slice_memory_order().is_none()); + + let e = b.remove_axis(Axis(2)); + assert_eq!(e, arr2(&[[11, 9], [10, 8]])); + assert!(e.as_slice_memory_order().is_some()); + + let f = e.insert_axis(Axis(2)); + assert_eq!(f, arr3(&[[[11], [9]], [[10], [8]]])); + assert!(f.as_slice_memory_order().is_some()); + + let mut g = b.clone(); + g.collapse_axis(Axis(1), 0); + assert_eq!(g, arr3(&[[[11, 7, 3]], [[10, 6, 2]]])); + assert!(g.as_slice_memory_order().is_none()); + + b.collapse_axis(Axis(2), 0); + assert_eq!(b, arr3(&[[[11], [9]], [[10], [8]]])); + assert!(b.as_slice_memory_order().is_some()); +} + #[test] fn test_swap() { let mut a = arr2(&[[1, 2, 3], [4, 5, 6], [7, 8, 9]]); diff --git a/tests/dimension.rs b/tests/dimension.rs index 7e76132aa..86a7e5ed7 100644 --- a/tests/dimension.rs +++ b/tests/dimension.rs @@ -118,19 +118,36 @@ fn fastest_varying_order() { let order = strides._fastest_varying_stride_order(); assert_eq!(order.slice(), &[3, 0, 2, 1]); + let strides = Dim([-2isize as usize, 8, -4isize as usize, -1isize as usize]); + let order = strides._fastest_varying_stride_order(); + assert_eq!(order.slice(), &[3, 0, 2, 1]); + assert_eq!(Dim([1, 3])._fastest_varying_stride_order(), Dim([0, 1])); + assert_eq!( + Dim([1, -3isize as usize])._fastest_varying_stride_order(), + Dim([0, 1]) + ); assert_eq!(Dim([7, 2])._fastest_varying_stride_order(), Dim([1, 0])); + assert_eq!( + Dim([-7isize as usize, 2])._fastest_varying_stride_order(), + Dim([1, 0]) + ); assert_eq!( Dim([6, 1, 3])._fastest_varying_stride_order(), Dim([1, 2, 0]) ); + assert_eq!( + Dim([-6isize as usize, 1, -3isize as usize])._fastest_varying_stride_order(), + Dim([1, 2, 0]) + ); // it's important that it produces distinct indices. Prefer the stable order // where 0 is before 1 when they are equal. assert_eq!(Dim([2, 2])._fastest_varying_stride_order(), [0, 1]); assert_eq!(Dim([2, 2, 1])._fastest_varying_stride_order(), [2, 0, 1]); assert_eq!( - Dim([2, 2, 3, 1, 2])._fastest_varying_stride_order(), + Dim([-2isize as usize, -2isize as usize, 3, 1, -2isize as usize]) + ._fastest_varying_stride_order(), [3, 0, 1, 4, 2] ); } diff --git a/tests/oper.rs b/tests/oper.rs index 7193a49da..91205c49e 100644 --- a/tests/oper.rs +++ b/tests/oper.rs @@ -707,7 +707,7 @@ fn gen_mat_vec_mul() { S2: Data, { let ((m, _), k) = (lhs.dim(), rhs.dim()); - reference_mat_mul(lhs, &rhs.to_owned().into_shape((k, 1)).unwrap()) + reference_mat_mul(lhs, &rhs.as_standard_layout().into_shape((k, 1)).unwrap()) .into_shape(m) .unwrap() } @@ -772,7 +772,7 @@ fn vec_mat_mul() { S2: Data, { let (m, (_, n)) = (lhs.dim(), rhs.dim()); - reference_mat_mul(&lhs.to_owned().into_shape((1, m)).unwrap(), rhs) + reference_mat_mul(&lhs.as_standard_layout().into_shape((1, m)).unwrap(), rhs) .into_shape(n) .unwrap() } diff --git a/tests/raw_views.rs b/tests/raw_views.rs index b63e42926..b955cac6b 100644 --- a/tests/raw_views.rs +++ b/tests/raw_views.rs @@ -81,3 +81,18 @@ fn raw_view_deref_into_view_misaligned() { let data: [u16; 2] = [0x0011, 0x2233]; misaligned_deref(&data); } + +#[test] +#[cfg(debug_assertions)] +#[should_panic = "Unsupported"] +fn raw_view_negative_strides() { + fn misaligned_deref(data: &[u16; 2]) -> ArrayView1<'_, u16> { + let ptr: *const u16 = data.as_ptr(); + unsafe { + let raw_view = RawArrayView::from_shape_ptr(1.strides((-1isize) as usize), ptr); + raw_view.deref_into_view() + } + } + let data: [u16; 2] = [0x0011, 0x2233]; + misaligned_deref(&data); +}