Skip to content

[libc][math] Fix incorrect logic in fputil::generic::add_or_sub #116129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 18, 2025
Merged

[libc][math] Fix incorrect logic in fputil::generic::add_or_sub #116129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04cfc4a
[libc][math] Fix incorrect logic in fputil::generic::add_or_sub
overmighty Nov 14, 2024
f41bef4
Add same type tests
overmighty Nov 20, 2024
d644891
Fix remaining issues
overmighty Dec 19, 2024
1ceafa2
Add float128 same-type MPFR tests
overmighty Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion libc/src/__support/FPUtil/generic/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ add_header_library(
HDRS
add_sub.h
DEPENDS
libc.hdr.errno_macros
libc.hdr.fenv_macros
libc.src.__support.CPP.algorithm
libc.src.__support.CPP.bit
Expand Down
27 changes: 12 additions & 15 deletions libc/src/__support/FPUtil/generic/add_sub.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#ifndef LLVM_LIBC_SRC___SUPPORT_FPUTIL_GENERIC_ADD_SUB_H
#define LLVM_LIBC_SRC___SUPPORT_FPUTIL_GENERIC_ADD_SUB_H

#include "hdr/errno_macros.h"
#include "hdr/fenv_macros.h"
#include "src/__support/CPP/algorithm.h"
#include "src/__support/CPP/bit.h"
Expand Down Expand Up @@ -110,12 +109,8 @@ add_or_sub(InType x, InType y) {
return cast<OutType>(tmp);
}

if (y_bits.is_zero()) {
volatile InType tmp = y;
if constexpr (IsSub)
tmp = -tmp;
return cast<OutType>(tmp);
}
if (y_bits.is_zero())
return cast<OutType>(x);
}

InType x_abs = x_bits.abs().get_val();
Expand Down Expand Up @@ -160,20 +155,22 @@ 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());
Comment on lines +159 to +160
Copy link
Member Author

@overmighty overmighty Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formula given in section 9.2.3.2 of Handbook of Floating-Point Arithmetic is $\delta = (E_x - n_x) - (E_y - n_y)$. When I implemented fputil::generic::add_or_sub, I asked myself why it wasn't just $\delta = E_x - E_y$, and ended up using that instead of the formula given in the book. Today I remembered asking myself that question, so I thought about it again and now it's obvious.


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)
aligned_min_mant_sticky =
(min_mant << (InFPBits::STORAGE_LEN - alignment)) != 0;
else
else if (alignment > InFPBits::FRACTION_LEN + GUARD_BITS_LEN)
aligned_min_mant_sticky = true;
else
aligned_min_mant_sticky =
(static_cast<InStorageType>(
min_mant << (InFPBits::STORAGE_LEN - alignment))) != 0;

InStorageType min_mant_sticky(static_cast<int>(aligned_min_mant_sticky));

Expand All @@ -183,7 +180,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>();
}
Expand Down
41 changes: 27 additions & 14 deletions libc/test/src/math/AddTest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#ifndef LLVM_LIBC_TEST_SRC_MATH_ADDTEST_H
#define LLVM_LIBC_TEST_SRC_MATH_ADDTEST_H

#include "src/__support/CPP/algorithm.h"
#include "test/UnitTest/FEnvSafeTest.h"
#include "test/UnitTest/FPMatcher.h"
#include "test/UnitTest/Test.h"
Expand Down Expand Up @@ -36,29 +37,34 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
InFPBits::min_subnormal().uintval();

public:
typedef OutType (*AddFunc)(InType, InType);
using AddFunc = OutType (*)(InType, InType);

