From c94a2292cee1d7c5ec1b13c648dd9cf2a6bd5992 Mon Sep 17 00:00:00 2001 From: Alex Guteniev Date: Sat, 23 Oct 2021 17:29:18 +0300 Subject: [PATCH 1/8] add explicit non-zero context to `_Countr_zero` fixes #2292 --- stl/inc/limits | 12 ++++++------ stl/inc/numeric | 6 +++--- stl/inc/vector | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/stl/inc/limits b/stl/inc/limits index da17ea93e03..b4ba8f22be1 100644 --- a/stl/inc/limits +++ b/stl/inc/limits @@ -1063,7 +1063,7 @@ extern int __isa_available; #endif // __clang__ } -template +template _NODISCARD int _Checked_x86_x64_countr_zero(const _Ty _Val) noexcept { constexpr int _Digits = numeric_limits<_Ty>::digits; constexpr _Ty _Max = (numeric_limits<_Ty>::max) (); @@ -1072,7 +1072,7 @@ _NODISCARD int _Checked_x86_x64_countr_zero(const _Ty _Val) noexcept { // Because the widening done below will always give a non-0 value, checking for tzcnt // is not required for 8-bit and 16-bit since the only difference in behavior between // bsf and tzcnt is when the value is 0. - if constexpr (_Digits > 16) { + if constexpr (_Digits > 16 && !_Assume_non_zero) { const bool _Definitely_have_tzcnt = __isa_available >= __ISA_AVAILABLE_AVX2; if (!_Definitely_have_tzcnt && _Val == 0) { return _Digits; @@ -1090,9 +1090,9 @@ _NODISCARD int _Checked_x86_x64_countr_zero(const _Ty _Val) noexcept { const unsigned int _High = _Val >> 32; const unsigned int _Low = static_cast(_Val); if (_Low == 0) { - return 32 + _Checked_x86_x64_countr_zero(_High); + return 32 + _Checked_x86_x64_countr_zero<_Assume_non_zero>(_High); } else { - return _Checked_x86_x64_countr_zero(_Low); + return _Checked_x86_x64_countr_zero(_Low); } #else // ^^^ _M_IX86 / !_M_IX86 vvv return static_cast(_TZCNT_U64(_Val)); @@ -1150,12 +1150,12 @@ template constexpr bool _Is_standard_unsigned_integer = _Is_any_of_v, unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long>; -template , int> = 0> +template , int> = 0> _NODISCARD constexpr int _Countr_zero(const _Ty _Val) noexcept { #if defined(_M_IX86) || defined(_M_X64) #if _HAS_CXX20 if (!_STD is_constant_evaluated()) { - return _Checked_x86_x64_countr_zero(_Val); + return _Checked_x86_x64_countr_zero<_Assume_non_zero>(_Val); } #endif // _HAS_CXX20 #endif // defined(_M_IX86) || defined(_M_X64) diff --git a/stl/inc/numeric b/stl/inc/numeric index 1d6c90bf094..d1cc12b22e2 100644 --- a/stl/inc/numeric +++ b/stl/inc/numeric @@ -560,13 +560,13 @@ _NODISCARD constexpr common_type_t<_Mt, _Nt> gcd(const _Mt _Mx, const _Nt _Nx) n return static_cast<_Common>(_Mx_magnitude); } - const auto _Mx_trailing_zeroes = static_cast(_Countr_zero(_Mx_magnitude)); + const auto _Mx_trailing_zeroes = static_cast(_Countr_zero(_Mx_magnitude)); const auto _Common_factors_of_2 = - (_STD min) (_Mx_trailing_zeroes, static_cast(_Countr_zero(_Nx_magnitude))); + (_STD min) (_Mx_trailing_zeroes, static_cast(_Countr_zero(_Nx_magnitude))); _Nx_magnitude >>= _Common_factors_of_2; _Mx_magnitude >>= _Mx_trailing_zeroes; do { - _Nx_magnitude >>= static_cast(_Countr_zero(_Nx_magnitude)); + _Nx_magnitude >>= static_cast(_Countr_zero(_Nx_magnitude)); if (_Mx_magnitude > _Nx_magnitude) { _Common_unsigned _Temp = _Mx_magnitude; _Mx_magnitude = _Nx_magnitude; diff --git a/stl/inc/vector b/stl/inc/vector index 92323342bc3..1759e8f6543 100644 --- a/stl/inc/vector +++ b/stl/inc/vector @@ -1820,7 +1820,7 @@ struct _Vbase_compare_three_way { #endif // ^^^ !defined(__cpp_lib_concepts) ^^^ } - const int _Bit_index = _Countr_zero(_Differing_bits); // number of least significant bits that match + const int _Bit_index = _Countr_zero(_Differing_bits); // number of least significant bits that match _STL_INTERNAL_CHECK(_Bit_index < _VBITS); // because we return early for equality const _Vbase _Mask = _Vbase{1} << _Bit_index; // selects the least significant bit that differs From 38287a2d52b5bd0b234c2ad29f1dd07bc7c9dc24 Mon Sep 17 00:00:00 2001 From: Alex Guteniev Date: Sat, 23 Oct 2021 17:57:44 +0300 Subject: [PATCH 2/8] for smallish vals can also optimize --- stl/inc/limits | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/stl/inc/limits b/stl/inc/limits index b4ba8f22be1..13cf7478e26 100644 --- a/stl/inc/limits +++ b/stl/inc/limits @@ -1081,10 +1081,14 @@ _NODISCARD int _Checked_x86_x64_countr_zero(const _Ty _Val) noexcept { #endif // __AVX2__ if constexpr (_Digits <= 32) { - // Intended widening to int. This operation means that a narrow 0 will widen - // to 0xFFFF....FFFF0... instead of 0. We need this to avoid counting all the zeros - // of the wider type. - return static_cast(_TZCNT_U32(static_cast(~_Max | _Val))); + if constexpr (_Assume_non_zero) { + return static_cast(_TZCNT_U32(_Val)); + } else { + // Intended widening to int. This operation means that a narrow 0 will widen + // to 0xFFFF....FFFF0... instead of 0. We need this to avoid counting all the zeros + // of the wider type. + return static_cast(_TZCNT_U32(static_cast(~_Max | _Val))); + } } else { #ifdef _M_IX86 const unsigned int _High = _Val >> 32; From efe7434922f5fee21360cb808deb3eb31f0aff95 Mon Sep 17 00:00:00 2001 From: Alex Guteniev Date: Sun, 24 Oct 2021 13:28:41 +0300 Subject: [PATCH 3/8] @miscco assumes this is clearer --- stl/inc/limits | 25 ++++++++++++++++++------- stl/inc/numeric | 6 +++--- stl/inc/vector | 3 ++- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/stl/inc/limits b/stl/inc/limits index 13cf7478e26..aaed932377c 100644 --- a/stl/inc/limits +++ b/stl/inc/limits @@ -1063,7 +1063,12 @@ extern int __isa_available; #endif // __clang__ } -template +enum class _Countr_zero_assumption { + _No_assmuption, + _Non_zero_input, +}; + +template <_Countr_zero_assumption _Assumption, class _Ty> _NODISCARD int _Checked_x86_x64_countr_zero(const _Ty _Val) noexcept { constexpr int _Digits = numeric_limits<_Ty>::digits; constexpr _Ty _Max = (numeric_limits<_Ty>::max) (); @@ -1072,7 +1077,7 @@ _NODISCARD int _Checked_x86_x64_countr_zero(const _Ty _Val) noexcept { // Because the widening done below will always give a non-0 value, checking for tzcnt // is not required for 8-bit and 16-bit since the only difference in behavior between // bsf and tzcnt is when the value is 0. - if constexpr (_Digits > 16 && !_Assume_non_zero) { + if constexpr (_Digits > 16 && _Assumption != _Countr_zero_assumption::_Non_zero_input) { const bool _Definitely_have_tzcnt = __isa_available >= __ISA_AVAILABLE_AVX2; if (!_Definitely_have_tzcnt && _Val == 0) { return _Digits; @@ -1081,7 +1086,7 @@ _NODISCARD int _Checked_x86_x64_countr_zero(const _Ty _Val) noexcept { #endif // __AVX2__ if constexpr (_Digits <= 32) { - if constexpr (_Assume_non_zero) { + if constexpr (_Assumption == _Countr_zero_assumption::_Non_zero_input) { return static_cast(_TZCNT_U32(_Val)); } else { // Intended widening to int. This operation means that a narrow 0 will widen @@ -1094,9 +1099,9 @@ _NODISCARD int _Checked_x86_x64_countr_zero(const _Ty _Val) noexcept { const unsigned int _High = _Val >> 32; const unsigned int _Low = static_cast(_Val); if (_Low == 0) { - return 32 + _Checked_x86_x64_countr_zero<_Assume_non_zero>(_High); + return 32 + _Checked_x86_x64_countr_zero<_Assumption>(_High); } else { - return _Checked_x86_x64_countr_zero(_Low); + return _Checked_x86_x64_countr_zero<_Countr_zero_assumption::_Non_zero_input>(_Low); } #else // ^^^ _M_IX86 / !_M_IX86 vvv return static_cast(_TZCNT_U64(_Val)); @@ -1154,12 +1159,13 @@ template constexpr bool _Is_standard_unsigned_integer = _Is_any_of_v, unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long>; -template , int> = 0> +template <_Countr_zero_assumption _Assumption = _Countr_zero_assumption::_No_assmuption, // + class _Ty, enable_if_t<_Is_standard_unsigned_integer<_Ty>, int> = 0> _NODISCARD constexpr int _Countr_zero(const _Ty _Val) noexcept { #if defined(_M_IX86) || defined(_M_X64) #if _HAS_CXX20 if (!_STD is_constant_evaluated()) { - return _Checked_x86_x64_countr_zero<_Assume_non_zero>(_Val); + return _Checked_x86_x64_countr_zero<_Assumption>(_Val); } #endif // _HAS_CXX20 #endif // defined(_M_IX86) || defined(_M_X64) @@ -1167,6 +1173,11 @@ _NODISCARD constexpr int _Countr_zero(const _Ty _Val) noexcept { return _Countr_zero_fallback(_Val); } +template , int> = 0> +_NODISCARD constexpr int _Countr_zero_nonzero_input(const _Ty _Val) noexcept { + return _Countr_zero<_Countr_zero_assumption::_Non_zero_input>(_Val); +} + template , int> _Enabled = 0> _NODISCARD _CONSTEXPR20 int _Popcount(const _Ty _Val) noexcept { #if _HAS_POPCNT_INTRINSICS || _HAS_NEON_INTRINSICS diff --git a/stl/inc/numeric b/stl/inc/numeric index d1cc12b22e2..c8ab706478b 100644 --- a/stl/inc/numeric +++ b/stl/inc/numeric @@ -560,13 +560,13 @@ _NODISCARD constexpr common_type_t<_Mt, _Nt> gcd(const _Mt _Mx, const _Nt _Nx) n return static_cast<_Common>(_Mx_magnitude); } - const auto _Mx_trailing_zeroes = static_cast(_Countr_zero(_Mx_magnitude)); + const auto _Mx_trailing_zeroes = static_cast(_Countr_zero_nonzero_input(_Mx_magnitude)); const auto _Common_factors_of_2 = - (_STD min) (_Mx_trailing_zeroes, static_cast(_Countr_zero(_Nx_magnitude))); + (_STD min) (_Mx_trailing_zeroes, static_cast(_Countr_zero_nonzero_input(_Nx_magnitude))); _Nx_magnitude >>= _Common_factors_of_2; _Mx_magnitude >>= _Mx_trailing_zeroes; do { - _Nx_magnitude >>= static_cast(_Countr_zero(_Nx_magnitude)); + _Nx_magnitude >>= static_cast(_Countr_zero_nonzero_input(_Nx_magnitude)); if (_Mx_magnitude > _Nx_magnitude) { _Common_unsigned _Temp = _Mx_magnitude; _Mx_magnitude = _Nx_magnitude; diff --git a/stl/inc/vector b/stl/inc/vector index 1759e8f6543..19165240ad1 100644 --- a/stl/inc/vector +++ b/stl/inc/vector @@ -1820,7 +1820,8 @@ struct _Vbase_compare_three_way { #endif // ^^^ !defined(__cpp_lib_concepts) ^^^ } - const int _Bit_index = _Countr_zero(_Differing_bits); // number of least significant bits that match + const int _Bit_index = + _Countr_zero_nonzero_input(_Differing_bits); // number of least significant bits that match _STL_INTERNAL_CHECK(_Bit_index < _VBITS); // because we return early for equality const _Vbase _Mask = _Vbase{1} << _Bit_index; // selects the least significant bit that differs From 6148e10694234090a875e881c20036f1eb539c8c Mon Sep 17 00:00:00 2001 From: Alex Guteniev Date: Sun, 24 Oct 2021 14:04:37 +0300 Subject: [PATCH 4/8] arm build --- stl/inc/limits | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/stl/inc/limits b/stl/inc/limits index aaed932377c..bb6dd8427ec 100644 --- a/stl/inc/limits +++ b/stl/inc/limits @@ -1051,6 +1051,11 @@ _NODISCARD constexpr int _Popcount_fallback(_Ty _Val) noexcept { return static_cast(_Val >> (_Digits - 8)); } +enum class _Countr_zero_assumption { + _No_assmuption, + _Non_zero_input, +}; + #if defined(_M_IX86) || defined(_M_X64) extern "C" { extern int __isa_available; @@ -1063,11 +1068,6 @@ extern int __isa_available; #endif // __clang__ } -enum class _Countr_zero_assumption { - _No_assmuption, - _Non_zero_input, -}; - template <_Countr_zero_assumption _Assumption, class _Ty> _NODISCARD int _Checked_x86_x64_countr_zero(const _Ty _Val) noexcept { constexpr int _Digits = numeric_limits<_Ty>::digits; From 498d80bc3599b439b2e3046cc1144c2e983bb2e4 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 16 Nov 2021 19:39:31 -0800 Subject: [PATCH 5/8] Rename enumerators. Fix "assmuption" typo. Simplify _Non_zero_input to _Nonzero, consistent with function name. Avoid negation in _No_assumption; _Possibly_zero focuses on what we're concerned with. --- stl/inc/limits | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/stl/inc/limits b/stl/inc/limits index 35bd6bd8e9d..e297e2b8e16 100644 --- a/stl/inc/limits +++ b/stl/inc/limits @@ -1052,8 +1052,8 @@ _NODISCARD constexpr int _Popcount_fallback(_Ty _Val) noexcept { } enum class _Countr_zero_assumption { - _No_assmuption, - _Non_zero_input, + _Possibly_zero, + _Nonzero, }; #if defined(_M_IX86) || (defined(_M_X64) && !defined(_M_ARM64EC)) @@ -1072,7 +1072,7 @@ template <_Countr_zero_assumption _Assumption, class _Ty> _NODISCARD int _Countr_zero_tzcnt(const _Ty _Val) noexcept { constexpr int _Digits = numeric_limits<_Ty>::digits; - if constexpr (_Digits <= 16 && _Assumption == _Countr_zero_assumption::_No_assmuption) { + if constexpr (_Digits <= 16 && _Assumption == _Countr_zero_assumption::_Possibly_zero) { // Intended widening to int. This operation means that a narrow 0 will widen // to 0xFFFF....FFFF0... instead of 0. We need this to avoid counting all the zeros // of the wider type. @@ -1102,7 +1102,7 @@ _NODISCARD int _Countr_zero_bsf(const _Ty _Val) noexcept { unsigned long _Result; unsigned char _Bsf_return; - if constexpr (_Digits <= 16 && _Assumption == _Countr_zero_assumption::_No_assmuption) { + if constexpr (_Digits <= 16 && _Assumption == _Countr_zero_assumption::_Possibly_zero) { // Intended widening to int. This operation means that a narrow 0 will widen // to 0xFFFF....FFFF0... instead of 0. We need this to avoid counting all the zeros // of the wider type. @@ -1128,7 +1128,7 @@ _NODISCARD int _Countr_zero_bsf(const _Ty _Val) noexcept { #endif // _M_IX86 } - if constexpr (_Digits >= 32 && _Assumption == _Countr_zero_assumption::_No_assmuption) { + if constexpr (_Digits >= 32 && _Assumption == _Countr_zero_assumption::_Possibly_zero) { if (!_Bsf_return) { return _Digits; } @@ -1205,7 +1205,7 @@ template constexpr bool _Is_standard_unsigned_integer = _Is_any_of_v, unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long>; -template <_Countr_zero_assumption _Assumption = _Countr_zero_assumption::_No_assmuption, // +template <_Countr_zero_assumption _Assumption = _Countr_zero_assumption::_Possibly_zero, // class _Ty, enable_if_t<_Is_standard_unsigned_integer<_Ty>, int> = 0> _NODISCARD constexpr int _Countr_zero(const _Ty _Val) noexcept { #if defined(_M_IX86) || (defined(_M_X64) && !defined(_M_ARM64EC)) @@ -1221,7 +1221,7 @@ _NODISCARD constexpr int _Countr_zero(const _Ty _Val) noexcept { template , int> = 0> _NODISCARD constexpr int _Countr_zero_nonzero_input(const _Ty _Val) noexcept { - return _Countr_zero<_Countr_zero_assumption::_Non_zero_input>(_Val); + return _Countr_zero<_Countr_zero_assumption::_Nonzero>(_Val); } template , int> _Enabled = 0> From 9ffbf06db8fb68abcfcc4ed940df15d6b40ee731 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 16 Nov 2021 19:46:38 -0800 Subject: [PATCH 6/8] _STL_INTERNAL_CHECK is non-core, oops. --- stl/inc/limits | 1 - 1 file changed, 1 deletion(-) diff --git a/stl/inc/limits b/stl/inc/limits index e297e2b8e16..ad73705f2b3 100644 --- a/stl/inc/limits +++ b/stl/inc/limits @@ -1133,7 +1133,6 @@ _NODISCARD int _Countr_zero_bsf(const _Ty _Val) noexcept { return _Digits; } } else { - _STL_INTERNAL_CHECK(_Bsf_return); (void) _Bsf_return; } From 64a9ecbe8795458481243fe985225718f6a055c2 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 16 Nov 2021 19:50:19 -0800 Subject: [PATCH 7/8] Tests directly call _Countr_zero_bsf. --- stl/inc/limits | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/limits b/stl/inc/limits index ad73705f2b3..927945eea2d 100644 --- a/stl/inc/limits +++ b/stl/inc/limits @@ -1095,7 +1095,7 @@ _NODISCARD int _Countr_zero_tzcnt(const _Ty _Val) noexcept { } } -template <_Countr_zero_assumption _Assumption, class _Ty> +template <_Countr_zero_assumption _Assumption = _Countr_zero_assumption::_Possibly_zero, class _Ty> _NODISCARD int _Countr_zero_bsf(const _Ty _Val) noexcept { constexpr int _Digits = numeric_limits<_Ty>::digits; From 00e0c62706107bf75639b1fc55a0e6e82b767f56 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 16 Nov 2021 19:58:53 -0800 Subject: [PATCH 8/8] Suggestion: Avoid one-line helper function. --- stl/inc/limits | 5 ----- stl/inc/numeric | 7 ++++--- stl/inc/vector | 4 ++-- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/stl/inc/limits b/stl/inc/limits index 927945eea2d..2456f48ccf8 100644 --- a/stl/inc/limits +++ b/stl/inc/limits @@ -1218,11 +1218,6 @@ _NODISCARD constexpr int _Countr_zero(const _Ty _Val) noexcept { return _Countr_zero_fallback(_Val); } -template , int> = 0> -_NODISCARD constexpr int _Countr_zero_nonzero_input(const _Ty _Val) noexcept { - return _Countr_zero<_Countr_zero_assumption::_Nonzero>(_Val); -} - template , int> _Enabled = 0> _NODISCARD _CONSTEXPR20 int _Popcount(const _Ty _Val) noexcept { #if _HAS_POPCNT_INTRINSICS || _HAS_NEON_INTRINSICS diff --git a/stl/inc/numeric b/stl/inc/numeric index c8ab706478b..4b8c74af988 100644 --- a/stl/inc/numeric +++ b/stl/inc/numeric @@ -560,13 +560,14 @@ _NODISCARD constexpr common_type_t<_Mt, _Nt> gcd(const _Mt _Mx, const _Nt _Nx) n return static_cast<_Common>(_Mx_magnitude); } - const auto _Mx_trailing_zeroes = static_cast(_Countr_zero_nonzero_input(_Mx_magnitude)); + constexpr auto _Nonzero = _Countr_zero_assumption::_Nonzero; + const auto _Mx_trailing_zeroes = static_cast(_Countr_zero<_Nonzero>(_Mx_magnitude)); const auto _Common_factors_of_2 = - (_STD min) (_Mx_trailing_zeroes, static_cast(_Countr_zero_nonzero_input(_Nx_magnitude))); + (_STD min) (_Mx_trailing_zeroes, static_cast(_Countr_zero<_Nonzero>(_Nx_magnitude))); _Nx_magnitude >>= _Common_factors_of_2; _Mx_magnitude >>= _Mx_trailing_zeroes; do { - _Nx_magnitude >>= static_cast(_Countr_zero_nonzero_input(_Nx_magnitude)); + _Nx_magnitude >>= static_cast(_Countr_zero<_Nonzero>(_Nx_magnitude)); if (_Mx_magnitude > _Nx_magnitude) { _Common_unsigned _Temp = _Mx_magnitude; _Mx_magnitude = _Nx_magnitude; diff --git a/stl/inc/vector b/stl/inc/vector index 6ed3600a476..bc896f47e99 100644 --- a/stl/inc/vector +++ b/stl/inc/vector @@ -1814,8 +1814,8 @@ struct _Vbase_compare_three_way { #endif // ^^^ !defined(__cpp_lib_concepts) ^^^ } - const int _Bit_index = - _Countr_zero_nonzero_input(_Differing_bits); // number of least significant bits that match + constexpr auto _Nonzero = _Countr_zero_assumption::_Nonzero; + const int _Bit_index = _Countr_zero<_Nonzero>(_Differing_bits); // number of least significant bits that match _STL_INTERNAL_CHECK(_Bit_index < _VBITS); // because we return early for equality const _Vbase _Mask = _Vbase{1} << _Bit_index; // selects the least significant bit that differs