[libc][NFC] Make EXPONENT_BIAS int32_t #75046
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gchatelet
requested review from
lntue and
michaelrj-google
and removed request for
lntue
|
@llvm/pr-subscribers-libc Author: Guillaume Chatelet (gchatelet) Changes
Full diff: https://github.com/llvm/llvm-project/pull/75046.diff 4 Files Affected:
diff --git a/libc/src/__support/FPUtil/FloatProperties.h b/libc/src/__support/FPUtil/FloatProperties.h
index 59e261e1047f1a..10db54a41ce78a 100644
--- a/libc/src/__support/FPUtil/FloatProperties.h
+++ b/libc/src/__support/FPUtil/FloatProperties.h
@@ -41,7 +41,7 @@ template <> struct FPProperties<FPType::IEEE754_Binary32> {
static constexpr BitsType SIGN_MASK = BitsType(1)
<< (EXPONENT_WIDTH + MANTISSA_WIDTH);
static constexpr BitsType EXPONENT_MASK = ~(SIGN_MASK | MANTISSA_MASK);
- static constexpr uint32_t EXPONENT_BIAS = 127;
+ static constexpr int32_t EXPONENT_BIAS = 127;
static constexpr BitsType EXP_MANT_MASK = MANTISSA_MASK + EXPONENT_MASK;
static_assert(EXP_MANT_MASK == ~SIGN_MASK,
@@ -65,7 +65,7 @@ template <> struct FPProperties<FPType::IEEE754_Binary64> {
static constexpr BitsType SIGN_MASK = BitsType(1)
<< (EXPONENT_WIDTH + MANTISSA_WIDTH);
static constexpr BitsType EXPONENT_MASK = ~(SIGN_MASK | MANTISSA_MASK);
- static constexpr uint32_t EXPONENT_BIAS = 1023;
+ static constexpr int32_t EXPONENT_BIAS = 1023;
static constexpr BitsType EXP_MANT_MASK = MANTISSA_MASK + EXPONENT_MASK;
static_assert(EXP_MANT_MASK == ~SIGN_MASK,
@@ -98,7 +98,7 @@ template <> struct FPProperties<FPType::X86_Binary80> {
BitsType(1) << (EXPONENT_WIDTH + MANTISSA_WIDTH + 1);
static constexpr BitsType EXPONENT_MASK =
((BitsType(1) << EXPONENT_WIDTH) - 1) << (MANTISSA_WIDTH + 1);
- static constexpr uint32_t EXPONENT_BIAS = 16383;
+ static constexpr int32_t EXPONENT_BIAS = 16383;
static constexpr BitsType EXP_MANT_MASK =
MANTISSA_MASK | EXPLICIT_BIT_MASK | EXPONENT_MASK;
@@ -126,7 +126,7 @@ template <> struct FPProperties<FPType::IEEE754_Binary128> {
static constexpr BitsType SIGN_MASK = BitsType(1)
<< (EXPONENT_WIDTH + MANTISSA_WIDTH);
static constexpr BitsType EXPONENT_MASK = ~(SIGN_MASK | MANTISSA_MASK);
- static constexpr uint32_t EXPONENT_BIAS = 16383;
+ static constexpr int32_t EXPONENT_BIAS = 16383;
static constexpr BitsType EXP_MANT_MASK = MANTISSA_MASK | EXPONENT_MASK;
static_assert(EXP_MANT_MASK == ~SIGN_MASK,
diff --git a/libc/src/__support/detailed_powers_of_ten.h b/libc/src/__support/detailed_powers_of_ten.h
index f9628484bbe609..1c30eff9b3f11c 100644
--- a/libc/src/__support/detailed_powers_of_ten.h
+++ b/libc/src/__support/detailed_powers_of_ten.h
@@ -29,7 +29,8 @@ constexpr int32_t DETAILED_POWERS_OF_TEN_MIN_EXP_10 = -348;
constexpr int32_t DETAILED_POWERS_OF_TEN_MAX_EXP_10 = 347;
// This rescales the base 10 exponent by a factor of log(10)/log(2).
-LIBC_INLINE int64_t exp10_to_exp2(int64_t exp10) {
+LIBC_INLINE int32_t exp10_to_exp2(int32_t exp10) {
+ // Valid if exp10 < 646 456 636.
return (217706 * exp10) >> 16;
}
diff --git a/libc/src/__support/str_to_float.h b/libc/src/__support/str_to_float.h
index ad73e93f6faa81..e2e088fe97f02a 100644
--- a/libc/src/__support/str_to_float.h
+++ b/libc/src/__support/str_to_float.h
@@ -131,9 +131,8 @@ eisel_lemire(ExpandedFloat<T> init_num,
uint32_t clz = leading_zeroes<BitsType>(mantissa);
mantissa <<= clz;
- uint32_t exp2 = static_cast<uint32_t>(exp10_to_exp2(exp10)) +
- BITS_IN_MANTISSA + fputil::FloatProperties<T>::EXPONENT_BIAS -
- clz;
+ int32_t exp2 = exp10_to_exp2(exp10) + BITS_IN_MANTISSA +
+ fputil::FloatProperties<T>::EXPONENT_BIAS - clz;
// Multiplication
const uint64_t *power_of_ten =
@@ -210,7 +209,8 @@ eisel_lemire(ExpandedFloat<T> init_num,
// The if block is equivalent to (but has fewer branches than):
// if exp2 <= 0 || exp2 >= 0x7FF { etc }
- if (exp2 - 1 >= (1 << fputil::FloatProperties<T>::EXPONENT_WIDTH) - 2) {
+ if (static_cast<uint32_t>(exp2) - 1 >=
+ (1 << fputil::FloatProperties<T>::EXPONENT_WIDTH) - 2) {
return cpp::nullopt;
}
@@ -251,9 +251,8 @@ eisel_lemire<long double>(ExpandedFloat<long double> init_num,
uint32_t clz = leading_zeroes<BitsType>(mantissa);
mantissa <<= clz;
- uint32_t exp2 = static_cast<uint32_t>(exp10_to_exp2(exp10)) +
- BITS_IN_MANTISSA +
- fputil::FloatProperties<long double>::EXPONENT_BIAS - clz;
+ int32_t exp2 = exp10_to_exp2(exp10) + BITS_IN_MANTISSA +
+ fputil::FloatProperties<long double>::EXPONENT_BIAS - clz;
// Multiplication
const uint64_t *power_of_ten =
@@ -383,7 +382,7 @@ simple_decimal_conversion(const char *__restrict numStart,
// float, return inf.
if (hpd.get_decimal_point() > 0 &&
exp10_to_exp2(hpd.get_decimal_point() - 1) >
- static_cast<int64_t>(fputil::FloatProperties<T>::EXPONENT_BIAS)) {
+ fputil::FloatProperties<T>::EXPONENT_BIAS) {
output.num = {0, fputil::FPBits<T>::MAX_EXPONENT};
output.error = ERANGE;
return output;
@@ -391,8 +390,8 @@ simple_decimal_conversion(const char *__restrict numStart,
// If the exponent is too small even for a subnormal, return 0.
if (hpd.get_decimal_point() < 0 &&
exp10_to_exp2(-hpd.get_decimal_point()) >
- static_cast<int64_t>(fputil::FloatProperties<T>::EXPONENT_BIAS +
- fputil::FloatProperties<T>::MANTISSA_WIDTH)) {
+ (fputil::FloatProperties<T>::EXPONENT_BIAS +
+ fputil::FloatProperties<T>::MANTISSA_WIDTH)) {
output.num = {0, 0};
output.error = ERANGE;
return output;
@@ -652,7 +651,7 @@ clinger_fast_path(ExpandedFloat<T> init_num,
// log10(2^(exponent bias)).
// The generic approximation uses the fact that log10(2^x) ~= x/3
template <typename T> constexpr int32_t get_upper_bound() {
- return static_cast<int32_t>(fputil::FloatProperties<T>::EXPONENT_BIAS) / 3;
+ return fputil::FloatProperties<T>::EXPONENT_BIAS / 3;
}
template <> constexpr int32_t get_upper_bound<float>() { return 39; }
@@ -668,9 +667,9 @@ template <> constexpr int32_t get_upper_bound<double>() { return 309; }
// other out, and subnormal numbers allow for the result to be at the very low
// end of the final mantissa.
template <typename T> constexpr int32_t get_lower_bound() {
- return -(static_cast<int32_t>(fputil::FloatProperties<T>::EXPONENT_BIAS +
- fputil::FloatProperties<T>::MANTISSA_WIDTH +
- (sizeof(T) * 8)) /
+ return -((fputil::FloatProperties<T>::EXPONENT_BIAS +
+ static_cast<int32_t>(fputil::FloatProperties<T>::MANTISSA_WIDTH +
+ (sizeof(T) * 8))) /
3);
}
diff --git a/libc/src/math/generic/powf.cpp b/libc/src/math/generic/powf.cpp
index 5f2e95b44e5287..cbfb12cf645278 100644
--- a/libc/src/math/generic/powf.cpp
+++ b/libc/src/math/generic/powf.cpp
@@ -392,8 +392,8 @@ LIBC_INLINE bool is_odd_integer(float x) {
int x_e = static_cast<int>((x_u & FloatProp::EXPONENT_MASK) >>
FloatProp::MANTISSA_WIDTH);
int lsb = cpp::countr_zero(x_u | FloatProp::EXPONENT_MASK);
- constexpr int UNIT_EXPONENT =
- static_cast<int>(FloatProp::EXPONENT_BIAS + FloatProp::MANTISSA_WIDTH);
+ constexpr int32_t UNIT_EXPONENT =
+ FloatProp::EXPONENT_BIAS + FloatProp::MANTISSA_WIDTH;
return (x_e + lsb == UNIT_EXPONENT);
}
@@ -404,7 +404,7 @@ LIBC_INLINE bool is_integer(float x) {
FloatProp::MANTISSA_WIDTH);
int lsb = cpp::countr_zero(x_u | FloatProp::EXPONENT_MASK);
constexpr int UNIT_EXPONENT =
- static_cast<int>(FloatProp::EXPONENT_BIAS + FloatProp::MANTISSA_WIDTH);
+ FloatProp::EXPONENT_BIAS + FloatProp::MANTISSA_WIDTH;
return (x_e + lsb >= UNIT_EXPONENT);
}
|
gchatelet
commented
Dec 11, 2023
| @@ -29,8 +29,9 @@ constexpr int32_t DETAILED_POWERS_OF_TEN_MIN_EXP_10 = -348; | |||
| constexpr int32_t DETAILED_POWERS_OF_TEN_MAX_EXP_10 = 347; | |||
|
|
|||
| // This rescales the base 10 exponent by a factor of log(10)/log(2). | |||
| LIBC_INLINE int64_t exp10_to_exp2(int64_t exp10) { | |||
| return (217706 * exp10) >> 16; | |||
| LIBC_INLINE int32_t exp10_to_exp2(int32_t exp10) { | |||
There was a problem hiding this comment.
This is always called with an int32_t and should also return an int32_t.
I've highlighted the overflow risk. Note that the risk was already there before this patch, only that it was handled on the callee side.
lntue
approved these changes
Dec 11, 2023
gchatelet
force-pushed
the
change_exponent_bias_to_int32
branch
from
b4fb680
to
4c86735
Compare
`EXPONENT_BIAS` is almost always used with signed arithmetic. Making it an `int32_t` from the start reduces the chances to run into implementation defined behavior (cast from unsigned to signed is implementation-defined until C++20). https://en.cppreference.com/w/cpp/language/implicit_conversion#:~:text=If%20the%20destination%20type%20is%20signed,arithmetic%20overflow%2C%20which%20is%20undefined).
gchatelet
force-pushed
the
change_exponent_bias_to_int32
branch
from
4c86735
to
b14dc0b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
EXPONENT_BIASis almost always used with signed arithmetic. Making it anint32_tfrom the start reduces the chances to run into implementation defined behavior (cast from unsigned to signed is implementation-defined until C++20).https://en.cppreference.com/w/cpp/language/implicit_conversion#:~:text=If%20the%20destination%20type%20is%20signed,arithmetic%20overflow%2C%20which%20is%20undefined).