void test_subnormal_range(AddFunc func) {
constexpr InStorageType COUNT = 100'001;
constexpr InStorageType STEP =
(IN_MAX_SUBNORMAL_U - IN_MIN_SUBNORMAL_U) / COUNT;
for (InStorageType i = 0, v = 0, w = IN_MAX_SUBNORMAL_U; i <= COUNT;
++i, v += STEP, w -= STEP) {
InType x = InFPBits(v).get_val();
InType y = InFPBits(w).get_val();
constexpr int COUNT = 100'001;
constexpr InStorageType STEP = LIBC_NAMESPACE::cpp::max(
static_cast<InStorageType>((IN_MAX_SUBNORMAL_U - IN_MIN_SUBNORMAL_U) /
COUNT),
InStorageType(1));
for (InStorageType i = IN_MIN_SUBNORMAL_U; i <= IN_MAX_SUBNORMAL_U;
i += STEP) {
InType x = InFPBits(i).get_val();
InType y = InFPBits(static_cast<InStorageType>(IN_MAX_SUBNORMAL_U - i))
.get_val();
mpfr::BinaryInput<InType> input{x, y};
EXPECT_MPFR_MATCH_ALL_ROUNDING(mpfr::Operation::Add, input, func(x, y),
0.5);
}
}

void test_normal_range(AddFunc func) {
constexpr InStorageType COUNT = 100'001;
constexpr InStorageType STEP = (IN_MAX_NORMAL_U - IN_MIN_NORMAL_U) / COUNT;
for (InStorageType i = 0, v = 0, w = IN_MAX_NORMAL_U; i <= COUNT;
++i, v += STEP, w -= STEP) {
InType x = InFPBits(v).get_val();
InType y = InFPBits(w).get_val();
constexpr int COUNT = 100'001;
constexpr InStorageType STEP = LIBC_NAMESPACE::cpp::max(
static_cast<InStorageType>((IN_MAX_NORMAL_U - IN_MIN_NORMAL_U) / COUNT),
InStorageType(1));
for (InStorageType i = IN_MIN_NORMAL_U; i <= IN_MAX_NORMAL_U; i += STEP) {
InType x = InFPBits(i).get_val();
InType y =
InFPBits(static_cast<InStorageType>(IN_MAX_NORMAL_U - i)).get_val();
mpfr::BinaryInput<InType> input{x, y};
EXPECT_MPFR_MATCH_ALL_ROUNDING(mpfr::Operation::Add, input, func(x, y),
0.5);
Expand All @@ -71,4 +77,11 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
TEST_F(LlvmLibcAddTest, SubnormalRange) { test_subnormal_range(&func); } \
TEST_F(LlvmLibcAddTest, NormalRange) { test_normal_range(&func); }

#define LIST_ADD_SAME_TYPE_TESTS(suffix, OutType, InType, func) \
using LlvmLibcAddTest##suffix = AddTest<OutType, InType>; \
TEST_F(LlvmLibcAddTest##suffix, SubnormalRange) { \
test_subnormal_range(&func); \
} \
TEST_F(LlvmLibcAddTest##suffix, NormalRange) { test_normal_range(&func); }

#endif // LLVM_LIBC_TEST_SRC_MATH_ADDTEST_H
29 changes: 29 additions & 0 deletions libc/test/src/math/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2800,6 +2800,35 @@ add_fp_unittest(
libc.src.stdlib.srand
)

add_fp_unittest(
add_same_type_test
NEED_MPFR
SUITE
libc-math-unittests
SRCS
add_same_type_test.cpp
HDRS
AddTest.h
DEPENDS
libc.src.__support.CPP.algorithm
libc.src.__support.FPUtil.generic.add_sub
libc.src.__support.macros.properties.types
)

add_fp_unittest(
sub_same_type_test
NEED_MPFR
SUITE
libc-math-unittests
SRCS
sub_same_type_test.cpp
HDRS
SubTest.h
DEPENDS
libc.src.__support.CPP.algorithm
libc.src.__support.FPUtil.generic.add_sub
libc.src.__support.macros.properties.types
)

add_subdirectory(generic)
add_subdirectory(smoke)
Expand Down
39 changes: 26 additions & 13 deletions libc/test/src/math/SubTest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#ifndef LLVM_LIBC_TEST_SRC_MATH_SUBTEST_H
#define LLVM_LIBC_TEST_SRC_MATH_SUBTEST_H

#include "src/__support/CPP/algorithm.h"
#include "test/UnitTest/FEnvSafeTest.h"
#include "test/UnitTest/FPMatcher.h"
#include "test/UnitTest/Test.h"
Expand Down Expand Up @@ -39,26 +40,31 @@ class SubTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
using SubFunc = OutType (*)(InType, InType);

void test_subnormal_range(SubFunc func) {
constexpr InStorageType COUNT = 100'001;
constexpr InStorageType STEP =
(IN_MAX_SUBNORMAL_U - IN_MIN_SUBNORMAL_U) / COUNT;
for (InStorageType i = 0, v = 0, w = IN_MAX_SUBNORMAL_U; i <= COUNT;
++i, v += STEP, w -= STEP) {
InType x = InFPBits(v).get_val();
InType y = InFPBits(w).get_val();
constexpr int COUNT = 100'001;
constexpr InStorageType STEP = LIBC_NAMESPACE::cpp::max(
static_cast<InStorageType>((IN_MAX_SUBNORMAL_U - IN_MIN_SUBNORMAL_U) /
COUNT),
InStorageType(1));
for (InStorageType i = IN_MIN_SUBNORMAL_U; i <= IN_MAX_SUBNORMAL_U;
i += STEP) {
InType x = InFPBits(i).get_val();
InType y = InFPBits(static_cast<InStorageType>(IN_MAX_SUBNORMAL_U - i))
.get_val();
mpfr::BinaryInput<InType> input{x, y};
EXPECT_MPFR_MATCH_ALL_ROUNDING(mpfr::Operation::Sub, input, func(x, y),
0.5);
}
}

void test_normal_range(SubFunc func) {
constexpr InStorageType COUNT = 100'001;
constexpr InStorageType STEP = (IN_MAX_NORMAL_U - IN_MIN_NORMAL_U) / COUNT;
for (InStorageType i = 0, v = 0, w = IN_MAX_NORMAL_U; i <= COUNT;
++i, v += STEP, w -= STEP) {
InType x = InFPBits(v).get_val();
InType y = InFPBits(w).get_val();
constexpr int COUNT = 100'001;
constexpr InStorageType STEP = LIBC_NAMESPACE::cpp::max(
static_cast<InStorageType>((IN_MAX_NORMAL_U - IN_MIN_NORMAL_U) / COUNT),
InStorageType(1));
for (InStorageType i = IN_MIN_NORMAL_U; i <= IN_MAX_NORMAL_U; i += STEP) {
InType x = InFPBits(i).get_val();
InType y =
InFPBits(static_cast<InStorageType>(IN_MAX_NORMAL_U - i)).get_val();
mpfr::BinaryInput<InType> input{x, y};
EXPECT_MPFR_MATCH_ALL_ROUNDING(mpfr::Operation::Sub, input, func(x, y),
0.5);
Expand All @@ -71,4 +77,11 @@ class SubTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
TEST_F(LlvmLibcSubTest, SubnormalRange) { test_subnormal_range(&func); } \
TEST_F(LlvmLibcSubTest, NormalRange) { test_normal_range(&func); }

#define LIST_SUB_SAME_TYPE_TESTS(suffix, OutType, InType, func) \
using LlvmLibcSubTest##suffix = SubTest<OutType, InType>; \
TEST_F(LlvmLibcSubTest##suffix, SubnormalRange) { \
test_subnormal_range(&func); \
} \
TEST_F(LlvmLibcSubTest##suffix, NormalRange) { test_normal_range(&func); }

#endif // LLVM_LIBC_TEST_SRC_MATH_SUBTEST_H
25 changes: 25 additions & 0 deletions libc/test/src/math/add_same_type_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//===-- Unittests for fputil::generic::add --------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "AddTest.h"

#include "src/__support/FPUtil/generic/add_sub.h"
#include "src/__support/macros/properties/types.h"

#define ADD_FUNC(T) (LIBC_NAMESPACE::fputil::generic::add<T, T>)

LIST_ADD_SAME_TYPE_TESTS(Double, double, double, ADD_FUNC(double))
LIST_ADD_SAME_TYPE_TESTS(Float, float, float, ADD_FUNC(float))
LIST_ADD_SAME_TYPE_TESTS(LongDouble, long double, long double,
ADD_FUNC(long double))
#ifdef LIBC_TYPES_HAS_FLOAT16
LIST_ADD_SAME_TYPE_TESTS(Float16, float16, float16, ADD_FUNC(float16))
#endif
#ifdef LIBC_TYPES_HAS_FLOAT128
LIST_ADD_SAME_TYPE_TESTS(Float128, float128, float128, ADD_FUNC(float128))
#endif
34 changes: 32 additions & 2 deletions libc/test/src/math/smoke/AddTest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

#include "hdr/errno_macros.h"
#include "hdr/fenv_macros.h"
#include "src/__support/FPUtil/BasicOperations.h"
#include "src/__support/macros/properties/os.h"
#include "test/UnitTest/FEnvSafeTest.h"
#include "test/UnitTest/FPMatcher.h"
Expand Down Expand Up @@ -59,6 +58,10 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
#ifndef LIBC_TARGET_OS_IS_WINDOWS
using namespace LIBC_NAMESPACE::fputil::testing;

if (LIBC_NAMESPACE::fputil::get_fp_type<OutType>() ==
LIBC_NAMESPACE::fputil::get_fp_type<InType>())
return;

if (ForceRoundingMode r(RoundingMode::Nearest); r.success) {
EXPECT_FP_EQ_WITH_EXCEPTION(inf, func(in.max_normal, in.max_normal),
FE_OVERFLOW | FE_INEXACT);
Expand Down Expand Up @@ -136,6 +139,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, 0U, 0b10U).get_val()));
}
};

#define LIST_ADD_TESTS(OutType, InType, func) \
Expand All @@ -145,6 +158,23 @@ 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); }

#define LIST_ADD_SAME_TYPE_TESTS(suffix, OutType, InType, func) \
using LlvmLibcAddTest##suffix = AddTest<OutType, InType>; \
TEST_F(LlvmLibcAddTest##suffix, SpecialNumbers) { \
test_special_numbers(&func); \
} \
TEST_F(LlvmLibcAddTest##suffix, InvalidOperations) { \
test_invalid_operations(&func); \
} \
TEST_F(LlvmLibcAddTest##suffix, RangeErrors) { test_range_errors(&func); } \
TEST_F(LlvmLibcAddTest##suffix, InexactResults) { \
test_inexact_results(&func); \
} \
TEST_F(LlvmLibcAddTest##suffix, MixedNormality) { \
test_mixed_normality(&func); \
}

#endif // LLVM_LIBC_TEST_SRC_MATH_SMOKE_ADDTEST_H
32 changes: 32 additions & 0 deletions libc/test/src/math/smoke/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5160,3 +5160,35 @@ add_fp_unittest(
DEPENDS
libc.src.math.ddivf128
)

add_fp_unittest(
add_same_type_test
SUITE
libc-math-smoke-tests
SRCS
add_same_type_test.cpp
HDRS
AddTest.h
DEPENDS
libc.hdr.errno_macros
libc.hdr.fenv_macros
libc.src.__support.FPUtil.generic.add_sub
libc.src.__support.macros.properties.os
libc.src.__support.macros.properties.types
)

add_fp_unittest(
sub_same_type_test
SUITE
libc-math-smoke-tests
SRCS
sub_same_type_test.cpp
HDRS
SubTest.h
DEPENDS
libc.hdr.errno_macros
libc.hdr.fenv_macros
libc.src.__support.FPUtil.generic.add_sub
libc.src.__support.macros.properties.os
libc.src.__support.macros.properties.types
)
17 changes: 17 additions & 0 deletions libc/test/src/math/smoke/SubTest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ class SubTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
#ifndef LIBC_TARGET_OS_IS_WINDOWS
using namespace LIBC_NAMESPACE::fputil::testing;

if (LIBC_NAMESPACE::fputil::get_fp_type<OutType>() ==
LIBC_NAMESPACE::fputil::get_fp_type<InType>())
return;

if (ForceRoundingMode r(RoundingMode::Nearest); r.success) {
EXPECT_FP_EQ_WITH_EXCEPTION(inf, func(in.max_normal, in.neg_max_normal),
FE_OVERFLOW | FE_INEXACT);
Expand Down Expand Up @@ -147,4 +151,17 @@ class SubTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
TEST_F(LlvmLibcSubTest, RangeErrors) { test_range_errors(&func); } \
TEST_F(LlvmLibcSubTest, InexactResults) { test_inexact_results(&func); }

#define LIST_SUB_SAME_TYPE_TESTS(suffix, OutType, InType, func) \
using LlvmLibcSubTest##suffix = SubTest<OutType, InType>; \
TEST_F(LlvmLibcSubTest##suffix, SpecialNumbers) { \
test_special_numbers(&func); \
} \
TEST_F(LlvmLibcSubTest##suffix, InvalidOperations) { \
test_invalid_operations(&func); \
} \
TEST_F(LlvmLibcSubTest##suffix, RangeErrors) { test_range_errors(&func); } \
TEST_F(LlvmLibcSubTest##suffix, InexactResults) { \
test_inexact_results(&func); \
}

#endif // LLVM_LIBC_TEST_SRC_MATH_SMOKE_SUBTEST_H
Loading
Loading