Wasm.simd Negation, Splat, ReplaceLane + tests

[Krovatkin]

This PR includes:

• Negation op + tests
• Splat op + tests
• ReplaceLane op + tests

---

This change is 

[Krovatkin]

@Cellule @MikeHolman please ignore this PR for now until I fix any possible issues w/ x-plat.

fyi @arunetm

[Krovatkin]

we could change the indexing to a dict w/ an array view per each type where the type comes from a test name if this version is too ugly :-)

[Krovatkin]

@Cellule could you please take a look when you have a moment? The tests seem to have cleared

[Krovatkin]

@Cellule hate to bug you that often; could you please take a look when you have a moment?

[Cellule]

Reviewed 15 of 15 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1286 at r1 (raw file):

EmitInfo WasmBytecodeGenerator::EnregisterIndex() 
{
    const uint offset = GetReader()->m_currentNode.lane.index;

I think I would put the code in CheckLaneIndex in here that way if you register an index you have to check the index is not out of range.
Also, could we rename this function to something like EmitLaneIndex(Js::OpCode requestingOp)

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1303 at r1 (raw file):

    }

    EmitInfo simdArg = PopEvalStack();

use PopEvalStack(WasmTypes::M128, _u("simd argument type mismatch")) instead of checking the type manually after

---

test/wasm.simd/replaceLaneTests.js, line 31 at r1 (raw file):

Previously, Krovatkin (Nick Korovaiko) wrote…

we could change the indexing to a dict w/ an array view per each type where the type comes from a test name if this version is too ugly :-)

Yeah this is a little hard to read and it feels hacky. I would just pass the typed array and the length by arguments.

---

test/wasm.simd/rlexe.xml, line 47 at r1 (raw file):

<test>
  <default>
    <files>splatNegTests.js</files>

This is not needed, rl will do a second run with prejit

---

test/wasm.simd/rlexe.xml, line 59 at r1 (raw file):

<test>
  <default>
    <files>replaceLaneTests.js</files>

This is not needed, rl will do a second run with prejit

---

Comments from Reviewable

[Krovatkin]

Review status: 10 of 15 files reviewed at latest revision, 5 unresolved discussions.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1286 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

I think I would put the code in CheckLaneIndex in here that way if you register an index you have to check the index is not out of range.
Also, could we rename this function to something like EmitLaneIndex(Js::OpCode requestingOp)

yup, looks much better that way

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1303 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

use PopEvalStack(WasmTypes::M128, _u("simd argument type mismatch")) instead of checking the type manually after

fixed

---

test/wasm.simd/replaceLaneTests.js, line 31 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Yeah this is a little hard to read and it feels hacky. I would just pass the typed array and the length by arguments.

I tried passing both, the array view and length, and it seems like a lot of boilerplate that can be avoided. Also, it makes harder to add new tests since w/ two additional parameters it's easier to pass the wrong thing.
I think the dict approach offers a nice compromise

• it's much easier to add new tests
• there's very little magic in helper functions to get the length and array view.

https://github.com/Krovatkin/ChakraCore/blob/b998a530697ff2902e8666c6847f2798148ab8cc/test/wasm.simd/splatNegTests.js#L34
https://github.com/Krovatkin/ChakraCore/blob/b998a530697ff2902e8666c6847f2798148ab8cc/test/wasm.simd/splatNegTests.js#L47
https://github.com/Krovatkin/ChakraCore/blob/b998a530697ff2902e8666c6847f2798148ab8cc/test/wasm.simd/splatNegTests.js#L63

But if you insist on passing those as extra parameters, we could definitely do it.

---

test/wasm.simd/rlexe.xml, line 47 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

This is not needed, rl will do a second run with prejit

fixed

---

test/wasm.simd/rlexe.xml, line 59 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

This is not needed, rl will do a second run with prejit

fixed

---

Comments from Reviewable

[Krovatkin]

@Cellule I was wondering if you could take another quick look? If everything looks good, I'll squash the commits and ask you to help me to merge this PR. @arunetm is still trying to figure out his perms issue.

[Cellule]

---

Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

test/wasm.simd/replaceLaneTests.js, line 31 at r1 (raw file):

Previously, Krovatkin (Nick Korovaiko) wrote…

I tried passing both, the array view and length, and it seems like a lot of boilerplate that can be avoided. Also, it makes harder to add new tests since w/ two additional parameters it's easier to pass the wrong thing.
I think the dict approach offers a nice compromise

• it's much easier to add new tests
• there's very little magic in helper functions to get the length and array view.

https://github.com/Krovatkin/ChakraCore/blob/b998a530697ff2902e8666c6847f2798148ab8cc/test/wasm.simd/splatNegTests.js#L34
https://github.com/Krovatkin/ChakraCore/blob/b998a530697ff2902e8666c6847f2798148ab8cc/test/wasm.simd/splatNegTests.js#L47
https://github.com/Krovatkin/ChakraCore/blob/b998a530697ff2902e8666c6847f2798148ab8cc/test/wasm.simd/splatNegTests.js#L63

But if you insist on passing those as extra parameters, we could definitely do it.

This looks pretty good!

---

Comments from Reviewable

[Cellule]

I'll try to merge it this afternoon

[Krovatkin]

@Cellule Thank you very much!
We'll try to figure out perms issues on our end, so we at least don't have to burden you w/ merges.

[Krovatkin]

@Cellule, awesome! Thanks a lot!