Update testsuite (for SIMD) #1373

binji · 2020-03-24T02:04:07Z

Lots of changes necessary to make this work, as well as some bug fixes.

The main change is allowing nan:canonical and nan:arithmetic as a
possible value for each lane of a v128. This needs to propogate
through the parser, IR, the JSON format, and the spec interpreter.

This also changes the format of the spec JSON file, where a SIMD value
is now stored as a list of values instead of a single u128:

{"type": "v128", "lane_type": "i32", "value": ["0", "0", "0", "0"]}

Since the lane type can be i8 and i16, these types can now be used
in more places (not just the decompiler). They'll be used for the GC
proposal too (for packed values), so I've updated them to use the binary
value specified for that proposal.

Here are the actual SIMD fixes:

SIMD lanes are malformed if they don't match the binary format, but
invalid if they are smaller than the lane width. For example,
i8x16.extract_lane_s is malformed if the lane is >= 256, because the
lane is stored as a byte. But it is invalid if the lane is >= 16.
The i8x16.narrow_i16x8_u, i16x8.narrow_i32x4_u and
i64x2.load_32x2_u instructions were not handling sign-extension
propoerly.

TODO: This code is pretty clumsy now; it would be better to have a
universal Value and ExpectedValue that can be used everywhere, so
the logic doesn't need to be duplicated.

Lots of changes necessary to make this work, as well as some bug fixes. The main change is allowing `nan:canonical` and `nan:arithmetic` as a possible value for each lane of a `v128`. This needs to propogate through the parser, IR, the JSON format, and the spec interpreter. This also changes the format of the spec JSON file, where a SIMD value is now stored as a list of values instead of a single u128: ``` {"type": "v128", "lane_type": "i32", "value": ["0", "0", "0", "0"]} ``` Since the lane type can be `i8` and `i16`, these types can now be used in more places (not just the decompiler). They'll be used for the GC proposal too (for packed values), so I've updated them to use the binary value specified for that proposal. Here are the actual SIMD fixes: * SIMD lanes are malformed if they don't match the binary format, but invalid if they are smaller than the lane width. For example, `i8x16.extract_lane_s` is malformed if the lane is >= 256, because the lane is stored as a byte. But it is invalid if the lane is >= 16. * The `i8x16.narrow_i16x8_u`, `i16x8.narrow_i32x4_u` and `i64x2.load_32x2_u` instructions were not handling sign-extension propoerly. TODO: This code is pretty clumsy now; it would be better to have a universal `Value` and `ExpectedValue` that can be used everywhere, so the logic doesn't need to be duplicated.

sbc100 · 2020-03-24T15:48:02Z

src/binary-writer-spec.cc

+      break;
+
+    case ExpectedNan::Arithmetic:
+      WriteString("nan:arithmetic");


Unrelated: Why is this called BinaryWriter?

Good question, it should be JSONWriter I suppose. I think it was historical, because the old binary-writer used to contain this stuff too, so I probably kept the name when I split it out.

sbc100 · 2020-03-24T15:49:47Z

src/interp/interp.cc

    case O::I32X4Load16X4S: return DoSimdLoadExtend<s32x4, s16x4>(instr, out_trap);
    case O::I32X4Load16X4U: return DoSimdLoadExtend<u32x4, u16x4>(instr, out_trap);
    case O::I64X2Load32X2S: return DoSimdLoadExtend<s64x2, s32x2>(instr, out_trap);
-    case O::I64X2Load32X2U: return DoSimdLoadExtend<s64x2, s32x2>(instr, out_trap);


Is this simply a bugfix here? Why didn't it show up before?

There weren't any tests of the output before, this is a relatively new instruction.

sbc100 · 2020-03-24T15:51:08Z

src/ir.h

-  void set_f32(ExpectedNan nan) { From<float>(Type::F32, 0); nan_ = nan; }
-  void set_f64(ExpectedNan nan) { From<double>(Type::F64, 0); nan_ = nan; }
+  void set_f32(ExpectedNan nan) { set_f32(0); set_expected_nan(0, nan); }
+  void set_f64(ExpectedNan nan) { set_f64(0); set_expected_nan(0, nan); }


Is clang format OK with multiple statements on one line like this?

No, it will expand, though I kind of prefer keeping it on a single line for readability. What do you think?

Depends what you want the policy for wabt to be ? Absolute deferral to clang-format, or programmer discretion. I don't think there is one correct answer. It depends on the project. We don't have a lot of formatting discussions so allowing discretion isn't currently effecting productivity. So I'm OK with either way right now. We should probably document it though.

Yeah, I'm thinking I'll spend some time writing up a C++ style guide finally this week.

src/type.h

src/tools/spectest-interp.cc

sbc100 · 2020-03-24T15:55:27Z

src/tools/spectest-interp.cc

-  PARSE_KEY_STRING_VALUE("value", &value_str);
-  EXPECT("}");
+  ExpectedValue expected;
+  CHECK_RESULT(ParseExpectedValue(&expected));


Are all the consts in the JSON expected values? What about const argument values?

No, the expected values are more expressive. It's nice to be able to share the code since it's a significant amount of code. But yeah, I'll add an enum to make sure we don't parse stuff that isn't valid.

sbc100 · 2020-03-24T15:59:09Z

test/update-spec-tests.py

    ProcessProposalDir('bulk-memory-operations', '--enable-bulk-memory')
    ProcessProposalDir('reference-types', '--enable-reference-types')
+    ProcessProposalDir('simd', '--enable-simd')



Oops.. is this why there were so many new tests added here? I guess we were not updating the simd tests previously?

Yep, I guess there weren't SIMD tests for a while...

* Remove private from v128 (causes gcc warning) * Fix stack smashing in `Const` (need to use `set_expected_nan` to avoid writing past lane 3) * Add `AllowExpected` enum, used to prevent parsing `nan:canonical` and `nan:arithmetic` for const values (i.e. invocation arguments).

`u16 * u16` has the type `int`. If the values are 65535 and 65535, then the value can't be stored in an int (`0xfffe0001`), which makes UBSan complain. We want wrapping behavior anyway, so cast this to a T. Note that this isn't an issue with the other operations, since those all fit in an `int`.

binji requested a review from sbc100 March 24, 2020 02:04

sbc100 approved these changes Mar 24, 2020

View reviewed changes

binji added 4 commits March 24, 2020 12:08

Add comment for Mul

03e3529

Real fix for u16*u16 promotion issue

c884db5

binji merged commit 1f397a3 into master Mar 25, 2020

binji deleted the test-spec-simd branch March 25, 2020 19:43

sbc100 mentioned this pull request Mar 26, 2020

Update to more recent test suite #1379

Closed

Update testsuite (for SIMD) #1373

Update testsuite (for SIMD) #1373

Uh oh!

Conversation

binji commented Mar 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sbc100 Mar 24, 2020 •

edited

Loading