Implement f32.demote_f64 instruction #458

chfast · 2020-08-04T17:03:42Z

codecov · 2020-08-12T14:28:55Z

Codecov Report

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

@@           Coverage Diff            @@
##           master     #458    +/-   ##
========================================
  Coverage   99.55%   99.55%            
========================================
  Files          54       54            
  Lines       16569    16685   +116     
========================================
+ Hits        16495    16611   +116     
  Misses         74       74

lib/fizzy/execute.cpp

test/unittests/execute_floating_point_test.cpp

gumb0 · 2020-08-13T12:32:18Z

test/unittests/execute_floating_point_test.cpp

+    auto instance = instantiate(parse(wasm));
+
+    // The boundary input value that results in the infinity.
+    constexpr double lowest_to_inf = 0x1.ffffffp127;


I'm a bit confused and might be wrong, but cases p > lowest_to_inf and p < -lowest_to_inf are missing, and I think they are undefined behavior in C++, but defined to result in infinities in wasm.

I think it is defined if IEEE 754 is supported (as for some other cases) because then infinities also counts.

The minimum range of representable values for a floating type is the most negative finite floating-point number representable in that type through the most positive finite floating-point number representable in that type. In addition, if negative infinity is representable in a type, the range of that type is extended to all negative real numbers; likewise, if positive infinity is representable in a type, the range of that type is extended to all positive real numbers.

https://stackoverflow.com/a/25831946/725174

Although, I'm confused how to chose nearest value between max and infinity...

The cases you mentions are not present because lowest_to_inf already yields infinity, so for any p > lowest_to_inf we get infinity too. But I can add test for nextafter(lowest_to_inf).

This answers my question.

In the default IEEE 754 round-to-nearest mode, a few double values above the maximum finite float (that is, FLT_MAX) convert to FLT_MAX. The exact limit is the number midway between FLT_MAX (0x1.fffffep127 in C99 hexadecimal representation) and the next float number that could be represented if the exponent in the single-precision format had a larger range, 0x2.0p127. The limit is thus 0x1.ffffffp127 or approximately 3.4028235677973366e+38 in decimal.

https://stackoverflow.com/a/17751145/725174

Although I wander where wasm spec handles this, if anywhere.

I finally have explanation for the lowest_to_inf value (see comments).
I also added two cases for p > lowest_to_inf.

gumb0 · 2020-08-14T09:09:42Z

test/unittests/execute_floating_point_test.cpp

+    constexpr double f32_limit = 0x1p128;  // 2**128.
+
+    // The lower boundary input value that results in the infinity. The number is midway between
+    // f32_max and f32_limit. For this value rounding prefers infinity, because f32_limit is even.


I don't quite understand the last sentence. f32_limit would indeed be even if casted to int, but for floats there's no even/odd ?

Ah I guess it's about evenness of the mantissa part

Of course there is (at least in Wasm spec).

In this case f32_limit is even by definition.

But still confused, mantissa of f32_max is even, manitssa of f32_limit is odd

I think in both cases you are confused by the size of the f32 mantisa which is 23 bits. When written as 6 hex digits the last bit should always be 0. The mantissa full of ones is 1.fffffe (leading 1 is implicit). Then m = 0x7fffff and is odd.

The f32_limit is even by definition: even_N(+-limit_N) <=> true. But also mantissa of 1p128 is all zero therefore even.

I see, yes, I was confused by hex notation.

gumb0 · 2020-08-14T09:12:36Z

test/unittests/execute_floating_point_test.cpp

+        {-lowest_to_inf, -FP32::Limits::infinity()},
+
+        {std::nextafter(lowest_to_inf, FP64::Limits::infinity()), FP32::Limits::infinity()},
+        {-std::nextafter(lowest_to_inf, FP64::Limits::infinity()), -FP32::Limits::infinity()},


shouldn't this be

Suggested change

{-std::nextafter(lowest_to_inf, FP64::Limits::infinity()), -FP32::Limits::infinity()},

{std::nextafter(-lowest_to_inf, -FP64::Limits::infinity()), -FP32::Limits::infinity()},

It is the same. floats has symmetrical positive and negative ranges.

Changed to use only single variant consistently.

test/unittests/execute_floating_point_test.cpp

gumb0 · 2020-08-14T12:05:22Z

test/unittests/execute_floating_point_test.cpp

