[libc][math] Fix incorrect logic in fputil::generic::add_or_sub #116129
Conversation
This file contains hidden or 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
|
@llvm/pr-subscribers-libc Author: OverMighty (overmighty) ChangesFixes incorrect logic that went unnoticed until the function was tested Full diff: https://github.com/llvm/llvm-project/pull/116129.diff 2 Files Affected:
diff --git a/libc/src/__support/FPUtil/generic/add_sub.h b/libc/src/__support/FPUtil/generic/add_sub.h
index 6bc9dcd23bafad..5c503e752c8ecb 100644
--- a/libc/src/__support/FPUtil/generic/add_sub.h
+++ b/libc/src/__support/FPUtil/generic/add_sub.h
@@ -160,20 +160,21 @@ add_or_sub(InType x, InType y) {
} else {
InStorageType max_mant = max_bits.get_explicit_mantissa() << GUARD_BITS_LEN;
InStorageType min_mant = min_bits.get_explicit_mantissa() << GUARD_BITS_LEN;
- int alignment =
- max_bits.get_biased_exponent() - min_bits.get_biased_exponent();
+
+ int alignment = (max_bits.get_biased_exponent() - max_bits.is_normal()) -
+ (min_bits.get_biased_exponent() - min_bits.is_normal());
InStorageType aligned_min_mant =
min_mant >> cpp::min(alignment, RESULT_MANTISSA_LEN);
bool aligned_min_mant_sticky;
- if (alignment <= 3)
+ if (alignment <= GUARD_BITS_LEN)
aligned_min_mant_sticky = false;
- else if (alignment <= InFPBits::FRACTION_LEN + 3)
+ else if (alignment > InFPBits::FRACTION_LEN + GUARD_BITS_LEN)
+ aligned_min_mant_sticky = true;
+ else
aligned_min_mant_sticky =
(min_mant << (InFPBits::STORAGE_LEN - alignment)) != 0;
- else
- aligned_min_mant_sticky = true;
InStorageType min_mant_sticky(static_cast<int>(aligned_min_mant_sticky));
@@ -183,7 +184,7 @@ add_or_sub(InType x, InType y) {
result_mant = max_mant - (aligned_min_mant | min_mant_sticky);
}
- int result_exp = max_bits.get_exponent() - RESULT_FRACTION_LEN;
+ int result_exp = max_bits.get_explicit_exponent() - RESULT_FRACTION_LEN;
DyadicFloat result(result_sign, result_exp, result_mant);
return result.template as<OutType, /*ShouldSignalExceptions=*/true>();
}
diff --git a/libc/test/src/math/smoke/AddTest.h b/libc/test/src/math/smoke/AddTest.h
index 66b188f4fa7b31..26efff4efaf948 100644
--- a/libc/test/src/math/smoke/AddTest.h
+++ b/libc/test/src/math/smoke/AddTest.h
@@ -136,6 +136,16 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
func(InType(1.0), in.min_denormal);
EXPECT_FP_EXCEPTION(FE_INEXACT);
}
+
+ void test_mixed_normality(AddFunc func) {
+ if (LIBC_NAMESPACE::fputil::get_fp_type<OutType>() !=
+ LIBC_NAMESPACE::fputil::get_fp_type<InType>())
+ return;
+
+ EXPECT_FP_EQ(FPBits::create_value(Sign::POS, 2U, 0b1U).get_val(),
+ func(InFPBits::create_value(Sign::POS, 2U, 0U).get_val(),
+ InFPBits::create_value(Sign::POS, 2U, 0b10U).get_val()));
+ }
};
#define LIST_ADD_TESTS(OutType, InType, func) \
@@ -145,6 +155,7 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
test_invalid_operations(&func); \
} \
TEST_F(LlvmLibcAddTest, RangeErrors) { test_range_errors(&func); } \
- TEST_F(LlvmLibcAddTest, InexactResults) { test_inexact_results(&func); }
+ TEST_F(LlvmLibcAddTest, InexactResults) { test_inexact_results(&func); } \
+ TEST_F(LlvmLibcAddTest, MixedNormality) { test_mixed_normality(&func); }
#endif // LLVM_LIBC_TEST_SRC_MATH_SMOKE_ADDTEST_H
|
overmighty
commented
Nov 14, 2024
Comment on lines
+164
to
+160
| int alignment = (max_bits.get_biased_exponent() - max_bits.is_normal()) - | ||
| (min_bits.get_biased_exponent() - min_bits.is_normal()); |
There was a problem hiding this comment.
The formula given in section 9.2.3.2 of Handbook of Floating-Point Arithmetic is fputil::generic::add_or_sub, I asked myself why it wasn't just
overmighty
force-pushed
the
libc-math-fix-add
branch
from
ab73332 to
fb91e3b
Compare
lntue
reviewed
Dec 19, 2024
overmighty
force-pushed
the
libc-math-fix-add
branch
from
fb91e3b to
80b1add
Compare
Fixes incorrect logic that went unnoticed until the function was tested with output and input types having the same underlying floating-point format. The resulting DyadicFloat's exponent was off by one when adding two subnormal numbers, and the minimum operand's mantissa was misaligned by one bit when adding a normal number with a subnormal number.
overmighty
force-pushed
the
libc-math-fix-add
branch
from
80b1add to
1ceafa2
Compare
lntue
approved these changes
Mar 18, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/104/builds/18422 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/188/builds/11972 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/196/builds/6132 Here is the relevant piece of the build log for the reference |
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.
Fixes incorrect logic that went unnoticed until the function was tested
with output and input types that have the same underlying floating-point
format.