From 2abf713439f1013e8105b1303861a4f3713a0f55 Mon Sep 17 00:00:00 2001 From: Jakub Mazurkiewicz Date: Tue, 15 Apr 2025 13:39:53 +0200 Subject: [PATCH 1/8] Fix `layout_stride::mapping::is_exhaustive` --- stl/inc/mdspan | 102 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 100 insertions(+), 2 deletions(-) diff --git a/stl/inc/mdspan b/stl/inc/mdspan index fa151c8a499..975984f03ef 100644 --- a/stl/inc/mdspan +++ b/stl/inc/mdspan @@ -10,6 +10,7 @@ #if !_HAS_CXX23 _EMIT_STL_WARNING(STL4038, "The contents of are available only with C++23 or later."); #else // ^^^ !_HAS_CXX23 / _HAS_CXX23 vvv +#include #include #include #include @@ -311,6 +312,10 @@ public: return ((0 <= _Indices && _Indices < extent(_Seq)) && ...); } } + + template + friend constexpr pair _Count_dynamic_extents_equal_to_zero_or_one( + const _ExtentsT&) noexcept; // NB: used by 'layout_stride::mapping::is_exhaustive' }; template @@ -788,6 +793,28 @@ concept _Layout_mapping_alike = requires { bool_constant<_Mp::is_always_unique()>::value; }; +template +constexpr size_t _Count_static_extents_equal_to = 0; + +template +constexpr size_t _Count_static_extents_equal_to, _Val> = + ((_Extents == _Val) + ... + 0); + +template +_NODISCARD constexpr pair _Count_dynamic_extents_equal_to_zero_or_one(const _Extents& _Exts) noexcept { + _STL_INTERNAL_STATIC_ASSERT(_Is_extents<_Extents> && _Extents::rank_dynamic() != 0); + size_t _Zero_extents = 0; + size_t _One_extents = 0; + for (const auto _Ext : _Exts._Array) { + if (_Ext == 0) { + ++_Zero_extents; + } else if (_Ext == 1) { + ++_One_extents; + } + } + return {_Zero_extents, _One_extents}; +} + template class layout_stride::mapping : private _Maybe_fully_static_extents<_Extents>, private _Maybe_empty_array { @@ -963,11 +990,50 @@ public: } _NODISCARD constexpr bool is_exhaustive() const noexcept { + constexpr size_t _Static_zero_extents = _Count_static_extents_equal_to; if constexpr (extents_type::rank() == 0) { return true; + } else if constexpr (extents_type::rank() == 1) { + return this->_Array[0] == 1; + } else if constexpr (_Static_zero_extents >= 2) { + // Per N5008 [mdspan.layout.stride.obs]/5.2, we are looking for permutation P of integers in range [0, rank) + // such that 'stride(p[i]) == stride(p[i-1])*extent(p[i-1])' is true for 'i' in range '[1, rank)'. Knowing + // that at lease two extents are equal to zero, we can deduce that such permutation does not exist: + // - Some 'stride(p[j])' would have to be equal to 'stride(p[j-1])*extents(p[j-1]) = stride(p[j-1])*0 = 0' + // which is not possible. + // - Only 'extent(p[rank-1]) can be equal to 0, because it's not required to satisfy condition above. + // Since we have two or more extents equal to 0 this is not possible either. + return false; + } else if constexpr (extents_type::rank() == 2) { + return (this->_Array[0] == 1 && this->_Array[1] == this->_Exts.extent(0)) + || (this->_Array[1] == 1 && this->_Array[0] == this->_Exts.extent(1)); } else { - return required_span_size() - == _Fwd_prod_of_extents::_Calculate(this->_Exts, extents_type::_Rank); + // NB: Extents equal to 1 are problematic too - sometimes in such cases even when mapping is exhaustive + // this function should return false. For example when extents are [2, 1, 2] and strides are [1, 5, 2], + // mapping is exhaustive per N5008 [mdspan.layout.reqmts]/16 but not per N5008 + // [mdspan.layout.stride.obs]/5.2. + constexpr size_t _Static_zero_or_one_extents = + _Static_zero_extents + _Count_static_extents_equal_to; + + if constexpr (extents_type::rank_dynamic() != 0) { + const auto [_Dynamic_zero_extents, _Dynamic_one_extents] = + _Count_dynamic_extents_equal_to_zero_or_one(this->_Exts); + + const size_t _All_zero_extents = _Static_zero_extents + _Dynamic_zero_extents; + if (_All_zero_extents >= 2) { + return false; + } + + const size_t _All_zero_or_one_extents = + _Static_zero_or_one_extents + _Dynamic_zero_extents + _Dynamic_one_extents; + if (_All_zero_or_one_extents == 0) { + return _Is_exhaustive_common_case(); + } + } else if constexpr (_Static_zero_or_one_extents == 0) { + return _Is_exhaustive_common_case(); + } + + return _Is_exhaustive_special_case(); } } @@ -1036,6 +1102,38 @@ private: return static_cast(((_Indices * this->_Array[_Seq]) + ... + 0)); } + + _NODISCARD constexpr bool _Is_exhaustive_common_case() const noexcept { + return required_span_size() == _Fwd_prod_of_extents::_Calculate(this->_Exts, extents_type::_Rank); + } + + _NODISCARD constexpr bool _Is_exhaustive_special_case() const noexcept { + using _Stride_extent_pair = pair; + array<_Stride_extent_pair, extents_type::rank()> _Pairs; + for (rank_type _Idx = 0; _Idx < extents_type::_Rank; ++_Idx) { + rank_type _Ext = static_cast(this->_Exts.extent(_Idx)); + if (_Ext == 0) { + // NB: _Ext equal to zero is special - we want it to end up as close to the end of sorted range as + // possible, so we assign max value of rank_type to it. + _Ext = static_cast(-1); + } + + _Pairs[_Idx] = {static_cast(this->_Array[_Idx]), _Ext}; + } + + _RANGES sort(_Pairs); + if (_Pairs[0].first != 1) { + return false; + } + + for (rank_type _Idx = 1; _Idx < extents_type::_Rank; ++_Idx) { + if (_Pairs[_Idx].first != _Pairs[_Idx - 1].first * _Pairs[_Idx - 1].second) { + return false; + } + } + + return true; + } }; _EXPORT_STD template From 9bb898c41fe92482f47e412a227d00618e6d8bd7 Mon Sep 17 00:00:00 2001 From: Jakub Mazurkiewicz Date: Tue, 15 Apr 2025 13:41:22 +0200 Subject: [PATCH 2/8] Update libc++ skip --- tests/libcxx/expected_results.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/libcxx/expected_results.txt b/tests/libcxx/expected_results.txt index 64b389eaf67..96edc9eb0d5 100644 --- a/tests/libcxx/expected_results.txt +++ b/tests/libcxx/expected_results.txt @@ -177,6 +177,9 @@ std/ranges/range.adaptors/range.join/range.join.iterator/arrow.pass.cpp FAIL # If any feature-test macro test is failing, this consolidated test will also fail. std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp FAIL +# libc++ incorrectly implements `layout_stride::mapping::is_exhaustive()` +std/containers/views/mdspan/layout_stride/is_exhaustive_corner_case.pass.cpp FAIL + # *** INTERACTIONS WITH MSVC THAT UPSTREAM LIKELY WON'T FIX *** # These tests set an allocator with a max_size() too small to default construct an unordered container @@ -919,9 +922,6 @@ std/time/time.syn/formatter.month_day_last.pass.cpp FAIL # Our monotonic_buffer_resource takes "user" space for metadata, which it probably should not do. std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_with_initial_size.pass.cpp FAIL -# Likely STL bug in layout_stride::mapping::is_exhaustive(). -std/containers/views/mdspan/layout_stride/is_exhaustive_corner_case.pass.cpp FAIL - # *** NOT YET ANALYZED *** # Not analyzed. Clang instantiates BoomOnAnything during template argument substitution. From 0a6e7607831f78a99a4f598b278c844433706761 Mon Sep 17 00:00:00 2001 From: Jakub Mazurkiewicz Date: Tue, 15 Apr 2025 14:23:04 +0200 Subject: [PATCH 3/8] Add extra tests --- .../P0009R18_mdspan_layout_stride/test.cpp | 95 +++++++++++++++---- 1 file changed, 79 insertions(+), 16 deletions(-) diff --git a/tests/std/tests/P0009R18_mdspan_layout_stride/test.cpp b/tests/std/tests/P0009R18_mdspan_layout_stride/test.cpp index dc6daa67e0a..ae4bc642956 100644 --- a/tests/std/tests/P0009R18_mdspan_layout_stride/test.cpp +++ b/tests/std/tests/P0009R18_mdspan_layout_stride/test.cpp @@ -400,22 +400,85 @@ constexpr void check_required_span_size() { } constexpr void check_is_exhaustive() { - { // Check exhaustive mappings (all possibilities) - using E = extents; - assert((layout_stride::mapping{E{}, array{1, 2, 6}}.is_exhaustive())); - assert((layout_stride::mapping{E{}, array{1, 10, 2}}.is_exhaustive())); - assert((layout_stride::mapping{E{}, array{3, 1, 6}}.is_exhaustive())); - assert((layout_stride::mapping{E{}, array{15, 1, 3}}.is_exhaustive())); - assert((layout_stride::mapping{E{}, array{5, 10, 1}}.is_exhaustive())); - assert((layout_stride::mapping{E{}, array{15, 5, 1}}.is_exhaustive())); - } - - { // Check non-exhaustive mappings - using E = extents; - assert((!layout_stride::mapping{E{}, array{1, 2, 12}}.is_exhaustive())); - assert((!layout_stride::mapping{E{}, array{8, 18, 1}}.is_exhaustive())); - assert((!layout_stride::mapping{E{}, array{5, 1, 12}}.is_exhaustive())); - } + auto check = [](const auto& exts, const auto& strides, bool expected) { + layout_stride::mapping m{exts, strides}; + assert(m.is_exhaustive() == expected); + }; + + // rank() is equal to 0 + check(extents{}, array{}, true); + + // rank() is equal to 1 + check(extents{}, array{1}, true); + check(dextents{0}, array{2}, false); + check(extents{}, array{3}, false); + check(dextents{2}, array{2}, false); + check(extents{}, array{1}, true); + check(dextents{4}, array{1}, true); + + // rank() is equal to 2 + check(extents{}, array{1, 3}, true); + check(extents{3}, array{3, 1}, true); + check(extents{3}, array{4, 1}, false); + check(dextents{3, 3}, array{3, 1}, true); + check(extents{}, array{5, 1}, true); + check(extents{5}, array{1, 6}, true); + check(extents{5}, array{1, 8}, false); + check(dextents{6, 5}, array{1, 10}, false); + check(extents{}, array{3, 1}, true); + check(extents{0}, array{6, 1}, false); + check(extents{3}, array{6, 2}, false); + check(dextents{0, 3}, array{7, 2}, false); + check(extents{}, array{1, 1}, false); + check(extents{0, 0}, array{1, 1}, false); + check(dextents{0, 0}, array{1, 2}, false); + check(extents{0}, array{1, 2}, false); + + // rank() is greater than 2 + check(extents{}, array{1, 2, 6}, true); + check(extents{2}, array{1, 10, 2}, true); + check(extents{5}, array{3, 1, 6}, true); + check(extents{2, 3}, array{15, 1, 3}, true); + check(extents{3, 5}, array{5, 10, 1}, true); + check(dextents{2, 3, 5}, array{15, 5, 1}, true); + check(extents{}, array{1, 2, 12}, false); + check(extents{5}, array{8, 18, 1}, false); + check(dextents{2, 5, 8}, array{5, 1, 12}, false); + + // rank() is greater than 2 and some extents are equal to 0 + check(extents{}, array{7, 14, 1}, true); + check(extents{2}, array{1, 14, 2}, true); + check(extents{0}, array{14, 28, 1}, false); + check(extents{0, 7}, array{1, 2, 2}, false); + check(dextents{2, 0, 7}, array{2, 28, 4}, false); + check(extents{}, array{3, 1, 1}, false); + check(extents{0}, array{1, 5, 1}, false); + check(dextents{5, 0, 0}, array{2, 1, 10}, false); + check(extents{}, array{1, 1, 1}, false); + check(extents{}, array{1, 1, 1}, true); + check(std::extents{}, std::array{6, 2}, false); + + // rank() is greater than 2 - one extent is equal to 0 while others are equal to each other + check(extents{}, array{1, 9, 3}, true); + check(extents{3}, array{3, 9, 1}, true); + check(extents{0, 3}, array{1, 3, 3}, false); + check(dextents{3, 0, 3}, array{1, 4, 8}, false); + check(dextents{0, 1, 1}, array{1, 1, 1}, true); + + // required_span_size() is equal to 1 + check(extents{}, array{1}, true); + check(dextents{1}, array{3}, false); + check(extents{1}, array{1, 1}, true); + check(extents{}, array{1, 2, 1}, false); + + // Mapping is exhaustive, but is_exhaustive() should return false because of the way standard defined this function + check(extents{}, array{1, 4}, false); + check(dextents{5, 1, 2}, array{2, 11, 1}, false); + check(dextents{2, 3, 1}, array{3, 1, 8}, false); + check(extents{6}, array{50, 7, 1}, false); + check(dextents{1, 2}, array{5, 1}, false); + check(extents{}, array{1, 10}, false); + check(dextents{2, 1, 2}, array{3, 3, 1}, false); } constexpr void check_call_operator() { From 45a1398bcaf6959f1941ec3c094629faaa095adf Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 9 May 2025 05:36:10 -0700 Subject: [PATCH 4/8] Fix comment typos and minor cleanups. --- stl/inc/mdspan | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/stl/inc/mdspan b/stl/inc/mdspan index 975984f03ef..56b1af67b36 100644 --- a/stl/inc/mdspan +++ b/stl/inc/mdspan @@ -996,22 +996,23 @@ public: } else if constexpr (extents_type::rank() == 1) { return this->_Array[0] == 1; } else if constexpr (_Static_zero_extents >= 2) { - // Per N5008 [mdspan.layout.stride.obs]/5.2, we are looking for permutation P of integers in range [0, rank) - // such that 'stride(p[i]) == stride(p[i-1])*extent(p[i-1])' is true for 'i' in range '[1, rank)'. Knowing - // that at lease two extents are equal to zero, we can deduce that such permutation does not exist: + // Per N5008 [mdspan.layout.stride.obs]/5.2, we are looking for a permutation P of integers in the range + // '[0, rank)' such that 'stride(p[i]) == stride(p[i-1])*extent(p[i-1])' is true for 'i' in the range + // '[1, rank)'. Knowing that at least two extents are equal to zero, we can deduce that such a permutation + // does not exist: // - Some 'stride(p[j])' would have to be equal to 'stride(p[j-1])*extents(p[j-1]) = stride(p[j-1])*0 = 0' // which is not possible. - // - Only 'extent(p[rank-1]) can be equal to 0, because it's not required to satisfy condition above. + // - Only 'extent(p[rank-1])' can be equal to 0, because it's not required to satisfy the condition above. // Since we have two or more extents equal to 0 this is not possible either. return false; } else if constexpr (extents_type::rank() == 2) { return (this->_Array[0] == 1 && this->_Array[1] == this->_Exts.extent(0)) || (this->_Array[1] == 1 && this->_Array[0] == this->_Exts.extent(1)); } else { - // NB: Extents equal to 1 are problematic too - sometimes in such cases even when mapping is exhaustive - // this function should return false. For example when extents are [2, 1, 2] and strides are [1, 5, 2], - // mapping is exhaustive per N5008 [mdspan.layout.reqmts]/16 but not per N5008 - // [mdspan.layout.stride.obs]/5.2. + // NB: Extents equal to 1 are problematic too - sometimes in such cases even when the mapping is exhaustive + // this function should return false. + // For example, when the extents are [2, 1, 2] and the strides are [1, 5, 2], the mapping is exhaustive + // per N5008 [mdspan.layout.reqmts]/16 but not per N5008 [mdspan.layout.stride.obs]/5.2. constexpr size_t _Static_zero_or_one_extents = _Static_zero_extents + _Count_static_extents_equal_to; @@ -1113,7 +1114,7 @@ private: for (rank_type _Idx = 0; _Idx < extents_type::_Rank; ++_Idx) { rank_type _Ext = static_cast(this->_Exts.extent(_Idx)); if (_Ext == 0) { - // NB: _Ext equal to zero is special - we want it to end up as close to the end of sorted range as + // NB: _Ext equal to zero is special - we want it to end up as close to the end of the sorted range as // possible, so we assign max value of rank_type to it. _Ext = static_cast(-1); } From 7bb86ff321349debcfa67fe65f827d6dd2c5a426 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 9 May 2025 05:44:16 -0700 Subject: [PATCH 5/8] `static_cast` before adding `bool`s. --- stl/inc/mdspan | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/mdspan b/stl/inc/mdspan index 56b1af67b36..dd4b69260dd 100644 --- a/stl/inc/mdspan +++ b/stl/inc/mdspan @@ -798,7 +798,7 @@ constexpr size_t _Count_static_extents_equal_to = 0; template constexpr size_t _Count_static_extents_equal_to, _Val> = - ((_Extents == _Val) + ... + 0); + (static_cast(_Extents == _Val) + ... + 0); template _NODISCARD constexpr pair _Count_dynamic_extents_equal_to_zero_or_one(const _Extents& _Exts) noexcept { From 6ad5a70d2108c189d708f0337488e3d1fadd55da Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 9 May 2025 05:47:15 -0700 Subject: [PATCH 6/8] Iterate in-place. --- stl/inc/mdspan | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/mdspan b/stl/inc/mdspan index dd4b69260dd..ad4d9e1fcc1 100644 --- a/stl/inc/mdspan +++ b/stl/inc/mdspan @@ -805,7 +805,7 @@ _NODISCARD constexpr pair _Count_dynamic_extents_equal_to_zero_o _STL_INTERNAL_STATIC_ASSERT(_Is_extents<_Extents> && _Extents::rank_dynamic() != 0); size_t _Zero_extents = 0; size_t _One_extents = 0; - for (const auto _Ext : _Exts._Array) { + for (const auto& _Ext : _Exts._Array) { if (_Ext == 0) { ++_Zero_extents; } else if (_Ext == 1) { From a9d9e6eca3f3e36fa3b1a0c5285620350b833f2e Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 9 May 2025 05:56:57 -0700 Subject: [PATCH 7/8] Avoid a dead `return`. --- stl/inc/mdspan | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/stl/inc/mdspan b/stl/inc/mdspan index ad4d9e1fcc1..f11eda31a47 100644 --- a/stl/inc/mdspan +++ b/stl/inc/mdspan @@ -1030,11 +1030,13 @@ public: if (_All_zero_or_one_extents == 0) { return _Is_exhaustive_common_case(); } + + return _Is_exhaustive_special_case(); } else if constexpr (_Static_zero_or_one_extents == 0) { return _Is_exhaustive_common_case(); + } else { + return _Is_exhaustive_special_case(); } - - return _Is_exhaustive_special_case(); } } From 0f5534a6bb9986e29b16400591b7459030231232 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 9 May 2025 07:30:52 -0700 Subject: [PATCH 8/8] Drop `std::`, properly categorize "rank() is equal to 2". --- tests/std/tests/P0009R18_mdspan_layout_stride/test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/std/tests/P0009R18_mdspan_layout_stride/test.cpp b/tests/std/tests/P0009R18_mdspan_layout_stride/test.cpp index ae4bc642956..8e6f27ff054 100644 --- a/tests/std/tests/P0009R18_mdspan_layout_stride/test.cpp +++ b/tests/std/tests/P0009R18_mdspan_layout_stride/test.cpp @@ -426,6 +426,7 @@ constexpr void check_is_exhaustive() { check(extents{5}, array{1, 8}, false); check(dextents{6, 5}, array{1, 10}, false); check(extents{}, array{3, 1}, true); + check(extents{}, array{6, 2}, false); check(extents{0}, array{6, 1}, false); check(extents{3}, array{6, 2}, false); check(dextents{0, 3}, array{7, 2}, false); @@ -456,7 +457,6 @@ constexpr void check_is_exhaustive() { check(dextents{5, 0, 0}, array{2, 1, 10}, false); check(extents{}, array{1, 1, 1}, false); check(extents{}, array{1, 1, 1}, true); - check(std::extents{}, std::array{6, 2}, false); // rank() is greater than 2 - one extent is equal to 0 while others are equal to each other check(extents{}, array{1, 9, 3}, true);