+
+    EXPECT_THAT(execute(*instance, 0, {0x1.fffffefffffffp0}), Result(0x1.fffffep0f));  // round down
+    EXPECT_THAT(execute(*instance, 0, {0x1.fffffe0000000p0}), Result(0x1.fffffep0f));  // exact
+    EXPECT_THAT(execute(*instance, 0, {0x1.fffffd0000001p0}), Result(0x1.fffffep0f));  // round up


Maye also add the case with double value in the middle between two floats, to show tie-to-even

gumb0 · 2020-08-14T12:05:48Z

test/unittests/execute_floating_point_test.cpp

@@ -92,7 +92,7 @@ struct WasmTypeName
 template <typename T>
 class execute_floating_point_types : public testing::Test
 {
-protected:
+public:


I want to access the list of NaNs from other test suite.

axic · 2020-08-14T12:56:11Z

test/unittests/execute_floating_point_test.cpp

+    auto instance = instantiate(parse(wasm));
+
+    constexpr double f32_max = FP32::Limits::max();
+    ASSERT_EQ(f32_max, 0x1.fffffep127);


Why is this assert here and not in limits? If here, can't this be made into a static_assert?

Can be. I just want to see what the value is.

axic · 2020-08-14T13:00:28Z

test/unittests/execute_floating_point_test.cpp

+    // The lower boundary input value that results in the infinity. The number is midway between
+    // f32_max and f32_limit. For this value rounding prefers infinity, because f32_limit is even.
+    constexpr double lowest_to_inf = (f32_max + f32_limit) / 2;
+    ASSERT_EQ(lowest_to_inf, 0x1.ffffffp127);


It doesn't really matter, but is there a reason this should assert_eq and not static_assert?

I started with static_assert, then decided that maybe it should build anyway even if not true. I guess static_assert may be less confusing for readers.

test/unittests/execute_floating_point_test.cpp

axic · 2020-08-14T13:52:53Z

test/unittests/execute_floating_point_test.cpp

+        {0x1.fffffd0000001p0, 0x1.fffffep0f},  // round up
+
+        {0x1.fffff8p0, 0x1.fffff8p0f},                      // exact (even)
+        {(0x1.fffff8p0 + 0x1.fffffap0) / 2, 0x1.fffff8p0},  // tie-to-even down


Suggested change

{(0x1.fffff8p0 + 0x1.fffffap0) / 2, 0x1.fffff8p0}, // tie-to-even down

{(0x1.fffff8p0 + 0x1.fffffap0) / 2, 0x1.fffff8p0f}, // tie-to-even down

axic · 2020-08-14T13:53:00Z

test/unittests/execute_floating_point_test.cpp

+        {0x1.fffff8p0, 0x1.fffff8p0f},                      // exact (even)
+        {(0x1.fffff8p0 + 0x1.fffffap0) / 2, 0x1.fffff8p0},  // tie-to-even down
+        {0x1.fffffap0, 0x1.fffffap0f},                      // exact (odd)
+        {(0x1.fffffap0 + 0x1.fffffcp0) / 2, 0x1.fffffcp0},  // tie-to-even up


Suggested change

{(0x1.fffffap0 + 0x1.fffffcp0) / 2, 0x1.fffffcp0}, // tie-to-even up

{(0x1.fffffap0 + 0x1.fffffcp0) / 2, 0x1.fffffcp0f}, // tie-to-even up

chfast requested review from axic and gumb0 August 4, 2020 17:03

chfast force-pushed the fp_promote branch from 32d282b to 13cbeab Compare August 5, 2020 08:21

chfast force-pushed the fp_demote branch from 7ef62ad to f8a6675 Compare August 5, 2020 08:21

chfast force-pushed the fp_promote branch from 13cbeab to d441269 Compare August 6, 2020 10:29

chfast force-pushed the fp_demote branch from f8a6675 to 20cdd1b Compare August 6, 2020 10:34

chfast force-pushed the fp_promote branch 6 times, most recently from 4fe85a3 to 6636ce0 Compare August 12, 2020 10:39

chfast force-pushed the fp_demote branch from 20cdd1b to 8dd6ebf Compare August 12, 2020 12:18

Base automatically changed from fp_promote to master August 12, 2020 13:51

chfast force-pushed the fp_demote branch from 8dd6ebf to 7728b8f Compare August 12, 2020 14:19

chfast force-pushed the fp_demote branch 3 times, most recently from 7d8ad08 to b5cef53 Compare August 12, 2020 16:33

gumb0 reviewed Aug 12, 2020

View reviewed changes

lib/fizzy/execute.cpp Show resolved Hide resolved

chfast force-pushed the fp_demote branch from b5cef53 to 15dba1e Compare August 12, 2020 16:53

axic reviewed Aug 12, 2020

View reviewed changes

test/unittests/execute_floating_point_test.cpp Show resolved Hide resolved

axic reviewed Aug 12, 2020

View reviewed changes

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

axic reviewed Aug 12, 2020

View reviewed changes

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

chfast force-pushed the fp_demote branch 2 times, most recently from a94bc35 to 111b100 Compare August 12, 2020 21:02

gumb0 reviewed Aug 13, 2020

View reviewed changes

chfast force-pushed the fp_demote branch 2 times, most recently from e59b743 to 6facac3 Compare August 13, 2020 20:07

chfast marked this pull request as draft August 14, 2020 07:30

chfast force-pushed the fp_demote branch from 6facac3 to d7524df Compare August 14, 2020 07:59

chfast marked this pull request as ready for review August 14, 2020 08:21

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_demote branch 2 times, most recently from 6014abe to 22adbb7 Compare August 14, 2020 09:54

axic reviewed Aug 14, 2020

View reviewed changes

test/unittests/execute_floating_point_test.cpp Show resolved Hide resolved

chfast force-pushed the fp_demote branch from 22adbb7 to 4f317ae Compare August 14, 2020 11:58

gumb0 reviewed Aug 14, 2020

View reviewed changes

chfast force-pushed the fp_demote branch 2 times, most recently from b5159f4 to 28be58f Compare August 14, 2020 12:32

axic reviewed Aug 14, 2020

View reviewed changes

gumb0 approved these changes Aug 14, 2020

View reviewed changes

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

chfast force-pushed the fp_demote branch from 28be58f to d99c375 Compare August 14, 2020 13:32

axic reviewed Aug 14, 2020

View reviewed changes

chfast added 3 commits August 14, 2020 16:29

Implement f32.demote_f64

6c26a86

test: Add tests for f32.demote_f64

821eb74

test: Extend NaN propagation tests for promote and demote

b0b31f4

chfast force-pushed the fp_demote branch from d99c375 to b0b31f4 Compare August 14, 2020 14:30

axic approved these changes Aug 14, 2020

View reviewed changes

chfast merged commit 439af1d into master Aug 14, 2020

chfast deleted the fp_demote branch August 14, 2020 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement f32.demote_f64 instruction #458

Implement f32.demote_f64 instruction #458

chfast commented Aug 4, 2020 •

edited

Loading

codecov bot commented Aug 12, 2020 •

edited

Loading

gumb0 Aug 13, 2020

chfast Aug 13, 2020

chfast Aug 13, 2020

chfast Aug 14, 2020

gumb0 Aug 14, 2020

gumb0 Aug 14, 2020

chfast Aug 14, 2020

gumb0 Aug 14, 2020

chfast Aug 14, 2020

gumb0 Aug 14, 2020

gumb0 Aug 14, 2020

chfast Aug 14, 2020

chfast Aug 14, 2020

gumb0 Aug 14, 2020

chfast Aug 14, 2020

gumb0 Aug 14, 2020

chfast Aug 14, 2020

axic Aug 14, 2020

chfast Aug 14, 2020

axic Aug 14, 2020

chfast Aug 14, 2020

axic Aug 14, 2020

axic Aug 14, 2020

	{-std::nextafter(lowest_to_inf, FP64::Limits::infinity()), -FP32::Limits::infinity()},
	{std::nextafter(-lowest_to_inf, -FP64::Limits::infinity()), -FP32::Limits::infinity()},

	{(0x1.fffff8p0 + 0x1.fffffap0) / 2, 0x1.fffff8p0}, // tie-to-even down
	{(0x1.fffff8p0 + 0x1.fffffap0) / 2, 0x1.fffff8p0f}, // tie-to-even down

	{(0x1.fffffap0 + 0x1.fffffcp0) / 2, 0x1.fffffcp0}, // tie-to-even up
	{(0x1.fffffap0 + 0x1.fffffcp0) / 2, 0x1.fffffcp0f}, // tie-to-even up

Implement f32.demote_f64 instruction #458

Implement f32.demote_f64 instruction #458

Conversation

chfast commented Aug 4, 2020 • edited Loading

codecov bot commented Aug 12, 2020 • edited Loading

Codecov Report

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

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

chfast commented Aug 4, 2020 •

edited

Loading

codecov bot commented Aug 12, 2020 •

edited

Loading