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

gumb0 · 2020-08-05T17:48:39Z

No description provided.

codecov · 2020-08-05T17:59:16Z

Codecov Report

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

@@           Coverage Diff           @@
##           master     #460   +/-   ##
=======================================
  Coverage   99.51%   99.51%           
=======================================
  Files          54       54           
  Lines       15120    15153   +33     
=======================================
+ Hits        15046    15079   +33     
  Misses         74       74

chfast

Any smoketests additions?

chfast · 2020-08-05T18:21:41Z

test/spectests/spectests.cpp

+bool float_values_equal(fizzy::Value value1, fizzy::Value value2) noexcept
+{
+    if (std::isnan(value1.as<float>()))
+        return value1.as<uint32_t>() == value2.as<uint32_t>();  // compare binary representations


This is quite close to undefined behavior: only single union member may be active at any point in time and you are allowed to only access the active one. Here you are accessing two members is a row - one of the accesses must be wrong.

But also to simplify: you can always compare binary representation of floats. I would just leave return value1.as<uint32_t>() == value2.as<uint32_t>();.

Also: add the following check in value.hpp: static_assert(offsetof(Value, f32) == offsetof(Value, i32));.

(now I see why you needed to_uint64/from_uint64)

Well ok, then the current way here it will be UB only for NaN case, but if I do always as<uint32_t>() then always...

That's actually not true, because you always set i64 to active in read_value(). In theory, you are never allowed to access f32 nor f64. But I doubt this will cause issues in practice. However, I had issues with performing type punning using unions in GCC before (when the misuse is obvious, in the same function).

But my motivation to always use "binary" representation for comparison is to make sure this is a reliable method and there are no regressions in running spectests.

This was replaced to comparison using FP utility from #454

chfast · 2020-08-06T07:35:21Z

No spectests results change is expected here?

gumb0 · 2020-08-06T10:07:22Z

No spectests results change is expected here?

No, I guess because all NaN tests are skipped on master so far.

gumb0 · 2020-08-06T14:59:30Z

test/spectests/spectests.cpp

+        const uint64_t uint_value = std::stoull(v.at("value").get<std::string>());
+
+        if (type == "i32" || type == "i64")
+            return uint_value;


I decided for now not to complicate it with checking if the value declared as 32-bit is really not wider. It won't result in UB if it happens.

axic · 2020-08-07T14:06:59Z

test/smoketests/spectests/failures/failures.wast

@@ -40,9 +40,15 @@
 ;; invalid result
 (module
    (func (export "foo.i32") (result i32) (i32.const 1))
+    (func (export "foo.i64") (result i64) (i64.const 1))
+    (func (export "foo.f32") (result f32) (f32.const 1))
+    (func (export "foo.f64") (result f64) (f64.const 1))


I think it would be better to use actual floats for these, to be sure. Such as 1.234.

axic · 2020-08-07T14:08:11Z

test/spectests/spectests.cpp

-        if (arg_type != "i32" && arg_type != "i64" && arg_type != "f32" && arg_type != "f64")
+        const auto type = v.at("type").get<std::string>();
+
+        // JSON tests have all values including floats serialized as 64-bit unsigned integers


Not that JSON supports the whole 64-bit 😉

The format supports arbitrary long literals, in theory.

gumb0 requested review from chfast and axic August 5, 2020 17:59

chfast requested changes Aug 5, 2020

View reviewed changes

gumb0 force-pushed the spectest-float-comparison branch from 35c33d6 to 70ca840 Compare August 5, 2020 18:39

chfast force-pushed the spectest-float-comparison branch from 70ca840 to aaf1127 Compare August 6, 2020 11:03

chfast changed the base branch from master to fp_utils August 6, 2020 11:03

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

gumb0 force-pushed the spectest-float-comparison branch 4 times, most recently from ca41f68 to d204758 Compare August 6, 2020 13:49

gumb0 commented Aug 6, 2020

View reviewed changes

gumb0 force-pushed the spectest-float-comparison branch from b9f3a27 to ee2d12e Compare August 6, 2020 16:09

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

gumb0 mentioned this pull request Aug 7, 2020

Implement floating point memory instructions #456

Merged

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

gumb0 force-pushed the spectest-float-comparison branch 2 times, most recently from bd36d5e to 2196c0a Compare August 7, 2020 13:20

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

gumb0 force-pushed the spectest-float-comparison branch from 2196c0a to ad1bee0 Compare August 7, 2020 13:30

gumb0 requested a review from chfast August 7, 2020 13:31

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

Base automatically changed from fp_utils to master August 7, 2020 13:51

gumb0 added 3 commits August 7, 2020 15:57

test: Fix reading uint->float and comparison of floating point results

4a86042

test: add smoketest for comparing NaN values

4f6cdf4

test: Correctly output result mismatch error values in spectest runner

fd7fed9

gumb0 force-pushed the spectest-float-comparison branch from ad1bee0 to 2ef71c1 Compare August 7, 2020 13:57

chfast approved these changes Aug 7, 2020

View reviewed changes

axic reviewed Aug 7, 2020

View reviewed changes

test: add smoketest for test failures with i64/f32/f64 results

6c48a0e

gumb0 force-pushed the spectest-float-comparison branch from 2ef71c1 to 6c48a0e Compare August 7, 2020 14:15

axic approved these changes Aug 7, 2020

View reviewed changes

gumb0 merged commit 162c066 into master Aug 7, 2020

gumb0 deleted the spectest-float-comparison branch August 7, 2020 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gumb0 commented Aug 5, 2020

codecov bot commented Aug 5, 2020 •

edited

Loading

chfast left a comment

chfast Aug 5, 2020

gumb0 Aug 5, 2020

chfast Aug 5, 2020

gumb0 Aug 6, 2020

chfast commented Aug 6, 2020

gumb0 commented Aug 6, 2020

gumb0 Aug 6, 2020

axic Aug 7, 2020

gumb0 Aug 7, 2020

axic Aug 7, 2020

chfast Aug 7, 2020

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

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

Conversation

gumb0 commented Aug 5, 2020

codecov bot commented Aug 5, 2020 • edited Loading

Codecov Report

chfast 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 6, 2020

gumb0 commented Aug 6, 2020

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

codecov bot commented Aug 5, 2020 •

edited

Loading