Implement f{32,64}.{ceil,floor,trunc} instructions #481

chfast · 2020-08-13T09:22:16Z

codecov · 2020-08-14T10:34:21Z

Codecov Report

Merging #481 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master     #481    +/-   ##
========================================
  Coverage   99.56%   99.56%            
========================================
  Files          54       54            
  Lines       17015    17163   +148     
========================================
+ Hits        16941    17089   +148     
  Misses         74       74

test/unittests/execute_floating_point_test.cpp

axic · 2020-08-14T18:19:43Z

test/unittests/execute_floating_point_test.cpp

+{
+    auto instance = instantiate(parse(this->get_unop_code(Instr::f32_ceil)));
+    const auto exec = [&](auto arg) { return execute(*instance, 0, {arg}); };
+


I think there should be a specific case for -q = -0 (if -1 < -q < 0) here.

The rules about infinity and zero matches that of C++ too (https://en.cppreference.com/w/cpp/numeric/math/ceil), but stil having tests here would be needed.

Not sure the rule requires quiet_nan, but needs a test too, because C++ just passes through any nan.

I can the special case.

Zeros and infinities are included in the positive_trunc_tests. Rules are simple and the same for all 4 rounding instructions, so I don't see point of having them duplicated everywhere.

NaNs are checked in unop_nan_propagation as this is common rules for all instructions (except some which ignore NaNs).

Those special cases also exists for floor/trunc (actually 2 for trunc).

Will check positive_trunc_tests.

What we could do instead of duplication, is just leave a comment that those rules are handled where.

I added this comment in the positive_trunc_tests.

axic · 2020-08-14T18:32:21Z

lib/fizzy/execute.cpp

+    static_assert(std::is_floating_point_v<T>);
+    if (std::isnan(value))
+        return std::numeric_limits<T>::quiet_NaN();
+    return std::ceil(value);


From https://en.cppreference.com/w/cpp/numeric/math/ceil:

What about this? The same applies for std::floor, and std::trunc.

We don't care if the rounding is exact or not so we can ignore this error. These are set in the errno or similar and can be accessed by special functions, e.g. https://en.cppreference.com/w/cpp/numeric/fenv/fetestexcept.

Okay, the documentation just kept calling it "floating point exception" and I thought it is an actual exception, not just a flag.

Can you leave perhaps a comment above ceil/floor that FE_INEXACT has no bearing on the correctness of this?

This is GCC documentation about the floating point exceptions: https://www.gnu.org/software/libc/manual/html_node/FP-Exceptions.html
They can trigger SIGFPE signal, but this is not default behavior.

Apparently can be forcefully disabled with fdisableexcept, see https://en.cppreference.com/w/cpp/numeric/fenv and https://www.gnu.org/savannah-checkouts/gnu/libc/manual/html_node/Control-Functions.html:

And there's the non-call-exceptions but that apparently also catches SIGSEGV.

I think eventually we could have an enter/leave function for setting up FP properties. "enter" in the beginning of execute and then "enter"/"leave" wrapping on host functions.

Depends on which is more likely: heavy use of ceil/floor/trunc/round or not, because these are concerned by the rounding settings and the exceptions.

Alternatively using the builtins wouldn't relieve us from this stdlib madness?

Alternatively using the builtins wouldn't relieve us from this stdlib madness?

This is not portable and we are using them currently anyway. This is the implementation of std::ceil() in libstdc++:

inline _GLIBCXX_CONSTEXPR float ceil(float __x) { return __builtin_ceilf(__x); }

I think sticking to the C stdlib is fine. If someone wants to enable GNU's extension to trap on FP exceptions be my guest.

If someone wants to enable GNU's extension to trap on FP exceptions be my guest.

Behaviour should be documented, and I'm leaning towards we should mention these in the README (since we don't have a complete API doc yet).

test/unittests/execute_floating_point_test.cpp

axic · 2020-08-14T20:16:29Z

test/unittests/execute_floating_point_test.cpp

+
+    // ftrunc(+q) = +0  (if 0 < +q < 1)
+    EXPECT_THAT(exec(FP<TypeParam>::Limits::denorm_min()), Result(TypeParam{0}));
+    EXPECT_THAT(exec(std::nextafter(TypeParam{1}, TypeParam{0})), Result(TypeParam{0}));


Should these have swapped operand: nextafter(0, 1) e.g. the smallest number larger than 0 ?

The denorm_min is the smallest > 0.
The nextafter(0, 1) is the largest < 1.

If nextafter(0, 1) is the largest < 1, then why do you have nextafter(1, 0) here?

Wrong comment.
nextafter(0,1) is denorm_min - i.e. it is the next after 0 towards 1.
nextafter(1,0) is the largest < 1.

gumb0 · 2020-08-17T10:30:24Z

test/unittests/execute_floating_point_test.cpp

+    auto instance = instantiate(parse(this->get_unop_code(Instr::f32_ceil)));
+    const auto exec = [&](auto arg) { return execute(*instance, 0, {arg}); };
+
+    // fceil(-q) = -0  (if -1 < -q < 0)


I'm not a fan of duplicating these cases, it might be confusing why it's tested two times.

Perhaps a comment, that it's tested in the loop over positive_trunc_tests, but added here for explicitness, would help.

These are not tested two times, these are some edge cases. I mean they are edge cases in the specification for this given function.

These are the same cases as positive_trunc_tests items
{std::nextafter(T{1}, T{0}), T{0}}
{L::denorm_min(), 0}
tested in the loop as

EXPECT_THAT(exec(-arg), Result(-expected_trunc)) << -arg << ": " << -expected_trunc;

similar in tests for other instructions

Are we fine here?

@gumb0 you are correct. I still prefer if we have clear single test cases for the edge cases listed in the specific for the given instruction. It is much easier to review that every case is tested.

Stuff hidden in those loops is extremely hard to review, in my opinion.

I'm fine with including it, already approved.

gumb0

Looks good to me.

axic · 2020-08-17T15:59:43Z

test/unittests/execute_floating_point_test.cpp

+    // fceil(-q) = -0  (if -1 < -q < 0)
+    // (also included in positive_trunc_tests, here for explicitness).
+    EXPECT_THAT(exec(std::nextafter(-TypeParam{1}, TypeParam{0})), Result(-TypeParam{0}));
+    EXPECT_THAT(exec(-FP<TypeParam>::Limits::denorm_min()), Result(-TypeParam{0}));


So this is -nextafter(0, 1) ?

chfast · 2020-08-18T18:15:25Z

There is an issue with std::floor implementation.
When rounding direction is set to FE_DOWNWARD, std::floor() returns -0 instead of 0 for the following configurations:

GCC9, Release, {float, double},
GCC10, Debug, float,

but GCC, Release, native build is fine.

axic · 2020-08-18T19:14:41Z

lib/fizzy/execute.cpp

+    // When rounding direction is set to FE_DOWNWARD the std::floor outputs -0 where it should +0.
+    // The following workarounds the issue by using the fact that the sign of the output must always
+    // match the sign of the input value.
+    return std::copysign(result, value);


Perhaps also leave the gcc bug link here (I know it was reported for ppc, but this seems similar).

That one is fixed. We need to report new one, but we need a reproducer first.

ceil that results in -0 doesn't have this problem?

ceil is tested the same way, and no issues.

Do you mean ceil() returning +0 where it should -0? The test cases of values from [-1,-0] are included.

axic · 2020-08-18T19:16:37Z

test/unittests/execute_floating_point_test.cpp

 #include <cmath>

+#pragma STDC FENV_ACCESS ON


I know it is possible to look what this means -- I just did that --, but perhaps nicer to leave a small warning here so that it is not removed in the future:

Note: this is important because our tests need complete control over floating point settings.

I would comment what pragma does (enables access and modification of the floating-point environment (rounding directions in particular) ).

Yes, I will add it.

axic · 2020-08-18T19:17:26Z

test/unittests/execute_floating_point_test.cpp

+    // and exponent 2**(mantissa digits without implicit leading 1).
+    // The main point of using int_only_begin is to tests nearest() as for int_only_begin - 0.5 and
+    // int_only_begin - 1.5 we have equal distance to nearby integer values.
+    // (Integer shift is used instead of std::pow() because of the clang compiler bug).


What clang compiler bug is this? I guess we have it linked in the hackmd, do we want to link to it here too?

I only have the clang_bug branch for now.

axic · 2020-08-18T19:21:51Z

test/unittests/execute_floating_point_test.cpp

+        // ftrunc(-q) = -0  (if -1 < -q < 0)
+        // (also included in positive_trunc_tests, here for explicitness).
+        EXPECT_THAT(exec(std::nextafter(-TypeParam{1}, TypeParam{0})), Result(-TypeParam{0}));
+        EXPECT_THAT(exec(-FP<TypeParam>::Limits::denorm_min()), Result(-TypeParam{0}));


Potentially all these would be easier to follow by using nextafter(0, -1) ?

I don't think so. I prefer using named values. And nextafter(0, -1) is probably -0.

axic

Looks good to me, but please squash the appropriate commits.

chfast · 2020-08-19T15:25:37Z

Looks good to me, but please squash the appropriate commits.

Done. I left the "GCC bug workaround" commit separated.

chfast requested review from gumb0 and axic August 13, 2020 16:40

chfast force-pushed the fp_ceil_floor_trunc branch 2 times, most recently from 886b042 to 559f967 Compare August 14, 2020 08:21

chfast marked this pull request as draft August 14, 2020 09:11

chfast force-pushed the fp_ceil_floor_trunc branch from 559f967 to fdbaebd Compare August 14, 2020 10:26

chfast changed the base branch from fp_sub to master August 14, 2020 10:26

chfast marked this pull request as ready for review August 14, 2020 10:26

chfast force-pushed the fp_ceil_floor_trunc branch from fdbaebd to 0cc72e3 Compare August 14, 2020 12:02

gumb0 reviewed Aug 14, 2020

View reviewed changes

test/unittests/execute_floating_point_test.cpp Outdated Show resolved Hide resolved

test/unittests/execute_floating_point_test.cpp Outdated Show resolved Hide resolved

chfast force-pushed the fp_ceil_floor_trunc branch 2 times, most recently from 0c96df1 to 91fa494 Compare August 14, 2020 13:44

gumb0 reviewed Aug 14, 2020

View reviewed changes

test/unittests/execute_floating_point_test.cpp Outdated Show resolved Hide resolved

chfast force-pushed the fp_ceil_floor_trunc branch 2 times, most recently from e098abc to e4c2652 Compare August 14, 2020 16:35

axic reviewed Aug 14, 2020

View reviewed changes

chfast force-pushed the fp_ceil_floor_trunc branch from e4c2652 to 1fcc1db Compare August 14, 2020 18:45

axic reviewed Aug 14, 2020

View reviewed changes

test/unittests/execute_floating_point_test.cpp Outdated Show resolved Hide resolved

axic reviewed Aug 14, 2020

View reviewed changes

axic mentioned this pull request Aug 14, 2020

Implement f{32,64}.nearest instructions #486

Merged

gumb0 reviewed Aug 17, 2020

View reviewed changes

chfast force-pushed the fp_ceil_floor_trunc branch from b42242f to 6f41e36 Compare August 17, 2020 10:55

gumb0 approved these changes Aug 17, 2020

View reviewed changes

axic reviewed Aug 17, 2020

View reviewed changes

chfast force-pushed the fp_ceil_floor_trunc branch from 6f41e36 to 31fa3a6 Compare August 18, 2020 17:48

chfast force-pushed the fp_ceil_floor_trunc branch from 31fa3a6 to a1532be Compare August 18, 2020 18:01

chfast requested review from gumb0 and axic August 18, 2020 18:40

chfast changed the base branch from master to ci_native_build August 18, 2020 18:41

Base automatically changed from ci_native_build to master August 18, 2020 18:49

chfast force-pushed the fp_ceil_floor_trunc branch from 05740dd to bb05127 Compare August 18, 2020 19:11

axic reviewed Aug 18, 2020

View reviewed changes

chfast force-pushed the fp_ceil_floor_trunc branch from 3db9364 to 08def8c Compare August 19, 2020 11:13

axic approved these changes Aug 19, 2020

View reviewed changes

chfast added 4 commits August 19, 2020 17:23

Implement f{32,64}.{ceil,floor,trunc} instructions

3e2a39f

test: Add tests for ceil, floor, trunc

ef67234

Add GCC floor bug workaround

e7547a6

ci: Update spectests expectations

b3282b4

chfast force-pushed the fp_ceil_floor_trunc branch from 08def8c to b3282b4 Compare August 19, 2020 15:24

gumb0 approved these changes Aug 19, 2020

View reviewed changes

chfast merged commit 1c713e2 into master Aug 19, 2020

chfast deleted the fp_ceil_floor_trunc branch August 19, 2020 18:33

Implement f{32,64}.{ceil,floor,trunc} instructions #481

Implement f{32,64}.{ceil,floor,trunc} instructions #481

Conversation

chfast commented Aug 13, 2020 • edited Loading

codecov bot commented Aug 14, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

chfast Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

gumb0 Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic left a comment

Choose a reason for hiding this comment

chfast commented Aug 19, 2020

chfast commented Aug 13, 2020 •

edited

Loading

codecov bot commented Aug 14, 2020 •

edited

Loading

axic Aug 14, 2020 •

edited

Loading

axic Aug 17, 2020 •

edited

Loading

axic Aug 14, 2020 •

edited

Loading

chfast Aug 17, 2020 •

edited

Loading

axic Aug 17, 2020 •

edited

Loading

gumb0 Aug 17, 2020 •

edited

Loading

chfast commented Aug 18, 2020 •

edited

Loading