Skip to content

Commit 1bb8b65

Browse files
overmightyovermighty
authored
[libc][math] Fix incorrect logic in fputil::generic::add_or_sub (#116129)
Fixes incorrect logic that went unnoticed until the function was tested with output and input types that have the same underlying floating-point format.
1 parent 2fbfbf4 commit 1bb8b65

File tree

13 files changed

+285
-45
lines changed

13 files changed

+285
-45
lines changed

‎libc/src/__support/FPUtil/generic/CMakeLists.txt‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ add_header_library(
5656
HDRS
5757
add_sub.h
5858
DEPENDS
59-
libc.hdr.errno_macros
6059
libc.hdr.fenv_macros
6160
libc.src.__support.CPP.algorithm
6261
libc.src.__support.CPP.bit

‎libc/src/__support/FPUtil/generic/add_sub.h‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
#ifndef LLVM_LIBC_SRC___SUPPORT_FPUTIL_GENERIC_ADD_SUB_H
1010
#define LLVM_LIBC_SRC___SUPPORT_FPUTIL_GENERIC_ADD_SUB_H
1111

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

113-
if (y_bits.is_zero()) {
114-
volatile InType tmp = y;
115-
if constexpr (IsSub)
116-
tmp = -tmp;
117-
return cast<OutType>(tmp);
118-
}
112+
if (y_bits.is_zero())
113+
return cast<OutType>(x);
119114
}
120115

121116
InType x_abs = x_bits.abs().get_val();
@@ -160,20 +155,22 @@ add_or_sub(InType x, InType y) {
160155
} else {
161156
InStorageType max_mant = max_bits.get_explicit_mantissa() << GUARD_BITS_LEN;
162157
InStorageType min_mant = min_bits.get_explicit_mantissa() << GUARD_BITS_LEN;
163-
int alignment =
164-
max_bits.get_biased_exponent() - min_bits.get_biased_exponent();
158+
159+
int alignment = (max_bits.get_biased_exponent() - max_bits.is_normal()) -
160+
(min_bits.get_biased_exponent() - min_bits.is_normal());
165161

166162
InStorageType aligned_min_mant =
167163
min_mant >> cpp::min(alignment, RESULT_MANTISSA_LEN);
168164
bool aligned_min_mant_sticky;
169165

170-
if (alignment <= 3)
166+
if (alignment <= GUARD_BITS_LEN)
171167
aligned_min_mant_sticky = false;
172-
else if (alignment <= InFPBits::FRACTION_LEN + 3)
173-
aligned_min_mant_sticky =
174-
(min_mant << (InFPBits::STORAGE_LEN - alignment)) != 0;
175-
else
168+
else if (alignment > InFPBits::FRACTION_LEN + GUARD_BITS_LEN)
176169
aligned_min_mant_sticky = true;
170+
else
171+
aligned_min_mant_sticky =
172+
(static_cast<InStorageType>(
173+
min_mant << (InFPBits::STORAGE_LEN - alignment))) != 0;
177174

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

@@ -183,7 +180,7 @@ add_or_sub(InType x, InType y) {
183180
result_mant = max_mant - (aligned_min_mant | min_mant_sticky);
184181
}
185182

186-
int result_exp = max_bits.get_exponent() - RESULT_FRACTION_LEN;
183+
int result_exp = max_bits.get_explicit_exponent() - RESULT_FRACTION_LEN;
187184
DyadicFloat result(result_sign, result_exp, result_mant);
188185
return result.template as<OutType, /*ShouldSignalExceptions=*/true>();
189186
}

‎libc/test/src/math/AddTest.h‎

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#ifndef LLVM_LIBC_TEST_SRC_MATH_ADDTEST_H
1010
#define LLVM_LIBC_TEST_SRC_MATH_ADDTEST_H
1111

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

3839
public:
39-
typedef OutType (*AddFunc)(InType, InType);
40+
using AddFunc = OutType (*)(InType, InType);
4041

4142
void test_subnormal_range(AddFunc func) {
42-
constexpr InStorageType COUNT = 100'001;
43-
constexpr InStorageType STEP =
44-
(IN_MAX_SUBNORMAL_U - IN_MIN_SUBNORMAL_U) / COUNT;
45-
for (InStorageType i = 0, v = 0, w = IN_MAX_SUBNORMAL_U; i <= COUNT;
46-
++i, v += STEP, w -= STEP) {
47-
InType x = InFPBits(v).get_val();
48-
InType y = InFPBits(w).get_val();
43+
constexpr int COUNT = 100'001;
44+
constexpr InStorageType STEP = LIBC_NAMESPACE::cpp::max(
45+
static_cast<InStorageType>((IN_MAX_SUBNORMAL_U - IN_MIN_SUBNORMAL_U) /
46+
COUNT),
47+
InStorageType(1));
48+
for (InStorageType i = IN_MIN_SUBNORMAL_U; i <= IN_MAX_SUBNORMAL_U;
49+
i += STEP) {
50+
InType x = InFPBits(i).get_val();
51+
InType y = InFPBits(static_cast<InStorageType>(IN_MAX_SUBNORMAL_U - i))
52+
.get_val();
4953
mpfr::BinaryInput<InType> input{x, y};
5054
EXPECT_MPFR_MATCH_ALL_ROUNDING(mpfr::Operation::Add, input, func(x, y),
5155
0.5);
5256
}
5357
}
5458

5559
void test_normal_range(AddFunc func) {
56-
constexpr InStorageType COUNT = 100'001;
57-
constexpr InStorageType STEP = (IN_MAX_NORMAL_U - IN_MIN_NORMAL_U) / COUNT;
58-
for (InStorageType i = 0, v = 0, w = IN_MAX_NORMAL_U; i <= COUNT;
59-
++i, v += STEP, w -= STEP) {
60-
InType x = InFPBits(v).get_val();
61-
InType y = InFPBits(w).get_val();
60+
constexpr int COUNT = 100'001;
61+
constexpr InStorageType STEP = LIBC_NAMESPACE::cpp::max(
62+
static_cast<InStorageType>((IN_MAX_NORMAL_U - IN_MIN_NORMAL_U) / COUNT),
63+
InStorageType(1));
64+
for (InStorageType i = IN_MIN_NORMAL_U; i <= IN_MAX_NORMAL_U; i += STEP) {
65+
InType x = InFPBits(i).get_val();
66+
InType y =
67+
InFPBits(static_cast<InStorageType>(IN_MAX_NORMAL_U - i)).get_val();
6268
mpfr::BinaryInput<InType> input{x, y};
6369
EXPECT_MPFR_MATCH_ALL_ROUNDING(mpfr::Operation::Add, input, func(x, y),
6470
0.5);
@@ -71,4 +77,11 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
7177
TEST_F(LlvmLibcAddTest, SubnormalRange) { test_subnormal_range(&func); } \
7278
TEST_F(LlvmLibcAddTest, NormalRange) { test_normal_range(&func); }
7379

80+
#define LIST_ADD_SAME_TYPE_TESTS(suffix, OutType, InType, func) \
81+
using LlvmLibcAddTest##suffix = AddTest<OutType, InType>; \
82+
TEST_F(LlvmLibcAddTest##suffix, SubnormalRange) { \
83+
test_subnormal_range(&func); \
84+
} \
85+
TEST_F(LlvmLibcAddTest##suffix, NormalRange) { test_normal_range(&func); }
86+
7487
#endif // LLVM_LIBC_TEST_SRC_MATH_ADDTEST_H

‎libc/test/src/math/CMakeLists.txt‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2800,6 +2800,35 @@ add_fp_unittest(
28002800
libc.src.stdlib.srand
28012801
)
28022802

2803+
add_fp_unittest(
2804+
add_same_type_test
2805+
NEED_MPFR
2806+
SUITE
2807+
libc-math-unittests
2808+
SRCS
2809+
add_same_type_test.cpp
2810+
HDRS
2811+
AddTest.h
2812+
DEPENDS
2813+
libc.src.__support.CPP.algorithm
2814+
libc.src.__support.FPUtil.generic.add_sub
2815+
libc.src.__support.macros.properties.types
2816+
)
2817+
2818+
add_fp_unittest(
2819+
sub_same_type_test
2820+
NEED_MPFR
2821+
SUITE
2822+
libc-math-unittests
2823+
SRCS
2824+
sub_same_type_test.cpp
2825+
HDRS
2826+
SubTest.h
2827+
DEPENDS
2828+
libc.src.__support.CPP.algorithm
2829+
libc.src.__support.FPUtil.generic.add_sub
2830+
libc.src.__support.macros.properties.types
2831+
)
28032832

28042833
add_subdirectory(generic)
28052834
add_subdirectory(smoke)

‎libc/test/src/math/SubTest.h‎

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#ifndef LLVM_LIBC_TEST_SRC_MATH_SUBTEST_H
1010
#define LLVM_LIBC_TEST_SRC_MATH_SUBTEST_H
1111

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

4142
void test_subnormal_range(SubFunc func) {
42-
constexpr InStorageType COUNT = 100'001;
43-
constexpr InStorageType STEP =
44-
(IN_MAX_SUBNORMAL_U - IN_MIN_SUBNORMAL_U) / COUNT;
45-
for (InStorageType i = 0, v = 0, w = IN_MAX_SUBNORMAL_U; i <= COUNT;
46-
++i, v += STEP, w -= STEP) {
47-
InType x = InFPBits(v).get_val();
48-
InType y = InFPBits(w).get_val();
43+
constexpr int COUNT = 100'001;
44+
constexpr InStorageType STEP = LIBC_NAMESPACE::cpp::max(
45+
static_cast<InStorageType>((IN_MAX_SUBNORMAL_U - IN_MIN_SUBNORMAL_U) /
46+
COUNT),
47+
InStorageType(1));
48+
for (InStorageType i = IN_MIN_SUBNORMAL_U; i <= IN_MAX_SUBNORMAL_U;
49+
i += STEP) {
50+
InType x = InFPBits(i).get_val();
51+
InType y = InFPBits(static_cast<InStorageType>(IN_MAX_SUBNORMAL_U - i))
52+
.get_val();
4953
mpfr::BinaryInput<InType> input{x, y};
5054
EXPECT_MPFR_MATCH_ALL_ROUNDING(mpfr::Operation::Sub, input, func(x, y),
5155
0.5);
5256
}
5357
}
5458

5559
void test_normal_range(SubFunc func) {
56-
constexpr InStorageType COUNT = 100'001;
57-
constexpr InStorageType STEP = (IN_MAX_NORMAL_U - IN_MIN_NORMAL_U) / COUNT;
58-
for (InStorageType i = 0, v = 0, w = IN_MAX_NORMAL_U; i <= COUNT;
59-
++i, v += STEP, w -= STEP) {
60-
InType x = InFPBits(v).get_val();
61-
InType y = InFPBits(w).get_val();
60+
constexpr int COUNT = 100'001;
61+
constexpr InStorageType STEP = LIBC_NAMESPACE::cpp::max(
62+
static_cast<InStorageType>((IN_MAX_NORMAL_U - IN_MIN_NORMAL_U) / COUNT),
63+
InStorageType(1));
64+
for (InStorageType i = IN_MIN_NORMAL_U; i <= IN_MAX_NORMAL_U; i += STEP) {
65+
InType x = InFPBits(i).get_val();
66+
InType y =
67+
InFPBits(static_cast<InStorageType>(IN_MAX_NORMAL_U - i)).get_val();
6268
mpfr::BinaryInput<InType> input{x, y};
6369
EXPECT_MPFR_MATCH_ALL_ROUNDING(mpfr::Operation::Sub, input, func(x, y),
6470
0.5);
@@ -71,4 +77,11 @@ class SubTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
7177
TEST_F(LlvmLibcSubTest, SubnormalRange) { test_subnormal_range(&func); } \
7278
TEST_F(LlvmLibcSubTest, NormalRange) { test_normal_range(&func); }
7379

80+
#define LIST_SUB_SAME_TYPE_TESTS(suffix, OutType, InType, func) \
81+
using LlvmLibcSubTest##suffix = SubTest<OutType, InType>; \
82+
TEST_F(LlvmLibcSubTest##suffix, SubnormalRange) { \
83+
test_subnormal_range(&func); \
84+
} \
85+
TEST_F(LlvmLibcSubTest##suffix, NormalRange) { test_normal_range(&func); }
86+
7487
#endif // LLVM_LIBC_TEST_SRC_MATH_SUBTEST_H

‎libc/test/src/math/add_same_type_test.cpp‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
//===-- Unittests for fputil::generic::add --------------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "AddTest.h"
10+
11+
#include "src/__support/FPUtil/generic/add_sub.h"
12+
#include "src/__support/macros/properties/types.h"
13+
14+
#define ADD_FUNC(T) (LIBC_NAMESPACE::fputil::generic::add<T, T>)
15+
16+
LIST_ADD_SAME_TYPE_TESTS(Double, double, double, ADD_FUNC(double))
17+
LIST_ADD_SAME_TYPE_TESTS(Float, float, float, ADD_FUNC(float))
18+
LIST_ADD_SAME_TYPE_TESTS(LongDouble, long double, long double,
19+
ADD_FUNC(long double))
20+
#ifdef LIBC_TYPES_HAS_FLOAT16
21+
LIST_ADD_SAME_TYPE_TESTS(Float16, float16, float16, ADD_FUNC(float16))
22+
#endif
23+
#ifdef LIBC_TYPES_HAS_FLOAT128
24+
LIST_ADD_SAME_TYPE_TESTS(Float128, float128, float128, ADD_FUNC(float128))
25+
#endif

‎libc/test/src/math/smoke/AddTest.h‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111

1212
#include "hdr/errno_macros.h"
1313
#include "hdr/fenv_macros.h"
14-
#include "src/__support/FPUtil/BasicOperations.h"
1514
#include "src/__support/macros/properties/os.h"
1615
#include "test/UnitTest/FEnvSafeTest.h"
1716
#include "test/UnitTest/FPMatcher.h"
@@ -59,6 +58,10 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
5958
#ifndef LIBC_TARGET_OS_IS_WINDOWS
6059
using namespace LIBC_NAMESPACE::fputil::testing;
6160

61+
if (LIBC_NAMESPACE::fputil::get_fp_type<OutType>() ==
62+
LIBC_NAMESPACE::fputil::get_fp_type<InType>())
63+
return;
64+
6265
if (ForceRoundingMode r(RoundingMode::Nearest); r.success) {
6366
EXPECT_FP_EQ_WITH_EXCEPTION(inf, func(in.max_normal, in.max_normal),
6467
FE_OVERFLOW | FE_INEXACT);
@@ -136,6 +139,16 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
136139
func(InType(1.0), in.min_denormal);
137140
EXPECT_FP_EXCEPTION(FE_INEXACT);
138141
}
142+
143+
void test_mixed_normality(AddFunc func) {
144+
if (LIBC_NAMESPACE::fputil::get_fp_type<OutType>() !=
145+
LIBC_NAMESPACE::fputil::get_fp_type<InType>())
146+
return;
147+
148+
EXPECT_FP_EQ(FPBits::create_value(Sign::POS, 2U, 0b1U).get_val(),
149+
func(InFPBits::create_value(Sign::POS, 2U, 0U).get_val(),
150+
InFPBits::create_value(Sign::POS, 0U, 0b10U).get_val()));
151+
}
139152
};
140153

141154
#define LIST_ADD_TESTS(OutType, InType, func) \
@@ -145,6 +158,23 @@ class AddTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
145158
test_invalid_operations(&func); \
146159
} \
147160
TEST_F(LlvmLibcAddTest, RangeErrors) { test_range_errors(&func); } \
148-
TEST_F(LlvmLibcAddTest, InexactResults) { test_inexact_results(&func); }
161+
TEST_F(LlvmLibcAddTest, InexactResults) { test_inexact_results(&func); } \
162+
TEST_F(LlvmLibcAddTest, MixedNormality) { test_mixed_normality(&func); }
163+
164+
#define LIST_ADD_SAME_TYPE_TESTS(suffix, OutType, InType, func) \
165+
using LlvmLibcAddTest##suffix = AddTest<OutType, InType>; \
166+
TEST_F(LlvmLibcAddTest##suffix, SpecialNumbers) { \
167+
test_special_numbers(&func); \
168+
} \
169+
TEST_F(LlvmLibcAddTest##suffix, InvalidOperations) { \
170+
test_invalid_operations(&func); \
171+
} \
172+
TEST_F(LlvmLibcAddTest##suffix, RangeErrors) { test_range_errors(&func); } \
173+
TEST_F(LlvmLibcAddTest##suffix, InexactResults) { \
174+
test_inexact_results(&func); \
175+
} \
176+
TEST_F(LlvmLibcAddTest##suffix, MixedNormality) { \
177+
test_mixed_normality(&func); \
178+
}
149179

150180
#endif // LLVM_LIBC_TEST_SRC_MATH_SMOKE_ADDTEST_H

‎libc/test/src/math/smoke/CMakeLists.txt‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5160,3 +5160,35 @@ add_fp_unittest(
51605160
DEPENDS
51615161
libc.src.math.ddivf128
51625162
)
5163+
5164+
add_fp_unittest(
5165+
add_same_type_test
5166+
SUITE
5167+
libc-math-smoke-tests
5168+
SRCS
5169+
add_same_type_test.cpp
5170+
HDRS
5171+
AddTest.h
5172+
DEPENDS
5173+
libc.hdr.errno_macros
5174+
libc.hdr.fenv_macros
5175+
libc.src.__support.FPUtil.generic.add_sub
5176+
libc.src.__support.macros.properties.os
5177+
libc.src.__support.macros.properties.types
5178+
)
5179+
5180+
add_fp_unittest(
5181+
sub_same_type_test
5182+
SUITE
5183+
libc-math-smoke-tests
5184+
SRCS
5185+
sub_same_type_test.cpp
5186+
HDRS
5187+
SubTest.h
5188+
DEPENDS
5189+
libc.hdr.errno_macros
5190+
libc.hdr.fenv_macros
5191+
libc.src.__support.FPUtil.generic.add_sub
5192+
libc.src.__support.macros.properties.os
5193+
libc.src.__support.macros.properties.types
5194+
)

‎libc/test/src/math/smoke/SubTest.h‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,10 @@ class SubTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
5858
#ifndef LIBC_TARGET_OS_IS_WINDOWS
5959
using namespace LIBC_NAMESPACE::fputil::testing;
6060

61+
if (LIBC_NAMESPACE::fputil::get_fp_type<OutType>() ==
62+
LIBC_NAMESPACE::fputil::get_fp_type<InType>())
63+
return;
64+
6165
if (ForceRoundingMode r(RoundingMode::Nearest); r.success) {
6266
EXPECT_FP_EQ_WITH_EXCEPTION(inf, func(in.max_normal, in.neg_max_normal),
6367
FE_OVERFLOW | FE_INEXACT);
@@ -147,4 +151,17 @@ class SubTest : public LIBC_NAMESPACE::testing::FEnvSafeTest {
147151
TEST_F(LlvmLibcSubTest, RangeErrors) { test_range_errors(&func); } \
148152
TEST_F(LlvmLibcSubTest, InexactResults) { test_inexact_results(&func); }
149153

154+
#define LIST_SUB_SAME_TYPE_TESTS(suffix, OutType, InType, func) \
155+
using LlvmLibcSubTest##suffix = SubTest<OutType, InType>; \
156+
TEST_F(LlvmLibcSubTest##suffix, SpecialNumbers) { \
157+
test_special_numbers(&func); \
158+
} \
159+
TEST_F(LlvmLibcSubTest##suffix, InvalidOperations) { \
160+
test_invalid_operations(&func); \
161+
} \
162+
TEST_F(LlvmLibcSubTest##suffix, RangeErrors) { test_range_errors(&func); } \
163+
TEST_F(LlvmLibcSubTest##suffix, InexactResults) { \
164+
test_inexact_results(&func); \
165+
}
166+
150167
#endif // LLVM_LIBC_TEST_SRC_MATH_SMOKE_SUBTEST_H

0 commit comments

Comments
 (0)