WASM.SIMD BitSelect & Shuffle ops + Tests

[Krovatkin]

This PR adds

• BitSelect + tests
• Shuffle + tests

---

This change is 

[Krovatkin]

@Cellule could you please take a look when you have a mnt? Thanks!

[Krovatkin]

ping @Cellule

[Krovatkin]

@Cellule ping.

[Cellule]

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

---

lib/Backend/IRBuilderAsmJs.cpp, line 3986 at r1 (raw file):

IRBuilderAsmJs::BuildInt32x4_4(Js::OpCodeAsmJs newOpcode, uint32 offset, BUILD_SIMD_ARGS_REG4)
{
    ValueType valueType = GetSimdValueTypeFromIRType(TySimd128F4);

Shouldn't the IRType be TySimd128I4 in this function ?

---

lib/Backend/IRBuilderAsmJs.cpp, line 5843 at r1 (raw file):

    IR::RegOpnd * src2Opnd = BuildSrcOpnd(GetRegSlotFromSimd128Reg(layout->R2), TySimd128U16);

    IR::RegOpnd * src3Opnd =  (IR::RegOpnd*)IR::IntConstOpnd::New(layout->INDICES[0], TyInt32, this->m_func);

Could we make a loop for these and AddExtendedArg instead of copy/pasting 16 times

---

lib/Runtime/ByteCode/AsmJsByteCodeWriter.cpp, line 256 at r1 (raw file):

    bool AsmJsByteCodeWriter::TryWriteAsmShuffle(OpCodeAsmJs op, RegSlot R0, RegSlot R1, RegSlot R2, uint8 indices[])
    {
        const uint32 MAX_LANES = 16;

Can we use Wasm::Simd::MAX_LANES instead ?
Same on other places where we have an hardcoded 16

---

lib/Runtime/ByteCode/OpCodesSimd.h, line 432 at r1 (raw file):

V8

Shouldn't this be U8 ?

---

lib/Runtime/Language/InterpreterProcessOpCodeAsmJs.h, line 2187 at r1 (raw file):

// v8x16shuffle
#define PROCESS_SIMD_V8X16_2I16toV8X16_1_COMMON(name, func, suffix) \

This is very specific, could we just make this a CUSTOM process and use a helper instead.

---

lib/WasmReader/WasmBinaryReader.cpp, line 605 at r1 (raw file):

void WasmBinaryReader::ShuffleNode()
{
    for (uint32 i = 0; i < Simd::MAX_LANES; i++)

Add CheckBytesLeft(Simd::MAX_LANES);

---

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

}

EmitInfo WasmBytecodeGenerator::EnregisterIntConst(uint constVal)

I don't see any other uses of this function, do we really need to extract this ?

---

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

EmitInfo WasmBytecodeGenerator::EmitM128BitSelect()
{
    EmitInfo mask = PopEvalStack(WasmTypes::M128, _u("Argument should be of type M128"));

We can leave the default error message for type mismatch. This message shows less information than the default

---

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

    }

    m_writer->AsmShuffle(Js::OpCodeAsmJs::Simd128_Shuffle_V8X16, resultReg, arg1Info.location, arg2Info.location, GetReader()->m_currentNode.shuffle.indices);

nit: just use indices here instead of using the reader again

---

Comments from Reviewable

[Krovatkin]

Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

lib/Runtime/ByteCode/OpCodesSimd.h, line 432 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

V8

Shouldn't this be U8 ?

There's already Simd128_Shuffle_U16 which uses a different implementation to extract indices. I find Simd128_Shuffle_V8X16 to be a bit more consistent w/ the corresponding WASM opcode.

---

Comments from Reviewable

[Krovatkin]

tagging @arunetm

[Cellule]

Reviewed 9 of 9 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

lib/Runtime/Language/InterpreterStackFrame.h, line 264 at r2 (raw file):

        template <class T> void OP_SimdUint32x4FromFloat32x4(const unaligned T* playout);
        template <class T> void OP_WasmSimdConst(const unaligned T* playout);
        template <class T> inline void OP_SimdShuffleV8X16(const unaligned T* playout);

nit: inline is not necessary

---

lib/WasmReader/WasmBinaryReader.cpp, line 609 at r2 (raw file):

        m_currentNode.shuffle.indices[i] = ReadConst<uint8>();
    }
    CheckBytesLeft(Simd::MAX_LANES);

We should check before reading the byte as it could potentially trigger an A/V

---

Comments from Reviewable

[MikeHolman]

why the change with AsmReg19? Seems like this isn't being used in wasm?

[Krovatkin]

Ah, good catch!

[Krovatkin]

@Cellule could you please give it another look?

[Krovatkin]

Review status: 22 of 25 files reviewed at latest revision, 3 unresolved discussions.

---

lib/WasmReader/WasmBinaryReader.cpp, line 609 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

We should check before reading the byte as it could potentially trigger an A/V

oh, my bad. I thought I'd put before the loop

---

Comments from Reviewable

[Cellule]

---

Reviewed 3 of 3 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[Cellule]

@dotnet-bot test this please

[Krovatkin]

@dotnet-bot test Windows 7 ci_dev12_x64_debug please

[Krovatkin]

@Cellule did you do test this because Windows 7 ci_dev12_x64_debug got stuck or was it another error?

[Krovatkin]

@Cellule @MikeHolman guys could one of you please help me to merge this change?

[Krovatkin]

@MikeHolman @Cellule , thank you!