WebAssembly SIMD support in Chakra

[arunetm-zz]

This PR contains the implementation of the current WASM SIMD specification
Spec: https://github.com/WebAssembly/simd

---

This change is 

[arunetm-zz]

@dotnet-bot test OSX static_osx_osx_debug pleaese

[Cellule]

I am not yet done with the review, but I have a few action items for you to handle so I am publishing my progress for now

---

Reviewed 65 of 129 files at r1, 2 of 22 files at r2.
Review status: 67 of 129 files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

---

lib/WasmReader/WasmBinaryOpCodes.h, line 11 at r2 (raw file):

#endif

#ifndef WASM_OPCODE

nit: could you move this back to the top

---

lib/WasmReader/WasmBinaryOpCodes.h, line 88 at r2 (raw file):

WASM_SIGNATURE(Limit,   1,   WasmTypes::Void)

#include "WasmBinaryOpcodesSimd.h"

could we move this to the bottom instead and wrap it with #ifdef ENABLE_WASM_SIMD

---

lib/WasmReader/WasmBinaryOpcodesSimd.h, line 8 at r2 (raw file):

//temporary until 64x2 types are implemented
#ifndef FOREACH_SIMD_TYPE

Since there is only one type and it might stay like this for a little while.
Can we remove this macro and just use M128 instead where it is used

---

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

};

const char16* GetStrId(WasmType type)

WasmTypes::GetTypeName does the same thing as this function, we should remove this one.
Also, remove strIds

---

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

}

bool IsSIMDType(WasmTypes::WasmType type)

Looks like this is unused, can we clean it up please ?

---

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

            m_currentNode.cnst.v128[i] = ReadConst<uint>();
        }
        m_funcState.count += sizeof(m_currentNode.cnst.v128);

Here I think I would prefer m_funcState.count += Simd::VEC_WIDTH
Technically it should be equivalent, but what we're really doing is read Simd::VEC_WIDTH bytes

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 213 at r2 (raw file):

    case WasmTypes::F64: return Js::AsmJsVarType::Double;
    case WasmTypes::M128:  return Js::AsmJsVarType::Float32x4;
    //case WasmTypes::I2:  return Js::AsmJsVarType::Int64x2; @TODO

Could you cleanup those comments please ?

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 474 at r2 (raw file):

    m_alloc(_u("WasmBytecodeGen"), scriptContext->GetThreadContext()->GetPageAllocator(), Js::Throw::OutOfMemory),
    m_evalStack(&m_alloc),
    mTypedRegisterAllocator(&m_alloc, AllocateRegisterSpace, 0),

Here we should do
mTypedRegisterAllocator(&m_alloc, AllocateRegisterSpace, CONFIG_FLAG(WasmSimd) ? 0 : 1 << WAsmJs::SIMD),

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 623 at r2 (raw file):

    const WasmTypes::WasmType type = signature[1];

    Js::RegSlot resultReg = GetRegisterSpace(signature[0])->AcquireTmpRegister();

nit: use resultType instead of signature[0]

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 625 at r2 (raw file):

    Js::RegSlot resultReg = GetRegisterSpace(signature[0])->AcquireTmpRegister();

    EmitInfo args[Simd::MAX_LANES];

Here you should rewrite this to

    EmitInfo args[lanes];
    for (uint i = 0; i < lanes; i++)
    {
        args[i] = PopEvalStack(type);
    }

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1171 at r2 (raw file):

            break;
        case WasmTypes::M128:
            argOp = Js::OpCodeAsmJs::Simd128_I_ArgOut_F4;

shouldn't we use isImportCall ? Js::OpCodesAsmJs::Simd128_ArgOut_F4 : Js::OpCodesAsmJs::Simd128_I_ArgOut_F4 ?

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1311 at r2 (raw file):

}

nit: extra new line

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1448 at r2 (raw file):

}

void WasmBytecodeGenerator::CheckLaneIndex(Js::OpCodeAsmJs op)

We should pass the index by argument here instead of looking at the reader.
Also, call it index instead of offset

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1479 at r2 (raw file):

    default:
        Assert(UNREACHED);
        numLanes = UINT_MAX;

The default should be 0 so that we always throw index out of range

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1503 at r2 (raw file):

    const WasmTypes::WasmType valueType = signature[1];
    EmitInfo valueArg = PopEvalStack(valueType, _u("lane argument type mismatch"));
    

