Core: Fix reshape traversal order and unsafe packet assignments libeigen/eigen!3167
diff --git a/Eigen/src/Core/AssignEvaluator.h b/Eigen/src/Core/AssignEvaluator.h index 67b2a1a..0008a94 100644 --- a/Eigen/src/Core/AssignEvaluator.h +++ b/Eigen/src/Core/AssignEvaluator.h
@@ -94,10 +94,12 @@ static constexpr bool StorageOrdersAgree = DstIsRowMajor == SrcIsRowMajor; static constexpr bool MightVectorize = StorageOrdersAgree && bool(DstFlags & SrcFlags & ActualPacketAccessBit) && bool(functor_traits<AssignFunc>::PacketAccess); - static constexpr bool MayInnerVectorize = MightVectorize && (InnerSizeAtCompileTime != Dynamic) && - (InnerSizeAtCompileTime % InnerPacketSize == 0) && - (OuterStride != Dynamic) && (OuterStride % InnerPacketSize == 0) && - (EIGEN_UNALIGNED_VECTORIZE || JointAlignment >= InnerRequiredAlignment); + // Generic packet assignment stores forward from coeffRef(); the swap kernel uses writePacket(). + static constexpr bool MayInnerVectorize = + MightVectorize && (DstHasDirectAccess || std::is_same<AssignFunc, swap_assign_op<DstScalar>>::value) && + (InnerSizeAtCompileTime != Dynamic) && (InnerSizeAtCompileTime % InnerPacketSize == 0) && + (OuterStride != Dynamic) && (OuterStride % InnerPacketSize == 0) && + (EIGEN_UNALIGNED_VECTORIZE || JointAlignment >= InnerRequiredAlignment); static constexpr bool MayLinearize = StorageOrdersAgree && (DstFlags & SrcFlags & LinearAccessBit); static constexpr bool MayLinearVectorize = MightVectorize && MayLinearize && DstHasDirectAccess &&
diff --git a/Eigen/src/Core/Reshaped.h b/Eigen/src/Core/Reshaped.h index 09d0f77..4a94cd1 100644 --- a/Eigen/src/Core/Reshaped.h +++ b/Eigen/src/Core/Reshaped.h
@@ -295,12 +295,12 @@ CoeffReadCost = evaluator<ArgType>::CoeffReadCost /* TODO + cost of index computations */, // The reshape enumerates the nested expression's elements in `Order`. When the nested - // evaluator's linear enumeration follows the same order -- its storage order matches, or it is - // vector-shaped so the order is immaterial -- the n-th reshaped element is the n-th nested - // element and every access forwards linearly, with no division/modulo index remapping. + // evaluator and the reshape's storage both enumerate in that order, linear accesses and + // packets can forward unchanged. A vector shape makes only its own storage order immaterial. NestedRowMajor = (int(evaluator<ArgType>::Flags) & RowMajorBit) != 0, OrderMatchesNested = (Order == int(ColMajor)) != NestedRowMajor, ForwardLinearAccess = (OrderMatchesNested || ArgType::RowsAtCompileTime == 1 || ArgType::ColsAtCompileTime == 1) && + (Order == int(traits<XprType>::ReshapedStorageOrder) || Rows == 1 || Cols == 1) && ((int(evaluator<ArgType>::Flags) & LinearAccessBit) != 0) // Flags and Alignment are defined by evaluator<Reshaped>, which derives from this evaluator.
diff --git a/test/array_reverse.cpp b/test/array_reverse.cpp index 28eae6f..c9fc4ee 100644 --- a/test/array_reverse.cpp +++ b/test/array_reverse.cpp
@@ -220,6 +220,34 @@ } } +template <typename MatrixType, int Direction> +void reverse_destination() { + using Scalar = typename MatrixType::Scalar; + const MatrixType source = MatrixType::Random(); + MatrixType destination, expected; + Reverse<MatrixType, Direction> reversed(destination); + for (Index j = 0; j < source.cols(); ++j) + for (Index i = 0; i < source.rows(); ++i) + expected(i, j) = source(Direction == Horizontal ? i : source.rows() - 1 - i, + Direction == Vertical ? j : source.cols() - 1 - j); + + reversed = source; + VERIFY_IS_APPROX(destination, expected); + destination.setConstant(Scalar(3)); + reversed += source; + VERIFY_IS_APPROX(destination, (expected.array() + Scalar(3))); + destination.setConstant(Scalar(3)); + reversed -= source; + VERIFY_IS_APPROX(destination, (Scalar(3) - expected.array())); +} + +template <typename MatrixType> +void reverse_destinations() { + reverse_destination<MatrixType, BothDirections>(); + reverse_destination<MatrixType, Vertical>(); + reverse_destination<MatrixType, Horizontal>(); +} + EIGEN_DECLARE_TEST(array_reverse) { for (int i = 0; i < g_repeat; i++) { CALL_SUBTEST_1(reverse(Matrix<float, 1, 1>())); @@ -243,4 +271,10 @@ CALL_SUBTEST_10(reverseInPlace_boundary<float>()); CALL_SUBTEST_10(reverseInPlace_boundary<double>()); CALL_SUBTEST_10(reverseInPlace_boundary<int>()); + CALL_SUBTEST_11(reverse_destinations<Vector4d>()); + CALL_SUBTEST_11(reverse_destinations<RowVector4d>()); + CALL_SUBTEST_11(reverse_destinations<Matrix4f>()); + CALL_SUBTEST_11((reverse_destinations<Matrix<float, 4, 4, RowMajor>>())); + CALL_SUBTEST_11((reverse_destinations<Array<float, 4, 4>>())); + CALL_SUBTEST_11((reverse_destinations<Matrix<std::complex<float>, 4, 4>>())); }
diff --git a/test/reshape.cpp b/test/reshape.cpp index 9a90b06..eaf3e24 100644 --- a/test/reshape.cpp +++ b/test/reshape.cpp
@@ -225,10 +225,17 @@ LinearAccessBit); STATIC_CHECK(int(internal::evaluator<Reshaped<Sum, Dynamic, Dynamic, ColMajor> >::Alignment) == int(internal::evaluator<Sum>::Alignment)); - // A vector-shaped expression source forwards in either enumeration order. + // A vector source does not make a matrix-shaped result's storage order immaterial. typedef CwiseBinaryOp<internal::scalar_sum_op<Scalar, Scalar>, const Vec, const Vec> VecSum; - STATIC_CHECK((int(internal::evaluator<Reshaped<VecSum, Dynamic, Dynamic, RowMajor> >::Flags) & PacketAccessBit) == + STATIC_CHECK((int(internal::evaluator<Reshaped<VecSum, Dynamic, Dynamic, RowMajor>>::Flags) & PacketAccessBit) == 0); + STATIC_CHECK((int(internal::evaluator<Reshaped<VecSum, Dynamic, Dynamic, RowMajor>>::Flags) & LinearAccessBit) == 0); + STATIC_CHECK((int(internal::evaluator<Reshaped<VecSum, Dynamic, Dynamic, ColMajor>>::Flags) & PacketAccessBit) == int(BasePacket)); + STATIC_CHECK((int(internal::evaluator<Reshaped<VecSum, 1, Dynamic, ColMajor>>::Flags) & PacketAccessBit) == + int(BasePacket)); + using RowVec = Matrix<Scalar, 1, Dynamic, RowMajor>; + STATIC_CHECK((int(internal::evaluator<Reshaped<RowVec, Dynamic, Dynamic, ColMajor>>::Flags) & + (LinearAccessBit | PacketAccessBit)) == 0); // No packet access: cross-order reshape (no direct access), STATIC_CHECK((int(internal::evaluator<Reshaped<Mat, Dynamic, Dynamic, RowMajor> >::Flags) & PacketAccessBit) == 0); @@ -299,6 +306,69 @@ } } +template <int Order, typename VectorType, typename RowsType, typename ColsType> +void reshape_vector_order(RowsType rows, ColsType cols) { + using Scalar = typename VectorType::Scalar; + using ColMatrix = Matrix<Scalar, Dynamic, Dynamic, ColMajor>; + using RowMatrix = Matrix<Scalar, Dynamic, Dynamic, RowMajor>; + const Index nrows = internal::get_runtime_value(rows), ncols = internal::get_runtime_value(cols); + VectorType vector(nrows * ncols); + for (Index k = 0; k < vector.size(); ++k) vector(k) = Scalar(k + 1); + ColMatrix expected(nrows, ncols); + const bool rowOrder = Order == RowMajor || (Order == AutoOrder && VectorType::IsRowMajor); + for (Index j = 0; j < ncols; ++j) + for (Index i = 0; i < nrows; ++i) expected(i, j) = Scalar(1 + (rowOrder ? i * ncols + j : j * nrows + i)); + + const VectorType& source = vector; + const auto view = source.template reshaped<Order>(rows, cols); + VERIFY_IS_EQUAL(ColMatrix(view), expected); + VERIFY_IS_EQUAL(RowMatrix(view), expected); + VERIFY_IS_EQUAL(ColMatrix(view.transpose()), expected.transpose()); + VERIFY_IS_EQUAL(RowMatrix(view.transpose()), expected.transpose()); + if (Order != AutoOrder) { + VERIFY_IS_EQUAL(ColMatrix(source.transpose().template reshaped<Order>(rows, cols).transpose()), + expected.transpose()); + VERIFY_IS_EQUAL(RowMatrix(source.transpose().template reshaped<Order>(rows, cols).transpose()), + expected.transpose()); + } + VERIFY_IS_EQUAL(ColMatrix((source + source).template reshaped<Order>(rows, cols)), 2 * expected); + VERIFY_IS_EQUAL(RowMatrix((source + source).template reshaped<Order>(rows, cols)), 2 * expected); + for (Index j = 0; j < ncols; ++j) + for (Index i = 0; i < nrows; ++i) VERIFY_IS_EQUAL(view.coeff(i, j), expected(i, j)); + if (Order == ColMajor) VERIFY_IS_EQUAL(ColMatrix(source.reshaped(rows, cols).transpose()), expected.transpose()); + + VectorType destination(vector.size()); + destination.template reshaped<Order>(rows, cols) = expected; + VERIFY_IS_EQUAL(destination, vector); + const RowMatrix rowExpected = expected; + destination.setZero(); + destination.template reshaped<Order>(rows, cols) = rowExpected; + VERIFY_IS_EQUAL(destination, vector); + destination.template reshaped<Order>(rows, cols) += rowExpected; + VERIFY_IS_EQUAL(destination, 2 * vector); + auto reversed = destination.reverse().template reshaped<Order>(rows, cols); + using AssignmentTraits = + internal::copy_using_evaluator_traits<internal::evaluator<decltype(reversed)>, internal::evaluator<RowMatrix>, + internal::assign_op<Scalar, Scalar>>; + VERIFY(!AssignmentTraits::Vectorized); + reversed = rowExpected; + VERIFY_IS_EQUAL(destination, vector.reverse()); + reversed += rowExpected; + VERIFY_IS_EQUAL(destination, 2 * vector.reverse()); + reversed -= rowExpected; + VERIFY_IS_EQUAL(destination, vector.reverse()); +} + +template <typename Scalar, int Order> +void reshape_vector_orders() { + reshape_vector_order<Order, Matrix<Scalar, Dynamic, 1>>(3, 2); + reshape_vector_order<Order, Matrix<Scalar, 1, Dynamic>>(3, 2); + reshape_vector_order<Order, Matrix<Scalar, Dynamic, 1>>(5, 7); + reshape_vector_order<Order, Matrix<Scalar, 1, Dynamic>>(5, 7); + reshape_vector_order<Order, Matrix<Scalar, 6, 1>>(fix<3>, fix<2>); + reshape_vector_order<Order, Matrix<Scalar, 1, 6>>(fix<3>, fix<2>); +} + template <typename BlockType> void reshape_block(const BlockType& M) { auto dense = M.eval(); @@ -315,6 +385,37 @@ } } +void reshape_reversed_swap() { + constexpr int packet_size = internal::packet_traits<double>::size; + using VectorType = Matrix<double, 1, 3 * packet_size>; + using MatrixType = Matrix<double, 3, packet_size, packet_size == 1 ? ColMajor : RowMajor>; + VectorType destination; + MatrixType source; + for (Index i = 0; i < destination.size(); ++i) { + destination(i) = double(i + 1); + source.data()[i] = double(i + 101); + } + const VectorType original = destination; + const MatrixType originalSource = source; + auto view = destination.reverse().reshaped<RowMajor>(fix<3>, fix<packet_size>); + using SwapTraits = + internal::copy_using_evaluator_traits<internal::evaluator<decltype(view)>, internal::evaluator<MatrixType>, + internal::swap_assign_op<double>>; + using AssignTraits = + internal::copy_using_evaluator_traits<internal::evaluator<decltype(view)>, internal::evaluator<MatrixType>, + internal::assign_op<double, double>>; + VERIFY(!AssignTraits::Vectorized); + if (internal::packet_traits<double>::Vectorizable && EIGEN_UNALIGNED_VECTORIZE) VERIFY(SwapTraits::Vectorized); + view.swap(source); + for (Index i = 0; i < destination.size(); ++i) { + VERIFY_IS_EQUAL(destination(i), originalSource.data()[destination.size() - 1 - i]); + VERIFY_IS_EQUAL(source.data()[i], original(destination.size() - 1 - i)); + } + source.swap(view); + VERIFY_IS_EQUAL(destination, original); + VERIFY_IS_EQUAL(source, originalSource); +} + EIGEN_DECLARE_TEST(reshape) { typedef Matrix<int, Dynamic, Dynamic, RowMajor> RowMatrixXi; typedef Matrix<int, 4, 4, RowMajor> RowMatrix4i; @@ -336,6 +437,15 @@ CALL_SUBTEST_6(check_reshaped_evaluator_flags<double>()); CALL_SUBTEST_6(check_reshaped_evaluator_flags<int>()); + CALL_SUBTEST_8((reshape_vector_orders<float, ColMajor>())); + CALL_SUBTEST_8((reshape_vector_orders<float, RowMajor>())); + CALL_SUBTEST_8((reshape_vector_orders<double, ColMajor>())); + CALL_SUBTEST_8((reshape_vector_orders<double, RowMajor>())); + CALL_SUBTEST_8((reshape_vector_orders<double, AutoOrder>())); + CALL_SUBTEST_8((reshape_vector_orders<int, ColMajor>())); + CALL_SUBTEST_8((reshape_vector_orders<int, RowMajor>())); + CALL_SUBTEST_8(reshape_reversed_swap()); + for (int i = 0; i < g_repeat; i++) { CALL_SUBTEST_7(reshape_copies<float>(17, 13)); CALL_SUBTEST_7(reshape_copies<float>(1, 19));