test: Add floating-point utils #454

chfast · 2020-08-04T12:58:18Z

These are helpers mostly to inspect and build NaN "values". Wasm has concepts of canonical and arithmetic NaNs, which are not present in C/C++.

Some future usage examples:

codecov · 2020-08-04T15:08:32Z

Codecov Report

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

@@           Coverage Diff            @@
##           master     #454    +/-   ##
========================================
  Coverage   99.50%   99.51%            
========================================
  Files          52       54     +2     
  Lines       14939    15120   +181     
========================================
+ Hits        14865    15046   +181     
  Misses         74       74

test/utils/floating_point_utils.hpp

gumb0 · 2020-08-05T12:47:59Z

test/utils/floating_point_utils.hpp

+}
+
+template <typename T, typename = std::enable_if<std::is_floating_point_v<T>>>
+T nan(uint64_t payload = FPUtils<T>::CanonicalNaNPayload) noexcept


Couldn't we use std::nan/std::nanf for this? They seem to be able to set a payload, too.

True, but they do it in an implementation-defined manner related to printing NaN "values". Maybe there is a way to find a combination of chars that gives wasm canonical and arithmetic NaNs, but I decided to go with my own NaN inspection helpers. Also to learn the internal structure of floats.

Added some unit tests comparing with std::nan(). It produces only quiet (aka arithmetic) nans. I prefer testing others (i.e. signaling) nans too.

test/utils/floating_point_utils.hpp

gumb0 · 2020-08-06T12:38:48Z

Tests for FP(UintType u) are missing

test/utils/floating_point_utils.hpp

chfast · 2020-08-06T19:59:08Z

Tests for FP(UintType u) are missing

Added some tests for that. They are also used in nan().

axic · 2020-08-06T23:12:27Z

test/utils/asserts.hpp

-    else if constexpr (std::is_same_v<value_type, double>)
-        return arg.value.f64 == value;
+    if constexpr (std::is_floating_point_v<value_type>)
+        return arg.value.template as<value_type>() == fizzy::test::FP{value};


This C++ syntax of template just to be able to use as<value_type> is terribly, but it is what it is.

~~Why is this beneficial over the previous one, btw?~~ Wouldn't it be simpler here just comparing the binary representation?

I understand the FP class will come handy later.

The FP is comparing binary representation.

The template is a mandatory hint to the compiler that the < in as<value_type> is a template bracket. Fortunately, GCC is giving nice hints when you forget it.

axic · 2020-08-06T23:17:39Z

test/utils/floating_point_utils.hpp

+        return std::isnan(value) ? (as_uint() & mantissa_mask) : 0;
+    }
+
+    /// Build the NaN value with the given payload.


~~Is this the canonical one?~~

Perhaps add a comment it is canonical by default? Though from a reading-tests-perspective perhaps having nan() and cnan() or canonical_nan() is better.

I think I can make the argument explicit, as they are coming in groups, e.g. canon, canon+1, 1, ....

axic · 2020-08-06T23:21:14Z

Out of curiousity: could we add tests to the main cmake (next to the bigendian tests) which ensures the platform has the proper IEEE-754 / NaN support?

chfast · 2020-08-07T10:21:47Z

Out of curiousity: could we add tests to the main cmake (next to the bigendian tests) which ensures the platform has the proper IEEE-754 / NaN support?

See #462.

axic · 2020-08-07T11:03:45Z

test/unittests/floating_point_utils_test.cpp

