From 79349eeb8ef5a5432f8b770acd615868f22a9fb9 Mon Sep 17 00:00:00 2001 From: Aditya Kashi Date: Tue, 23 Feb 2021 22:25:19 +0100 Subject: [PATCH] Addressed review comments including renaming the new accessor to block_col_major MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Thomas Grützmacher --- core/test/base/range_accessors.cpp | 26 +++--- include/ginkgo/core/base/range_accessors.hpp | 86 +++++++++++--------- 2 files changed, 60 insertions(+), 52 deletions(-) diff --git a/core/test/base/range_accessors.cpp b/core/test/base/range_accessors.cpp index 0694541278b..7027bed7420 100644 --- a/core/test/base/range_accessors.cpp +++ b/core/test/base/range_accessors.cpp @@ -275,13 +275,13 @@ TEST_F(RowMajorAccessor3d, CanAssignSubranges) } -class ColMajorAccessor3d : public ::testing::Test { +class BlockColMajorAccessor3d : public ::testing::Test { protected: using span = gko::span; static constexpr gko::size_type dimensionality{3}; - using col_major_range = - gko::range>; + using blk_col_major_range = + gko::range>; // clang-format off int data[2 * 3 * 4]{ @@ -308,14 +308,14 @@ class ColMajorAccessor3d : public ::testing::Test { // clang-format on const gko::dim dim1{2, 3, 4}; const gko::dim dim2{2, 2, 3}; - col_major_range default_r{data, dim1}; - col_major_range custom_r{ + blk_col_major_range default_r{data, dim1}; + blk_col_major_range custom_r{ data, dim2, std::array{12, 3}}; }; -TEST_F(ColMajorAccessor3d, ComputesCorrectStride) +TEST_F(BlockColMajorAccessor3d, ComputesCorrectStride) { auto range_stride = default_r.get_accessor().stride; auto check_stride = std::array{12, 3}; @@ -324,7 +324,7 @@ TEST_F(ColMajorAccessor3d, ComputesCorrectStride) } -TEST_F(ColMajorAccessor3d, CanAccessData) +TEST_F(BlockColMajorAccessor3d, CanAccessData) { EXPECT_EQ(default_r(0, 0, 0), 1); EXPECT_EQ(custom_r(0, 0, 0), 1); @@ -339,7 +339,7 @@ TEST_F(ColMajorAccessor3d, CanAccessData) } -TEST_F(ColMajorAccessor3d, CanWriteData) +TEST_F(BlockColMajorAccessor3d, CanWriteData) { default_r(0, 0, 0) = 4; custom_r(1, 1, 1) = 100; @@ -351,7 +351,7 @@ TEST_F(ColMajorAccessor3d, CanWriteData) } -TEST_F(ColMajorAccessor3d, CanCreateSubrange) +TEST_F(BlockColMajorAccessor3d, CanCreateSubrange) { auto subr = custom_r(span{0, 2}, span{1, 2}, span{1, 3}); @@ -362,7 +362,7 @@ TEST_F(ColMajorAccessor3d, CanCreateSubrange) } -TEST_F(ColMajorAccessor3d, CanCreateRowVector) +TEST_F(BlockColMajorAccessor3d, CanCreateRowVector) { auto subr = default_r(1u, 2u, span{0, 2}); @@ -371,7 +371,7 @@ TEST_F(ColMajorAccessor3d, CanCreateRowVector) } -TEST_F(ColMajorAccessor3d, CanCreateColumnVector) +TEST_F(BlockColMajorAccessor3d, CanCreateColumnVector) { auto subr = default_r(span{0u, 2u}, 1u, 3u); @@ -380,7 +380,7 @@ TEST_F(ColMajorAccessor3d, CanCreateColumnVector) } -TEST_F(ColMajorAccessor3d, CanAssignValues) +TEST_F(BlockColMajorAccessor3d, CanAssignValues) { default_r(1, 1, 1) = default_r(0, 0, 0); @@ -388,7 +388,7 @@ TEST_F(ColMajorAccessor3d, CanAssignValues) } -TEST_F(ColMajorAccessor3d, CanAssignSubranges) +TEST_F(BlockColMajorAccessor3d, CanAssignSubranges) { default_r(1u, span{0, 2}, span{0, 3}) = custom_r(0u, span{0, 2}, span{0, 3}); diff --git a/include/ginkgo/core/base/range_accessors.hpp b/include/ginkgo/core/base/range_accessors.hpp index aa91b025e17..8e380fa6ee4 100644 --- a/include/ginkgo/core/base/range_accessors.hpp +++ b/include/ginkgo/core/base/range_accessors.hpp @@ -468,6 +468,10 @@ template class row_major { public: friend class range; + + /** + * Number of dimensions of the accessor. + */ static constexpr size_type dimensionality = 2; /** @@ -480,10 +484,6 @@ class row_major { */ using data_type = value_type *; - /** - * Number of dimensions of the accessor. - */ - using const_accessor = row_major; protected: @@ -631,7 +631,11 @@ class row_major { }; -namespace detail_colmajor { +/** + * Namespace for helper functions and structs for + * the block column major accessor. + */ +namespace detail_block_col_major { /** @@ -658,9 +662,11 @@ struct index_helper_s { FirstType first, Indices &&... idxs) { if (current_iter == total_dim - 1) { - return first + - index_helper_s:: - compute(size, stride, std::forward(idxs)...); + return GKO_ASSERT(first < size[dim_idx]), + first + + index_helper_s:: + compute(size, stride, + std::forward(idxs)...); } return GKO_ASSERT(first < size[dim_idx]), @@ -676,12 +682,12 @@ struct index_helper_s { static constexpr size_type dim_idx{total_dim - 1}; - template + template static constexpr GKO_ATTRIBUTES ValueType compute(const std::array &size, const std::array 1 ? total_dim - 1 : 0)> &stride, - FirstType first, Indices &&...) + FirstType first) { return GKO_ASSERT(first < size[total_dim - 1]), first * stride[dim_idx - 1]; @@ -733,15 +739,15 @@ constexpr GKO_ATTRIBUTES } -} // namespace detail_colmajor +} // namespace detail_block_col_major /** - * A bridge between a range and a column-major memory layout. + * A bridge between a range and a block-column-major memory layout. * - * Note that this is not completely a 'layout right'. Only the innermost - * two dimensions are regarded as defining a column-major matrix, and - * the rest of the dimensions are treated identically to \ref row_major. + * Only the innermost two dimensions are regarded as defining + * a column-major matrix, and the rest of the dimensions are treated + * identically to \ref row_major. * * You should not try to explicitly create an instance of this accessor. * Instead, supply it as a template parameter to a range, and pass the @@ -752,14 +758,18 @@ constexpr GKO_ATTRIBUTES * @tparam Dimensionality number of dimensions of this accessor */ template -class col_major { +class block_col_major { public: - friend class range; + friend class range; static_assert(Dimensionality != 0, "This accessor does not support a dimensionality of 0!"); static_assert(Dimensionality != 1, "Please use row_major accessor for 1D ranges."); + + /** + * Number of dimensions of the accessor. + */ static constexpr size_type dimensionality = Dimensionality; /** @@ -772,17 +782,13 @@ class col_major { */ using data_type = value_type *; - /** - * Number of dimensions of the accessor. - */ - - using const_accessor = col_major; + using const_accessor = block_col_major; using stride_type = std::array; using length_type = std::array; protected: /** - * Creates a col_major accessor. + * Creates a block_col_major accessor. * * @param data pointer to the block of memory containing the data * @param lengths size / length of the accesses of each dimension @@ -791,33 +797,34 @@ class col_major { * `x_1 * stride_1 + x_2 * stride_2 * ... + x_(n-1) + x_n * stride_(n-1)` * points to the element at (x_1, x_2, ..., x_n)) */ - constexpr GKO_ATTRIBUTES explicit col_major(data_type data, - dim size, - stride_type stride) + constexpr GKO_ATTRIBUTES explicit block_col_major(data_type data, + dim size, + stride_type stride) : data{data}, lengths(detail::to_array(size)), stride(stride) {} /** - * Creates a col_major accessor with a default stride (assumes no padding) + * Creates a block_col_major accessor with a default stride + * (assumes no padding) * * @param data pointer to the block of memory containing the data * @param lengths size / length of the accesses of each dimension */ - constexpr GKO_ATTRIBUTES explicit col_major(data_type data, - dim size) + constexpr GKO_ATTRIBUTES explicit block_col_major(data_type data, + dim size) : data{data}, lengths(detail::to_array(size)), - stride(detail_colmajor::default_stride_array(lengths)) + stride(detail_block_col_major::default_stride_array(lengths)) {} public: /** - * Creates a col_major range which contains a read-only version of the - * current accessor. + * Creates a block_col_major range which contains a read-only version of + * the current accessor. * - * @returns a col major range which is read-only. + * @returns a block column major range which is read-only. */ constexpr GKO_ATTRIBUTES range to_const() const { @@ -838,7 +845,7 @@ class col_major { std::enable_if_t::value, value_type &> operator()(Indices &&... indices) const { - return data[detail_colmajor::compute_index( + return data[detail_block_col_major::compute_index( lengths, stride, std::forward(indices)...)]; } @@ -851,13 +858,14 @@ class col_major { * @return sub-range spanning the given spans */ template - constexpr GKO_ATTRIBUTES std::enable_if_t< - detail::are_span_compatible::value, range> - operator()(SpanTypes... spans) const + constexpr GKO_ATTRIBUTES + std::enable_if_t::value, + range> + operator()(SpanTypes... spans) const { return detail::validate_spans(lengths, spans...), - range{ - data + detail_colmajor::compute_index( + range{ + data + detail_block_col_major::compute_index( lengths, stride, (span{spans}.begin)...), dim{ (span{spans}.end - span{spans}.begin)...},