From 2c4c226f902d566ada48dec7143f86ec6f7d49f7 Mon Sep 17 00:00:00 2001 From: danakj Date: Fri, 13 Oct 2023 13:45:51 -0400 Subject: [PATCH] Code review snippets to address from @davidben Also: * sus_if_msvc for attributes should be sus_if_msvc_not_cl * "language is msvc variant" vs "compiler is msvc" * sus_if_gnuc_language * test wrapping for each integer size, esp < 32bit * not destroy nevervalue? since it's in a union can that be a requirement on NeverValueTypes, and is C++ okay with constructing an object in a place where one already exists and was not destroyed? --- sus/CMakeLists.txt | 1 - sus/macros/arch.h | 23 ------- sus/macros/pure.h | 4 +- sus/num/__private/intrinsics.h | 114 ++++++++++++++------------------- sus/num/__private/literals.h | 2 +- 5 files changed, 51 insertions(+), 93 deletions(-) delete mode 100644 sus/macros/arch.h diff --git a/sus/CMakeLists.txt b/sus/CMakeLists.txt index 8f243e2d3..552690754 100644 --- a/sus/CMakeLists.txt +++ b/sus/CMakeLists.txt @@ -131,7 +131,6 @@ target_sources(subspace PUBLIC "iter/zip.h" "macros/__private/compiler_bugs.h" "macros/assume.h" - "macros/arch.h" "macros/builtin.h" "macros/compiler.h" "macros/eval_macro.h" diff --git a/sus/macros/arch.h b/sus/macros/arch.h deleted file mode 100644 index 838443fe4..000000000 --- a/sus/macros/arch.h +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright 2022 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#pragma once - -#if defined(__x86_64__) || defined(_M_X64) -#define sus_is_64bit() true // x86_64 -#elif defined(__aarch64__) || defined(_M_ARM64) -#define sus_is_64bit() true // aarch64 -#else -#define sus_is_64bit() false -#endif diff --git a/sus/macros/pure.h b/sus/macros/pure.h index 4151653da..c083e0c91 100644 --- a/sus/macros/pure.h +++ b/sus/macros/pure.h @@ -21,7 +21,7 @@ /// /// A pure function is allowed to dereference pointers, and access global /// memory, but it may not change them. To do so can cause Undefined Behaviour. -#define sus_pure sus_if_msvc_else([[nodiscard]], __attribute__((pure))) +#define sus_pure [[nodiscard]] sus_if_not_msvc_compiler(, __attribute__((pure))) /// Used to mark a function as "const", meaning it does not change any values /// outside of its own scope, and does not read global memory. @@ -31,4 +31,4 @@ /// A const function is allowed to only read from its inputs and determine an /// output from them, without going through pointers or accessing global memory. /// To do so anyway can cause Undefined Behaviour. -#define sus_pure_const sus_if_msvc_else([[nodiscard]], __attribute__((const))) +#define sus_pure_const [[nodiscard]] sus_if_msvc_else(, __attribute__((const))) diff --git a/sus/num/__private/intrinsics.h b/sus/num/__private/intrinsics.h index c33b67c10..02d49375f 100644 --- a/sus/num/__private/intrinsics.h +++ b/sus/num/__private/intrinsics.h @@ -479,7 +479,7 @@ template ::sus::mem::size_of() <= 8) sus_pure_const sus_always_inline constexpr uint32_t count_ones( T value) noexcept { -#if _MSC_VER +#if _MSC_VER && !IS_CLANG_CL if (std::is_constant_evaluated()) { using M = MathType; auto mvalue = M{value}; @@ -522,8 +522,7 @@ sus_pure_const sus_always_inline constexpr uint32_t leading_zeros_nonzero( ::sus::marker::UnsafeFnMarker, T value) noexcept { if (std::is_constant_evaluated()) { uint32_t count = 0; - for (auto i = uint32_t{0}; - i < unchecked_mul(unchecked_sizeof(), uint32_t{8}); ++i) { + for (auto i = uint32_t{0}; i < num_bits(); ++i) { const bool zero = (value & high_bit()) == 0; if (!zero) break; count += 1; @@ -532,7 +531,7 @@ sus_pure_const sus_always_inline constexpr uint32_t leading_zeros_nonzero( return count; } -#if _MSC_VER +#if _MSC_VER && !IS_CLANG_CL if constexpr (::sus::mem::size_of() == 8u) { #if 1 unsigned long index; @@ -601,7 +600,7 @@ template ::sus::mem::size_of() <= 8) sus_pure_const sus_always_inline constexpr uint32_t leading_zeros( T value) noexcept { - if (value == 0) return unchecked_mul(unchecked_sizeof(), uint32_t{8}); + if (value == 0) return num_bits(); return leading_zeros_nonzero(::sus::marker::unsafe_fn, value); } @@ -616,8 +615,7 @@ sus_pure_const sus_always_inline constexpr uint32_t trailing_zeros_nonzero( ::sus::marker::UnsafeFnMarker, T value) noexcept { if (std::is_constant_evaluated()) { uint32_t count = 0; - for (auto i = uint32_t{0}; - i < unchecked_mul(unchecked_sizeof(), uint32_t{8}); ++i) { + for (auto i = uint32_t{0}; i < num_bits(); ++i) { const bool zero = (value & 1) == 0; if (!zero) break; count += 1; @@ -657,7 +655,6 @@ sus_pure_const sus_always_inline constexpr uint32_t trailing_zeros_nonzero( #endif } -// TODO: Any way to make it constexpr? template requires(std::is_integral_v && std::is_unsigned_v && ::sus::mem::size_of() <= 8) @@ -671,7 +668,7 @@ template requires(std::is_integral_v && std::is_unsigned_v && ::sus::mem::size_of() <= 8) sus_pure_const sus_always_inline constexpr T reverse_bits(T value) noexcept { -#if __clang__ +#if COMPILER_IS_CLANG if constexpr (::sus::mem::size_of() == 1) { return __builtin_bitreverse8(value); } else if constexpr (::sus::mem::size_of() == 2) { @@ -685,7 +682,7 @@ sus_pure_const sus_always_inline constexpr T reverse_bits(T value) noexcept { #else // Algorithm from Ken Raeburn: // http://graphics.stanford.edu/~seander/bithacks.html#ReverseParallel - uint32_t bits = unchecked_mul(unchecked_sizeof(), uint32_t{8}); + constexpr uint32_t bits = num_bits(); auto mask = unchecked_not(T(0)); while ((bits >>= 1) > 0) { mask ^= unchecked_shl(mask, bits); @@ -699,7 +696,9 @@ sus_pure_const sus_always_inline constexpr T reverse_bits(T value) noexcept { template requires(std::is_integral_v && std::is_unsigned_v) sus_pure_const inline constexpr T rotate_left(T value, uint32_t n) noexcept { - const uint32_t num_bits = unchecked_mul(unchecked_sizeof(), uint32_t{8}); + // Use this: + // https://boringssl.googlesource.com/boringssl/+/refs/heads/master/crypto/internal.h#1098 + constexpr uint32_t num_bits = num_bits(); // Try avoid slow % operation if we can. Comparisons are much faster than %. if (n >= num_bits) n %= num_bits; if (n == 0) return value; @@ -710,7 +709,9 @@ sus_pure_const inline constexpr T rotate_left(T value, uint32_t n) noexcept { template requires(std::is_integral_v && std::is_unsigned_v) sus_pure_const inline constexpr T rotate_right(T value, uint32_t n) noexcept { - const uint32_t num_bits = unchecked_mul(unchecked_sizeof(), uint32_t{8}); + // Use this: + // https://boringssl.googlesource.com/boringssl/+/refs/heads/master/crypto/internal.h#1098 + constexpr uint32_t num_bits = num_bits(); // Try avoid slow % operation if we can. Comparisons are much faster than %. if (n >= num_bits) n %= num_bits; if (n == 0) return value; @@ -830,8 +831,10 @@ sus_pure_const sus_always_inline constexpr auto into_signed(T x) noexcept { return static_cast(x); } +/// Returns just the sign bit for a (signed) integer, keeping it in place. template - requires(::sus::mem::size_of() <= 8) + requires(std::is_integral_v && std::is_signed_v && + ::sus::mem::size_of() <= 8) sus_pure_const sus_always_inline constexpr bool sign_bit(T x) noexcept { if constexpr (::sus::mem::size_of() == 1) return (x & (T(1) << 7)) != 0; @@ -848,7 +851,7 @@ template ::sus::mem::size_of() <= 8) sus_pure_const inline constexpr OverflowOut add_with_overflow(T x, T y) noexcept { - return OverflowOut sus_clang_bug_56394(){ + return OverflowOut{ .overflow = x > max_value() - y, .value = unchecked_add(x, y), }; @@ -861,7 +864,7 @@ sus_pure_const inline constexpr OverflowOut add_with_overflow(T x, T y) noexcept { const auto out = into_signed(unchecked_add(into_unsigned(x), into_unsigned(y))); - return OverflowOut sus_clang_bug_56394(){ + return OverflowOut{ .overflow = y >= 0 != out >= x, .value = out, }; @@ -873,7 +876,7 @@ template ()))> ::sus::mem::size_of() == ::sus::mem::size_of()) sus_pure_const inline constexpr OverflowOut add_with_overflow_signed( T x, U y) noexcept { - return OverflowOut sus_clang_bug_56394(){ + return OverflowOut{ .overflow = (y >= 0 && into_unsigned(y) > max_value() - x) || (y < 0 && into_unsigned(-y) > x), .value = unchecked_add(x, into_unsigned(y)), @@ -887,7 +890,7 @@ template ()))> sus_pure_const inline constexpr OverflowOut add_with_overflow_unsigned( T x, U y) noexcept { const auto out = into_signed(unchecked_add(into_unsigned(x), y)); - return OverflowOut sus_clang_bug_56394(){ + return OverflowOut{ .overflow = static_cast(max_value()) - static_cast(x) < y, .value = out, }; @@ -898,7 +901,7 @@ template ::sus::mem::size_of() <= 8) sus_pure_const inline constexpr OverflowOut sub_with_overflow(T x, T y) noexcept { - return OverflowOut sus_clang_bug_56394(){ + return OverflowOut{ .overflow = x < unchecked_add(min_value(), y), .value = unchecked_sub(x, y), }; @@ -911,7 +914,7 @@ sus_pure_const inline constexpr OverflowOut sub_with_overflow(T x, T y) noexcept { const auto out = into_signed(unchecked_sub(into_unsigned(x), into_unsigned(y))); - return OverflowOut sus_clang_bug_56394(){ + return OverflowOut{ .overflow = y >= 0 != out <= x, .value = out, }; @@ -924,7 +927,7 @@ template ()))> sus_pure_const inline constexpr OverflowOut sub_with_overflow_unsigned( T x, U y) noexcept { const auto out = into_signed(unchecked_sub(into_unsigned(x), y)); - return OverflowOut sus_clang_bug_56394(){ + return OverflowOut{ .overflow = static_cast(x) - static_cast(min_value()) < y, .value = out, }; @@ -946,8 +949,8 @@ sus_pure_const inline constexpr OverflowOut mul_with_overflow(T x, // TODO: Can we use compiler intrinsics? auto out = unchecked_mul(into_widened(x), into_widened(y)); using Wide = decltype(out); - return OverflowOut sus_clang_bug_56394(){ - .overflow = out > Wide{max_value()}, .value = static_cast(out)}; + return OverflowOut{.overflow = out > Wide{max_value()}, + .value = static_cast(out)}; } template @@ -959,21 +962,18 @@ sus_pure_const inline constexpr OverflowOut mul_with_overflow(T x, if (std::is_constant_evaluated()) { const bool overflow = x > T{1} && y > T{1} && x > unchecked_div(max_value(), y); - return OverflowOut sus_clang_bug_56394(){.overflow = overflow, - .value = unchecked_mul(x, y)}; + return OverflowOut{.overflow = overflow, .value = unchecked_mul(x, y)}; } else { // For MSVC, use _umul128, but what about constexpr?? If we can't do // it then make the whole function non-constexpr? uint64_t highbits; auto out = static_cast(_umul128(x, y, &highbits)); - return OverflowOut sus_clang_bug_56394(){.overflow = highbits != 0, - .value = out}; + return OverflowOut{.overflow = highbits != 0, .value = out}; } #else auto out = __uint128_t{x} * __uint128_t{y}; - return OverflowOut sus_clang_bug_56394(){ - .overflow = out > __uint128_t{max_value()}, - .value = static_cast(out)}; + return OverflowOut{.overflow = out > __uint128_t{max_value()}, + .value = static_cast(out)}; #endif } @@ -985,7 +985,7 @@ sus_pure_const inline constexpr OverflowOut mul_with_overflow(T x, // TODO: Can we use compiler intrinsics? auto out = into_widened(x) * into_widened(y); using Wide = decltype(out); - return OverflowOut sus_clang_bug_56394(){ + return OverflowOut{ .overflow = out > Wide{max_value()} || out < Wide{min_value()}, .value = static_cast(out)}; } @@ -995,10 +995,9 @@ template ::sus::mem::size_of() == 8) sus_pure_const inline constexpr OverflowOut mul_with_overflow(T x, T y) noexcept { -#if _MSC_VER +#if _MSC_VER && !IS_CLANG_CL if (x == T{0} || y == T{0}) - return OverflowOut sus_clang_bug_56394(){.overflow = false, - .value = T{0}}; + return OverflowOut{.overflow = false, .value = T{0}}; using U = decltype(into_unsigned(x)); const auto absx = @@ -1016,14 +1015,12 @@ sus_pure_const inline constexpr OverflowOut mul_with_overflow(T x, unchecked_add(into_unsigned(max_value()), U{mul_negative}); const bool overflow = absx > unchecked_div(mul_max, absy); const auto mul_val = unchecked_mul(into_unsigned(x), into_unsigned(y)); - return OverflowOut sus_clang_bug_56394(){.overflow = overflow, - .value = into_signed(mul_val)}; + return OverflowOut{.overflow = overflow, .value = into_signed(mul_val)}; #else auto out = __int128_t{x} * __int128_t{y}; - return OverflowOut sus_clang_bug_56394(){ - .overflow = - out > __int128_t{max_value()} || out < __int128_t{min_value()}, - .value = static_cast(out)}; + return OverflowOut{.overflow = out > __int128_t{max_value()} || + out < __int128_t{min_value()}, + .value = static_cast(out)}; #endif } @@ -1031,9 +1028,7 @@ template requires(std::is_integral_v && ::sus::mem::size_of() <= 8) sus_pure_const inline constexpr OverflowOut pow_with_overflow( T base, uint32_t exp) noexcept { - if (exp == 0) - return OverflowOut sus_clang_bug_56394(){.overflow = false, - .value = T{1}}; + if (exp == 0) return OverflowOut{.overflow = false, .value = T{1}}; auto acc = T{1}; bool overflow = false; while (exp > 1) { @@ -1048,8 +1043,7 @@ sus_pure_const inline constexpr OverflowOut pow_with_overflow( base = r.value; } auto r = mul_with_overflow(acc, base); - return OverflowOut sus_clang_bug_56394(){ - .overflow = overflow || r.overflow, .value = r.value}; + return OverflowOut{.overflow = overflow || r.overflow, .value = r.value}; } template @@ -1064,8 +1058,7 @@ sus_pure_const inline constexpr OverflowOut shl_with_overflow( const bool overflow = shift >= num_bits(); if (overflow) [[unlikely]] shift = shift & (unchecked_sub(num_bits(), uint32_t{1})); - return OverflowOut sus_clang_bug_56394(){.overflow = overflow, - .value = unchecked_shl(x, shift)}; + return OverflowOut{.overflow = overflow, .value = unchecked_shl(x, shift)}; } template @@ -1080,7 +1073,7 @@ sus_pure_const inline constexpr OverflowOut shl_with_overflow( const bool overflow = shift >= num_bits(); if (overflow) [[unlikely]] shift = shift & (unchecked_sub(num_bits(), uint32_t{1})); - return OverflowOut sus_clang_bug_56394(){ + return OverflowOut{ .overflow = overflow, .value = into_signed(unchecked_shl(into_unsigned(x), shift))}; } @@ -1097,8 +1090,7 @@ sus_pure_const inline constexpr OverflowOut shr_with_overflow( const bool overflow = shift >= num_bits(); if (overflow) [[unlikely]] shift = shift & (unchecked_sub(num_bits(), uint32_t{1})); - return OverflowOut sus_clang_bug_56394(){.overflow = overflow, - .value = unchecked_shr(x, shift)}; + return OverflowOut{.overflow = overflow, .value = unchecked_shr(x, shift)}; } template @@ -1113,8 +1105,7 @@ sus_pure_const inline constexpr OverflowOut shr_with_overflow( const bool overflow = shift >= num_bits(); if (overflow) [[unlikely]] shift = shift & (unchecked_sub(num_bits(), uint32_t{1})); - return OverflowOut sus_clang_bug_56394(){.overflow = overflow, - .value = unchecked_shr(x, shift)}; + return OverflowOut{.overflow = overflow, .value = unchecked_shr(x, shift)}; } template @@ -1207,15 +1198,14 @@ template requires(std::is_integral_v && !std::is_signed_v && ::sus::mem::size_of() <= 8) sus_pure_const sus_always_inline constexpr T wrapping_add(T x, T y) noexcept { - return x + y; + return unchecked_add(x, y); } template requires(std::is_integral_v && std::is_signed_v && ::sus::mem::size_of() <= 8) sus_pure_const sus_always_inline constexpr T wrapping_add(T x, T y) noexcept { - // TODO: Are there cheaper intrinsics? - return add_with_overflow(x, y).value; + return into_signed(unchecked_add(into_unsigned(x), into_unsigned(y))); // Test pass? } template @@ -1229,23 +1219,21 @@ template requires(std::is_integral_v && std::is_signed_v && ::sus::mem::size_of() <= 8) sus_pure_const sus_always_inline constexpr T wrapping_sub(T x, T y) noexcept { - // TODO: Are there cheaper intrinsics? - return sub_with_overflow(x, y).value; + return into_signed(unchecked_sub(into_unsigned(x), into_unsigned(y))); // Test pass? } template requires(std::is_integral_v && !std::is_signed_v && ::sus::mem::size_of() <= 8) sus_pure_const sus_always_inline constexpr T wrapping_mul(T x, T y) noexcept { - return x * y; + return unchecked_mul(x, y); } template requires(std::is_integral_v && std::is_signed_v && ::sus::mem::size_of() <= 8) sus_pure_const sus_always_inline constexpr T wrapping_mul(T x, T y) noexcept { - // TODO: Are there cheaper intrinsics? - return mul_with_overflow(x, y).value; + return into_signed(unchecked_mul(into_unsigned(x), into_unsigned(y))); // Test pass? } template @@ -1429,12 +1417,6 @@ sus_pure_const sus_always_inline constexpr int32_t max_10_exp() noexcept { return int32_t{308}; } -template - requires(std::is_floating_point_v) -sus_pure_const sus_always_inline constexpr uint32_t radix() noexcept { - return 2; -} - template requires(std::is_floating_point_v) sus_pure_const sus_always_inline constexpr uint32_t @@ -1745,7 +1727,7 @@ sus_pure_const constexpr inline ::sus::num::FpCategory float_category( if (float_nonzero_is_subnormal(x)) return ::sus::num::FpCategory::Subnormal; return ::sus::num::FpCategory::Normal; } else { - // C++23 requires a constexpr way to do this. + // This is constexpr in C++23. switch (::fpclassify(x)) { case FP_NAN: return ::sus::num::FpCategory::Nan; case FP_INFINITE: return ::sus::num::FpCategory::Infinite; diff --git a/sus/num/__private/literals.h b/sus/num/__private/literals.h index eb6b38639..0d66f271b 100644 --- a/sus/num/__private/literals.h +++ b/sus/num/__private/literals.h @@ -20,7 +20,7 @@ #include "sus/assertions/check.h" #endif -#if _MSC_VER && !defined(__clang__) +#if _MSC_VER && !SUS_COMPILER_IS_CLANG /// Literal integer value. #define _sus__integer_literal(Name, T) \ /* A `constexpr` workaround for MSVC bug that doesn't constant-evaluate \