+    EXPECT_EQ(FP(-FP64::Limits::denorm_min()).as_uint(), 0x800'0000000000001);
+    EXPECT_EQ(FP(1.0).as_uint(), 0x3FF'0000000000000);
+    EXPECT_EQ(FP(-1.0).as_uint(), 0xBFF'0000000000000);
+    EXPECT_EQ(FP(std::nextafter(1.0, 0.0)).as_uint(), 0x3FE'FFFFFFFFFFFFF);


Damn these helpers, so this is the 1.0 - <smallest representable value> (e.g. 0.999999999999999...)

Not exactly. This is the next representable value after 1.0 towards 0.0. But should be something like 0.9999....

axic · 2020-08-07T11:20:43Z

test/utils/floating_point_utils.hpp

+template <typename DstT, typename SrcT>
+DstT bit_cast(SrcT x) noexcept
+{
+    DstT z;


x, z are an interesting choice of combination :)

Took it from Go's BigInt, where z is always the return value. Want me to change it?

It is fine, does not matter too much. Would have used something like from/to, a/b, x/y, in/out -- but many of these could be subject to some clashes with certain compilers, so not worth the hassle.

test/unittests/floating_point_utils_test.cpp

axic · 2020-08-07T12:39:13Z

test/unittests/floating_point_utils_test.cpp

+    const auto one = 1.0;
+    const auto inf = FP64::Limits::infinity();
+    const auto cnan = FP64::nan(FP64::canon);
+    const auto snan = FP64::nan(1);


What is snan?

signaling nan

axic · 2020-08-07T12:40:34Z

test/utils/floating_point_utils.hpp

+template <typename T>
+struct FP
+{
+    static_assert(std::is_same_v<T, float> || std::is_same_v<T, double>);


Hm, do you also want to add a static_assert for float/double being iee754 or fine as we have it in value.h and/or the limits check below?

The static_assert(Limits::is_iec559); does exactly that, except only when FP is instantiated.

axic

Haven't conducted a 100% thorough review (depending on @gumb0 here), but this looks okay to me.

gumb0 · 2020-08-07T13:05:40Z

test/unittests/floating_point_utils_test.cpp

+    constexpr auto qnan = FP32::Limits::quiet_NaN();
+
+    EXPECT_EQ(FP(0.0f).nan_payload(), 0);
+    EXPECT_EQ(FP(FP32::Limits::signaling_NaN()).nan_payload(), 0x200000);


The exact payloads of Limits::quiet_NaN() and Limits::signaling_NaN() are implementation defined, aren't they? So maybe not worth it to put it into tests? Or maybe add a comment about this.

Moved to separate tests.

gumb0 · 2020-08-07T13:25:07Z

test/utils/floating_point_utils.hpp

+    friend bool operator!=(T a, FP b) noexcept { return FP{a} != b; }
+};
+
+FP(uint64_t)->FP<double>;


Nitpick: these two could be ordered float, then double, like using below

chfast force-pushed the fp_utils branch 3 times, most recently from 7e223b6 to e694fc7 Compare August 4, 2020 14:59

chfast requested review from axic and gumb0 August 4, 2020 15:12

chfast force-pushed the fp_utils branch from e694fc7 to 4beb4fd Compare August 4, 2020 19:52

gumb0 reviewed Aug 5, 2020

View reviewed changes

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

gumb0 reviewed Aug 5, 2020

View reviewed changes

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

gumb0 reviewed Aug 5, 2020

View reviewed changes

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

chfast force-pushed the fp_utils branch 3 times, most recently from e34d942 to d19a6fb Compare August 6, 2020 10:45

gumb0 reviewed Aug 6, 2020

View reviewed changes

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

gumb0 reviewed Aug 6, 2020

View reviewed changes

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

chfast force-pushed the fp_utils branch 3 times, most recently from 9dc8ab7 to f1de6c7 Compare August 6, 2020 11:28

gumb0 reviewed Aug 6, 2020

View reviewed changes

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

chfast force-pushed the fp_utils branch from f1de6c7 to d8d44aa Compare August 6, 2020 11:49

gumb0 mentioned this pull request Aug 6, 2020

test: Fix comparing of floating point results in spectest runner #460

Merged

gumb0 reviewed Aug 6, 2020

View reviewed changes

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

gumb0 reviewed Aug 6, 2020

View reviewed changes

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

gumb0 reviewed Aug 6, 2020

View reviewed changes

test/utils/floating_point_utils.hpp Outdated Show resolved Hide resolved

chfast force-pushed the fp_utils branch 2 times, most recently from 8c71f3e to 2b35f84 Compare August 6, 2020 19:58

axic reviewed Aug 6, 2020

View reviewed changes

axic reviewed Aug 7, 2020

View reviewed changes

gumb0 reviewed Aug 7, 2020

View reviewed changes

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

gumb0 reviewed Aug 7, 2020

View reviewed changes

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

chfast force-pushed the fp_utils branch from 33d5195 to d3199a8 Compare August 7, 2020 12:20

axic reviewed Aug 7, 2020

View reviewed changes

axic approved these changes Aug 7, 2020

View reviewed changes

gumb0 reviewed Aug 7, 2020

View reviewed changes

chfast force-pushed the fp_utils branch from d3199a8 to 707f100 Compare August 7, 2020 13:27

gumb0 approved these changes Aug 7, 2020

View reviewed changes

chfast added 2 commits August 7, 2020 15:32

test: Add floating-point utils

a4f238e

test: Use FP utils to compare values in Result()

167c41b

chfast force-pushed the fp_utils branch from 707f100 to 167c41b Compare August 7, 2020 13:32

chfast merged commit 55801bc into master Aug 7, 2020

chfast deleted the fp_utils branch August 7, 2020 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add floating-point utils #454

test: Add floating-point utils #454

chfast commented Aug 4, 2020 •

edited

Loading

codecov bot commented Aug 4, 2020 •

edited

Loading

gumb0 Aug 5, 2020 •

edited

Loading

chfast Aug 5, 2020

chfast Aug 6, 2020

gumb0 commented Aug 6, 2020

chfast commented Aug 6, 2020

axic Aug 6, 2020

axic Aug 6, 2020 •

edited

Loading

chfast Aug 7, 2020

axic Aug 6, 2020 •

edited

Loading

chfast Aug 7, 2020 •

edited

Loading

axic commented Aug 6, 2020

chfast commented Aug 7, 2020 •

edited

Loading

axic Aug 7, 2020 •

edited

Loading

chfast Aug 7, 2020

axic Aug 7, 2020

chfast Aug 7, 2020

axic Aug 7, 2020

axic Aug 7, 2020

chfast Aug 7, 2020

axic Aug 7, 2020

chfast Aug 7, 2020

axic left a comment

gumb0 Aug 7, 2020

chfast Aug 7, 2020

gumb0 Aug 7, 2020

chfast Aug 7, 2020

test: Add floating-point utils #454

test: Add floating-point utils #454

Conversation

chfast commented Aug 4, 2020 • edited Loading

codecov bot commented Aug 4, 2020 • edited Loading

Codecov Report

gumb0 Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Aug 6, 2020

chfast commented Aug 6, 2020

Choose a reason for hiding this comment

axic Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

chfast Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

axic commented Aug 6, 2020

chfast commented Aug 7, 2020 • edited Loading

axic Aug 7, 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

axic left a 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

chfast commented Aug 4, 2020 •

edited

Loading

codecov bot commented Aug 4, 2020 •

edited

Loading

gumb0 Aug 5, 2020 •

edited

Loading

axic Aug 6, 2020 •

edited

Loading

axic Aug 6, 2020 •

edited

Loading

chfast Aug 7, 2020 •

edited

Loading

chfast commented Aug 7, 2020 •

edited

Loading

axic Aug 7, 2020 •

edited

Loading