nit: trailing whitespaces

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1540 at r2 (raw file):

        if (indices[i] >= Simd::MAX_LANES * 2)
        {
            throw WasmCompilationException(_u("%-th shuffle lane index is larger than 31"), i);

The format string is invalid. I suppose you meant to use %u-th.
Also, it would be nice to change 31 by %u and use (Simd::MAX_LANES * 2) - 1
This also tells me that we are missing some error cases tests

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1552 at r2 (raw file):

    WasmTypes::WasmType resultType = signature[0];
    WasmTypes::WasmType simdArgType = signature[1];
    

nit: trailing whitespace

---

lib/WasmReader/WasmSignature.cpp, line 146 at r1 (raw file):

    CompileAssert(Local::Void == 0);

#if 0

We need to figure out the story with wasm.simd here at this point
I think we only need to change * 2 for * 3 and << 2 for << 3

---

test/rlexedirs.xml, line 332 at r1 (raw file):

  <default>
    <files>wasm.simd</files>
    <tags>exclude_serialized,exclude_arm,exclude_arm64,require_backend,exclude_xplat</tags>

I don't know how much we use it, but you should add require_wasm in that list

---

test/rlexedirs.xml, line 333 at r1 (raw file):

    <files>wasm.simd</files>
    <tags>exclude_serialized,exclude_arm,exclude_arm64,require_backend,exclude_xplat</tags>
    <tags>exclude_arm,exclude_arm64</tags>

Please remove this second line

---

Comments from Reviewable

[arunetm-zz]

lib/WasmReader/WasmBinaryOpcodesSimd.h, line 8 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Since there is only one type and it might stay like this for a little while.
Can we remove this macro and just use M128 instead where it is used

Yes, makes sense.

---

Comments from Reviewable

[arunetm-zz]

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

Previously, Cellule (Michael Ferris) wrote…

Here I think I would prefer m_funcState.count += Simd::VEC_WIDTH
Technically it should be equivalent, but what we're really doing is read Simd::VEC_WIDTH bytes

Yes, that is better. Done.

---

Comments from Reviewable

[arunetm-zz]

lib/WasmReader/WasmByteCodeGenerator.cpp, line 474 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Here we should do
mTypedRegisterAllocator(&m_alloc, AllocateRegisterSpace, CONFIG_FLAG(WasmSimd) ? 0 : 1 << WAsmJs::SIMD),

yes! good catch. Thanks.

---

Comments from Reviewable

[arunetm-zz]

Review status: 67 of 129 files reviewed at latest revision, 20 unresolved discussions, some commit checks failed.

---

lib/WasmReader/WasmBinaryOpCodes.h, line 11 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

nit: could you move this back to the top

Done.

---

lib/WasmReader/WasmBinaryOpCodes.h, line 88 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

could we move this to the bottom instead and wrap it with #ifdef ENABLE_WASM_SIMD

Done.

---

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

Previously, Cellule (Michael Ferris) wrote…

WasmTypes::GetTypeName does the same thing as this function, we should remove this one.
Also, remove strIds

Done.

---

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

Previously, Cellule (Michael Ferris) wrote…

Looks like this is unused, can we clean it up please ?

Done.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 213 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Could you cleanup those comments please ?

Done.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 623 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

nit: use resultType instead of signature[0]

Done.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 625 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Here you should rewrite this to

    EmitInfo args[lanes];
    for (uint i = 0; i < lanes; i++)
    {
        args[i] = PopEvalStack(type);
    }

Done.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1171 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

shouldn't we use isImportCall ? Js::OpCodesAsmJs::Simd128_ArgOut_F4 : Js::OpCodesAsmJs::Simd128_I_ArgOut_F4 ?

Done.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1311 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

nit: extra new line

Done.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1448 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

We should pass the index by argument here instead of looking at the reader.
Also, call it index instead of offset

Done.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1479 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

The default should be 0 so that we always throw index out of range

Yes, Thanks. Done.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1503 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

nit: trailing whitespaces

Done.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1540 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

The format string is invalid. I suppose you meant to use %u-th.
Also, it would be nice to change 31 by %u and use (Simd::MAX_LANES * 2) - 1
This also tells me that we are missing some error cases tests

Done. Will look into the error cases.

---

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1552 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

nit: trailing whitespace

Done.

---

lib/WasmReader/WasmSignature.cpp, line 146 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

We need to figure out the story with wasm.simd here at this point
I think we only need to change * 2 for * 3 and << 2 for << 3

Is it right to think of m_shortSig as a fast signature comparison scheme? Sorry for my ignorance here, but why do we have this logic disabled?

---

test/rlexedirs.xml, line 332 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

I don't know how much we use it, but you should add require_wasm in that list

Done.

---

test/rlexedirs.xml, line 333 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Please remove this second line

Done.

---

Comments from Reviewable

[Cellule]

Reviewed 42 of 129 files at r1, 19 of 22 files at r2, 9 of 9 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

lib/Backend/IRBuilderAsmJs.cpp, line 4585 at r3 (raw file):

    {
    case Js::OpCodeAsmJs::Simd128_Return_D2:
        CheckJitLoopReturn(dstRegSlot, TySimd128D2);

Here it should remain CheckJitLoopReturn it was changed recently.

---

lib/Backend/IRBuilderAsmJs.cpp, line 4750 at r3 (raw file):

    opcode = GetSimdOpcode(newOpcode);
    /*

I just noticed this was commented out.
Do we still need that code ? If not, can we remove it ?

---

lib/Common/ConfigFlagsList.h, line 900 at r3 (raw file):

FLAGNR(Boolean, WasmSignExtends       , "Use new WebAssembly sign extension operators", DEFAULT_CONFIG_WasmSignExtends)
#ifdef ENABLE_WASM_SIMD
FLAGR(Boolean, WasmSimd, "Enable SIMD in WebAssembly", DEFAULT_CONFIG_WasmSimd)

For now I think this should be a FLAGNR (non release) until we figure out the story to make this available through experimental flag.

---

lib/Runtime/Base/ThreadConfigFlagsList.h, line 60 at r3 (raw file):

#endif

#ifdef ENABLE_WASM_SIMD

You will have to remove this by changing the flag to FLAGNR

---

lib/Runtime/ByteCode/OpCodesAsmJs.h, line 337 at r3 (raw file):

#include "OpCodesSimd.h"

MACRO_EXTEND(AsmJsEntryTracing, Empty, None)

This is probably a merge remnant. I think it was remove a little while back.

---

lib/Runtime/Language/SimdInt64x2Operation.h, line 18 at r3 (raw file):

        //pointer-based arguments are used to ensure that calling conventions are consistent across x86/x64
        //and match call sequences JIT generates.
        //TODO: nikolayk back to "const SIMDValue& a"

We should add to this todo why we can't change it right now, if I recall it's because they're being called directly from the jit right ?

---

lib/WasmReader/WasmSignature.cpp, line 146 at r1 (raw file):

Previously, arunetm (Arun Etm) wrote…

Is it right to think of m_shortSig as a fast signature comparison scheme? Sorry for my ignorance here, but why do we have this logic disabled?

Yes, we want to do a simple integer comparison on signature with few arguments.
The reason it was disabled for wasm.simd originally is simply because it wasn't handled.
We used to use 3 bits for the return type and 2 bits for the arguments' type
Now we need to support the following

• Return types can be one of the following
  • void
  • int32
  • int64
  • float32
  • float64
  • simd
    So 6 types for return type, 3 bits is still good
• Arguments type
  • int32
  • int64
  • float32
  • float64
  • simd
    Now 5 types which is why we had to increase from 2bits to 3bits per arguments

I think you can leave this as it is for now.
I will clean it up once everything is merged

---

Comments from Reviewable

[Cellule]

Reviewed 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

lib/Runtime/Base/ThreadConfigFlagsList.h, line 60 at r3 (raw file):

Previously, Cellule (Michael Ferris) wrote…

You will have to remove this by changing the flag to FLAGNR

I think you removed the wrong one

---

Comments from Reviewable

[arunetm-zz]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

lib/Backend/IRBuilderAsmJs.cpp, line 4585 at r3 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Here it should remain CheckJitLoopReturn it was changed recently.

Code removed.

---

lib/Backend/IRBuilderAsmJs.cpp, line 4750 at r3 (raw file):

Previously, Cellule (Michael Ferris) wrote…

I just noticed this was commented out.
Do we still need that code ? If not, can we remove it ?

Done.

---

lib/Common/ConfigFlagsList.h, line 900 at r3 (raw file):

Previously, Cellule (Michael Ferris) wrote…

For now I think this should be a FLAGNR (non release) until we figure out the story to make this available through experimental flag.

Yes sure. Changing to FLAGNR.

---

lib/Runtime/Base/ThreadConfigFlagsList.h, line 60 at r3 (raw file):

Previously, Cellule (Michael Ferris) wrote…

I think you removed the wrong one

yes. :)

---

lib/Runtime/Language/SimdInt64x2Operation.h, line 18 at r3 (raw file):

Previously, Cellule (Michael Ferris) wrote…

We should add to this todo why we can't change it right now, if I recall it's because they're being called directly from the jit right ?

That's right. We currently have helper calls for these from jit. Will add the info to the ToDo.

---

lib/WasmReader/WasmSignature.cpp, line 146 at r1 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Yes, we want to do a simple integer comparison on signature with few arguments.
The reason it was disabled for wasm.simd originally is simply because it wasn't handled.
We used to use 3 bits for the return type and 2 bits for the arguments' type
Now we need to support the following

• Return types can be one of the following
  • void
  • int32
  • int64
  • float32
  • float64
  • simd
    So 6 types for return type, 3 bits is still good
• Arguments type
  • int32
  • int64
  • float32
  • float64
  • simd
    Now 5 types which is why we had to increase from 2bits to 3bits per arguments

I think you can leave this as it is for now.
I will clean it up once everything is merged

Yes makes sense. Thanks for the explanation.

---

lib/Runtime/ByteCode/OpCodesAsmJs.h, line 337 at r3 (raw file):

Previously, Cellule (Michael Ferris) wrote…

This is probably a merge remnant. I think it was remove a little while back.

Thanks for catching this. seems like a merge mistake. Fixed.

---

Comments from Reviewable

[arunetm-zz]

@Cellule, please take a look.

[arunetm-zz]

Review status: 125 of 127 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

lib/Runtime/Base/ThreadConfigFlagsList.h, line 60 at r3 (raw file):

Previously, arunetm (Arun Etm) wrote…

yes. :)

Done.

---

Comments from Reviewable

[Cellule]

---

Reviewed 3 of 3 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable