Support floating point globals #446

gumb0 · 2020-07-31T11:56:49Z

Depends on #445

codecov · 2020-07-31T13:44:35Z

Codecov Report

Merging #446 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
- Coverage   99.50%   99.50%   -0.01%     
==========================================
  Files          52       52              
  Lines       14813    14937     +124     
==========================================
+ Hits        14740    14863     +123     
- Misses         73       74       +1

gumb0 · 2020-08-03T10:56:55Z

lib/fizzy/parser.hpp

@@ -33,6 +33,18 @@ inline parser_result<uint8_t> parse_byte(const uint8_t* pos, const uint8_t* end)

    return {*pos, pos + 1};
 }
+template <typename T>
+
+inline parser_result<T> read_float_const(const uint8_t* pos, const uint8_t* end)


Should this be better named parse_float_const ?

read_constant()? read_constant_bytes()? But maybe keep the original name for now.

axic · 2020-08-03T17:49:54Z

lib/fizzy/parser.cpp

@@ -126,6 +126,18 @@ inline parser_result<GlobalType> parse_global_type(const uint8_t* pos, const uin
    return {type, pos};
 }

+template <typename T>
+inline parser_result<T> read_const(const uint8_t* pos, const uint8_t* end)


Is this read_const or just read/read_value/read_type?

I'm considering parse_float_value/parse_float_const because it returns parse_result and is similar to other parse_* functions above and below.

Reading parse_float_const<uint32_t>(..) is extremely confusing, so I would suggest to go with parse_value<uint32_t> or parse_type<uint32_t>, especially as this should do byteswapping should we support big endian targets. Right?

If parse_float_const<float> and parse_float_const<double> works that is fine too.

If parse_float_const<float> and parse_float_const<double> works that is fine too.

Not without a cast on a calling side.

That would work (but I would need to check assembly of that to be sure). But might be confusing as you can/will implement fN.const as iN.const: https://github.com/wasmx/fizzy/blob/master/lib/fizzy/execute.cpp#L1004-L1005.

Renamed to parse_value

axic · 2020-08-03T17:50:35Z

lib/fizzy/parser.cpp

        result.kind = ConstantExpression::Kind::Constant;
        result.value.constant = 0;
-        pos = skip(8, pos, end);
+        std::tie(result.value.constant, pos) = read_const<uint64_t>(pos, end);


This first commit is titled "move read_const form header to parser.cpp", yet it also changes the implementation here.

axic · 2020-08-03T17:50:51Z

lib/fizzy/parser.cpp

        constant_actual_type = ValType::f32;
        break;
    case Instr::f64_const:
-        // TODO: support this once floating points are implemented
        result.kind = ConstantExpression::Kind::Constant;
        result.value.constant = 0;


If this is removed for f32, should also be removed here.

gumb0 · 2020-08-03T18:03:19Z

Fixed the commits.

chfast · 2020-08-04T06:54:00Z

lib/fizzy/parser.hpp

@@ -33,6 +33,18 @@ inline parser_result<uint8_t> parse_byte(const uint8_t* pos, const uint8_t* end)

    return {*pos, pos + 1};
 }
+template <typename T>
+
+inline parser_result<T> read_float_const(const uint8_t* pos, const uint8_t* end)


read_constant()? read_constant_bytes()? But maybe keep the original name for now.

chfast · 2020-08-04T06:58:00Z

lib/fizzy/parser.cpp

        result.kind = ConstantExpression::Kind::Constant;
-        result.value.constant = 0;
-        pos = skip(4, pos, end);


The skip() function is probably unused now.

axic · 2020-08-04T15:16:45Z

lib/fizzy/execute.cpp

@@ -508,7 +508,8 @@ std::unique_ptr<Instance> instantiate(Module module,
    datasec_offsets.reserve(module.datasec.size());
    for (const auto& data : module.datasec)
    {
-        const uint64_t offset = eval_constant_expression(data.offset, imported_globals, globals);
+        const uint64_t offset =
+            eval_constant_expression(data.offset, imported_globals, globals).i64;


Aren't these two validated to be uin32_t? And if so, the check below doesn't need to operate on 128-bit for overflow checks.

It's validated to be 32 bit, and this, together with validation of data.init vector length to be 32 bit, guarantees that offset + data.init.size() won't overflow 64 bit.

If we do here as<uint32_t>(), the check below will implicitly cast it back to 64-bit anyway, so I'm not sure it's worth it.

Perhaps adding a note that these are actually 32-bit could be useful?

Added comments here and for elem section.

axic · 2020-08-04T15:16:51Z

lib/fizzy/execute.cpp

@@ -521,7 +522,8 @@ std::unique_ptr<Instance> instantiate(Module module,
    elementsec_offsets.reserve(module.elementsec.size());
    for (const auto& element : module.elementsec)
    {
-        const uint64_t offset = eval_constant_expression(element.offset, imported_globals, globals);
+        const uint64_t offset =


axic · 2020-08-06T23:07:16Z

lib/fizzy/parser.hpp

@@ -34,6 +34,18 @@ inline parser_result<uint8_t> parse_byte(const uint8_t* pos, const uint8_t* end)
    return {*pos, pos + 1};
 }

+template <typename T>
+inline parser_result<T> parse_value(const uint8_t* pos, const uint8_t* end)


Since this needs to be rebased, you may want to update the commit message to read parse_value insead of read_const, but it is not important.

It's "move and rename read_const" I would say

gumb0 force-pushed the float-globals branch from f0597f0 to 3a23f0e Compare July 31, 2020 12:05

gumb0 mentioned this pull request Jul 31, 2020

Support floats in spectest runner #445

Merged

gumb0 force-pushed the spectest-float branch from 24d978e to 79e7789 Compare July 31, 2020 12:30

gumb0 force-pushed the float-globals branch from 3a23f0e to fa22d37 Compare July 31, 2020 12:41

gumb0 force-pushed the spectest-float branch from 135371f to 284efe9 Compare July 31, 2020 13:20

gumb0 force-pushed the float-globals branch 2 times, most recently from 4214d5a to e5a248d Compare July 31, 2020 13:36

gumb0 force-pushed the float-globals branch from 099db4e to e5a248d Compare July 31, 2020 16:22

gumb0 force-pushed the spectest-float branch from f4f571e to 8674884 Compare July 31, 2020 16:39

Base automatically changed from spectest-float to master July 31, 2020 16:48

gumb0 force-pushed the float-globals branch 2 times, most recently from b736b49 to 75d90f2 Compare August 3, 2020 10:48

gumb0 commented Aug 3, 2020

View reviewed changes

gumb0 force-pushed the float-globals branch 6 times, most recently from a966fb7 to 967799e Compare August 3, 2020 13:32

gumb0 mentioned this pull request Aug 3, 2020

Replace static_casts with Value::as<> #448

Merged

gumb0 force-pushed the float-globals branch 7 times, most recently from 9a536a0 to 287f1fa Compare August 3, 2020 15:51

gumb0 changed the base branch from master to result-matcher-float August 3, 2020 15:52

gumb0 marked this pull request as ready for review August 3, 2020 16:00

gumb0 requested review from chfast and axic August 3, 2020 16:07

Base automatically changed from result-matcher-float to master August 3, 2020 17:49

axic reviewed Aug 3, 2020

View reviewed changes

gumb0 force-pushed the float-globals branch from 287f1fa to fe00202 Compare August 3, 2020 17:59

gumb0 requested a review from axic August 3, 2020 18:06

chfast reviewed Aug 4, 2020

View reviewed changes

gumb0 force-pushed the float-globals branch from fe00202 to 5221f03 Compare August 4, 2020 10:21

chfast approved these changes Aug 4, 2020

View reviewed changes

axic reviewed Aug 4, 2020

View reviewed changes

axic reviewed Aug 6, 2020

View reviewed changes

axic approved these changes Aug 6, 2020

View reviewed changes

gumb0 added 3 commits August 7, 2020 11:49

refactor: move read_const to parser.hpp and rename to parse_value

e50ea11

Support floating-point globals

ec2752a

ci: update spectest expectations

b9a4a6f

gumb0 force-pushed the float-globals branch from 78ee266 to f1a036d Compare August 7, 2020 09:53

gumb0 added 3 commits August 7, 2020 11:56

test: update smoketest expectations

5df413f

test: floating point globals

8bc7574

Represent constant in ConstantExpression with Value

12ac9ef

gumb0 force-pushed the float-globals branch from f1a036d to 12ac9ef Compare August 7, 2020 09:56

gumb0 merged commit 366aacf into master Aug 7, 2020

gumb0 deleted the float-globals branch August 7, 2020 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support floating point globals #446

Support floating point globals #446

gumb0 commented Jul 31, 2020 •

edited

Loading

codecov bot commented Jul 31, 2020 •

edited

Loading

gumb0 Aug 3, 2020

chfast Aug 4, 2020

axic Aug 3, 2020

gumb0 Aug 3, 2020

axic Aug 3, 2020

axic Aug 3, 2020

gumb0 Aug 3, 2020

chfast Aug 3, 2020

gumb0 Aug 4, 2020

axic Aug 3, 2020

axic Aug 3, 2020

gumb0 Aug 4, 2020

gumb0 commented Aug 3, 2020

chfast Aug 4, 2020

chfast Aug 4, 2020

gumb0 Aug 4, 2020

axic Aug 4, 2020

gumb0 Aug 4, 2020

axic Aug 6, 2020

gumb0 Aug 7, 2020 •

edited

Loading

axic Aug 4, 2020

axic Aug 6, 2020

gumb0 Aug 7, 2020

Support floating point globals #446

Support floating point globals #446

Conversation

gumb0 commented Jul 31, 2020 • edited Loading

codecov bot commented Jul 31, 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

gumb0 commented Aug 3, 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

Choose a reason for hiding this comment

gumb0 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

gumb0 commented Jul 31, 2020 •

edited

Loading

codecov bot commented Jul 31, 2020 •

edited

Loading

gumb0 Aug 7, 2020 •

edited

Loading