SIMD: load/store lane, any_true, q-format multiply, i64x2.widen

[lars-t-hansen]

This adds the usual stuff for a number of newly approved instructions, that's the first four commits. The fifth commit is tricker: it removes two instructions and renames a third, because these instructions all had the same meaning. The code is simple but this breaks a ton of tests. Most of those tests are simple to fix (not included here - what do I do with my changes?) but some are not, because the test runner roundtrips through wabt and wabt needs to be updated. @alexcrichton, guidance welcome.

[alexcrichton]

For wabt tests using the old syntax it should be fine to update this whitelist of tests to not run.

Other than that looks good to me! Since we're losing some test coverage with wabt, mind adding a new test which covers at least the new instructions?

[lars-t-hansen]

For wabt tests using the old syntax it should be fine to update this whitelist of tests to not run.

Other than that looks good to me! Since we're losing some test coverage with wabt, mind adding a new test which covers at least the new instructions?

Sounds good. I expect the SIMD tests will be updated shortly and can then be pulled in here, but somebody's calling a do-over on some of the opcode assignments... I'll keep this PR alive but will tell you when I think it's safe to merge.

[lars-t-hansen]

Achievement unlocked: add a new argument type to the parser in expr.rs...

But we have a problem, there's a grammar ambiguity here. Consider v128.load8_lane 8. What is the 8? The existing wasm-tools parser insists that this is the (optional) memory index, for multi-memory implementations. I of course want it to be the lane index. The operand order to the instruction is (memarg, lane), but here the intent of the memarg was to be empty.

The multi-memory spec is pretty quiet on the matter. I think the best way to resolve this is for the memory index in a memarg to be denoted with a keyword in the manner of alignment and offset, memidx=n or memory=n.

[alexcrichton]

Oh dear, that indeed sounds bad. Want to hardcode that instruction's memory index as 0 for now and bring this up as an issue on the multi-memory repo? I suspect we'll probably do as you mention but it's probably easier to hardcode 0 for now and implement that later as necessary.

[lars-t-hansen]

I can do that.

[lars-t-hansen]

@alexcrichton, PTAL. This excludes a number of included-repo test cases that will fail until wabt is updated, adds tests for all new instructions (and a few that were affected by the exclusion), pins the memory index to zero, and disables some local tests that were affectd by the memory index change.

I did not run rustfmt since that seemed to have effects well beyond my work; LMK.

The assigned opcodes are unfortunately tentative and may change, but to the best of my understanding what I have here corresponds with what V8 has chosen for the time being. (I'll double-check that now, since some time has passed.)

[lars-t-hansen]

Looks like the opcodes used in the patch are in sync with current v8 code (assuming that v128.any_true maps to the opcode of i8x16.any_true, as discussed in the committee), and v8 is kept in sync with llvm and binaryen. We've agreed to renumber any new opcodes when the set of instructions has been frozen; opcodes that were renumbered in the last renumbering pass will not be affected.

In short, no more work remaining here for the time being.

[alexcrichton]

I'm not really sure how rustfmt decides to format these sorts of enum declarations but it should be fine to just run cargo fmt to appease it. If it changes things that you didn't touch that's ok. If you'd like, though, you could make the format changes a separate commit (doesn't matter much to me). Sometimes this can arise when the codebase was last formatted with 1.48 and 1.49 differs slightly, for example.

[alexcrichton]

To confirm, is the intention that you'd write:

v128.load8_lane offset=3 3 

for loading lane 3 at offset 3? As opposed to

v128.load8_lane 3 offset=3  

(I figured the land would want to be prioritized here?)

[alexcrichton]

I thought this would be the latter, that is, since otherwise this breaks the multi-memory support for all instructions in wast I think which I think may be a bit too breaking for now. Changing from an index/identifier to (memory $m) should resolve the ambiguity there, though, and could be implemented for now. That'd be parsed as Option<ItemRef<kw::memory>> in wast

[lars-t-hansen]

Yes, confirmed. According to the SIMD spec, the memory argument comes before the lane argument in the binary form, and arguably this is best for the text form too. (The SIMD spec, alas, uses a C++-oriented form and is not a ton of help, but the order of operands is memory arg before lane.)

I'm not sure what you mean by the comment that it breaks all multi-memory support. With the current code, which does not parse any memory index, there is no multi-memory support, hence all test cases that use a memory index are commented out.

Andreas also thought (memory $m) would be the correct syntax. I can look into making that work, and will re-enable tests if and when it works.

[alexcrichton]

Ok makes sense. If you'd like I can land the change to (memory ..) before this PR since ideally this PR wouldn't be affected by multi-memory at all.

[lars-t-hansen]

Your call. Since my patch now seems to do what it needs to do, I'm fine either way.

[lars-t-hansen]

This introduces the use of the (memory $m) form for normal memargs, and updates tests to use that form. Everything passes and I think no tests have been disabled except the ones blacklisted because wabt does not roundtrip the new instructions.

What I did not do here is require the (memory $m) form for some of the non-memarg instructions such as memory.copy. Somehow that seemed like too much for what was supposed to be a simple SIMD update... LMK.

[alexcrichton]

Thanks! To confirm would you want a publish as well?

[lars-t-hansen]

Yeah, a new version would be welcome. Thanks!

[alexcrichton]

Ok should be published now