WASM.SIMD int64x2 operations #3490

Krovatkin · 2017-08-04T22:50:22Z

This PR includes:

interpreter handlers + lowerer evaluators for most int64x2 ops (e.g. no conversion ops yet since we need float64x2 for that)
tests

This change is

Krovatkin · 2017-08-04T22:50:49Z

@Cellule @MikeHolman please ignore this PR for now.

Krovatkin · 2017-08-08T22:04:16Z

@Cellule could you please take a look at this PR when you have a chance?

Krovatkin · 2017-08-11T06:14:19Z

@Cellule please ignore this PR. Looks like I have a few more things to look at.

Krovatkin · 2017-08-14T17:05:44Z

@Cellule @MikeHolman Hey guys! Could you please take a look? It finally looks somewhat decent. I'm thinking revisiting AllTrue implementation after we are operation-complete

Cellule · 2017-08-16T23:17:02Z

Reviewed 45 of 47 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

lib/Backend/IRBuilderAsmJs.cpp, line 4526 at r2 (raw file):

IRBuilderAsmJs::BuildInt64x2_1Long1(Js::OpCodeAsmJs newOpcode, uint32 offset, BUILD_SIMD_ARGS_REG2)
{

nit: extra newline

lib/Backend/LowerMDSharedSimd128.cpp, line 1164 at r2 (raw file):

    Assert(src1 && src1->IsRegOpnd() && (src1->GetType() == TyFloat32 || src1->GetType() == TyInt32 || src1->GetType() == TyFloat64 ||
        src1->GetType() == TyInt16 || src1->GetType() == TyInt8 || src1->GetType() == TyUint16 ||
        src1->GetType() == TyUint8 || src1->GetType() == TyUint32 || src1->GetType() == TyInt64));

nit: Prefer src1->IsInt64()

lib/Runtime/ByteCode/AsmJsByteCodeDumper.cpp, line 380 at r2 (raw file):

    }

    // Float64x2

nit: Int64x2

lib/Runtime/ByteCode/OpLayoutsAsmJs.h, line 301 at r2 (raw file):

#define LAYOUT_FIELDS_DEF(x, y) LAYOUT_FIELDS_HELPER(x, y)

//LAYOUT_TYPE_WMS_REG2(Int64x2_1Long1, Int64x2, Long)

nit: I believe this is a remnant of testing. Can you clean up please ?

lib/Runtime/Language/Chakra.Runtime.Language.vcxproj, line 98 at r2 (raw file):

    <ClCompile Include="$(MSBuildThisFileDirectory)JavascriptMathOperators.cpp" />
    <ClCompile Include="$(MSBuildThisFileDirectory)ProfilingHelpers.cpp" />
    <ClCompile Include="$(MSBuildThisFileDirectory)SimdInt64x2Operation.cpp" />

Please add the header file below

lib/Runtime/Language/SimdInt64x2Operation.cpp, line 43 at r2 (raw file):

    }

    void SIMDInt64x2Operation::OpShiftLeftByScalar(SIMDValue* dst, SIMDValue* src, int count)

Why did you changed to passing the dst and src by pointer instead of returning the dst and receive the src by reference ?

lib/Runtime/Language/SimdUtils.h, line 234 at r2 (raw file):

        static inline SIMDValue SIMD128InnerReplaceLaneI2(SIMDValue simdVal, const uint32 lane, const int64 value)
        {
            simdVal.i64[lane] = value;

I didn't realize before, but it would be nice to have Assert(lane < 2) here and in the other helpers.

lib/Runtime/Language/ValueType.h, line 193 at r2 (raw file):

    bool IsLikelySimd128Uint8x16() const;
    bool IsLikelySimd128Float64x2() const;
    bool IsLikelySimd128Int64x2() const;

Do we really need Likely types in Wasm ?

test/wasm.simd/int64x2.wast, line 16 at r2 (raw file):

        (i64x2.all_true (m128.load offset=0 align=4 (i32.const 0)))
    )

nit: trailing whitespaces

test/wasm.simd/int64x2.wast, line 18 at r2 (raw file):

    
    (func (export "func_i64x2_extractlane_0") (local $v1 m128)
        (set_local $v1 (m128.load offset=0 align=4 (i32.const 0)))

nit: You could write this without locals using the stack
ie:

(m128.load offset=0 align=4 (i32.const 0))
(i64x2.extract_lane lane=0)
(i64.store offset=0 (i32.const 8))

test/wasm.simd/int64x2Tests.js, line 31 at r2 (raw file):

    moveArgsIntoArray(inputArr, 0, array);
    moveArgsIntoArray(resultArr, 64, array);

Any reason to move the result into the array ? It doesn't look like it is used by the test and is simply reread for asserting.
You could do resultArr[i]|0 or resultArr[i]>>>0 to guaranty int32 (or uint32)

Comments from Reviewable

Krovatkin · 2017-08-17T22:40:22Z

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

lib/Runtime/Language/Chakra.Runtime.Language.vcxproj, line 98 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Please add the header file below

You mean to exclude it from ARM builds?
It doesn't contain any x86-specific intrinsics, though.

lib/Runtime/Language/SimdInt64x2Operation.cpp, line 43 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Why did you changed to passing the dst and src by pointer instead of returning the dst and receive the src by reference ?

The calling convention behaviour was inconsistent: arguments would end up either in a register or on a stack. I figured an added complexity required to handle both cases wasn't worth it considering this path is a fallback.

lib/Runtime/Language/SimdUtils.h, line 234 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

I didn't realize before, but it would be nice to have Assert(lane < 2) here and in the other helpers.

I was thinking the same thing, but I didn't note it down, so I forgot to eventually add it.

lib/Runtime/Language/ValueType.h, line 193 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Do we really need Likely types in Wasm ?

I'd keep it for the sake of completeness for now 😄 I'm a bit fuzzy if we are going to get JavaScript API for handling SIMD types.

test/wasm.simd/int64x2.wast, line 18 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

nit: You could write this without locals using the stack
ie:
(m128.load offset=0 align=4 (i32.const 0))
(i64x2.extract_lane lane=0)
(i64.store offset=0 (i32.const 8))

I'd like to keep the current version unless you feel strongly against it.
Stack versions seem to be harder to follow since mappings between operations
and arguments aren't explicit in the stack versions.
For example, it isn't immediately clear if (i32.const 0) belongs to replace_lane or
store in the snippet below.

    (func (export "func_i64x2_replacelane_0") (local $v1 m128) (local $val i64)
        (i32.const 0)
        (m128.load offset=0 align=4 (i32.const 0))
        (i64.load offset=16 align=4 (i32.const 0))
        (i64x2.replace_lane lane=1)
        (m128.store offset=0)
    )

test/wasm.simd/int64x2Tests.js, line 31 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Any reason to move the result into the array ? It doesn't look like it is used by the test and is simply reread for asserting.
You could do resultArr[i]|0 or resultArr[i]>>>0 to guaranty int32 (or uint32)

No reason. In fact, I started doing just that in my recent tests. I'll change the tests.

Comments from Reviewable

Cellule · 2017-08-18T18:48:51Z

Reviewed 7 of 7 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

lib/Runtime/Language/Chakra.Runtime.Language.vcxproj, line 98 at r2 (raw file):

Previously, Krovatkin (Nick Korovaiko) wrote…

You mean to exclude it from ARM builds?
It doesn't contain any x86-specific intrinsics, though.

I mean to add <ClInclude Include="SimdInt64x2Operation.h" /> in the header ItemGroup below in this file.
This way, the file will be part of the project and show up in the Solution Explorer in Visual Studio

lib/Runtime/Language/SimdInt64x2Operation.cpp, line 43 at r2 (raw file):

required to handle both cases wasn't worth it considering this path is a fallback

I see, so I guess if/when we'll implement these call directly in the jit and not call helper we could/should use the previous calling convention. Is that right ?
If so, can you put a todo where the helpers are defined to remember to do this.

test/wasm.simd/int64x2.wast, line 18 at r2 (raw file):

Previously, Krovatkin (Nick Korovaiko) wrote…

I'd like to keep the current version unless you feel strongly against it.
Stack versions seem to be harder to follow since mappings between operations
and arguments aren't explicit in the stack versions.
For example, it isn't immediately clear if (i32.const 0) belongs to replace_lane or
store in the snippet below.
    (func (export "func_i64x2_replacelane_0") (local $v1 m128) (local $val i64)
        (i32.const 0)
        (m128.load offset=0 align=4 (i32.const 0))
        (i64.load offset=16 align=4 (i32.const 0))
        (i64x2.replace_lane lane=1)
        (m128.store offset=0)
    )

I see your point. I don't feel particularly strongly about it so we can keep like that.

Comments from Reviewable

Cellule · 2017-08-18T21:47:45Z

I don't remember are able to run the tests in the private repository ? Also, I would recommend doing an integration of master into wasm.simd after this PR

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

Comments from Reviewable

Krovatkin · 2017-08-18T21:51:15Z

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

lib/Runtime/Language/Chakra.Runtime.Language.vcxproj, line 98 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

I mean to add <ClInclude Include="SimdInt64x2Operation.h" /> in the header ItemGroup below in this file.
This way, the file will be part of the project and show up in the Solution Explorer in Visual Studio

got you!

lib/Runtime/Language/SimdInt64x2Operation.cpp, line 43 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

required to handle both cases wasn't worth it considering this path is a fallback

I see, so I guess if/when we'll implement these call directly in the jit and not call helper we could/should use the previous calling convention. Is that right ?
If so, can you put a todo where the helpers are defined to remember to do this.

Will do. We already map i64x2.shr_u to pshrlq and i64x2.shl to pshlq. There's no instruction for i64x2.shr_s, unfortunately, so we generate a helper call. I'm planning to revisit i64x2.shr_s and i64x2.alltrue after we are instruction-complete.

test/wasm.simd/int64x2.wast, line 16 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

nit: trailing whitespaces

Hm... interesting. I don't see any trailing whitespaces in this file 😢

test/wasm.simd/int64x2.wast, line 18 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

I see your point. I don't feel particularly strongly about it so we can keep like that.

great! I was debating about changing some functions to a stack form, but I realized that this kind of inconsistency would look even worse

Comments from Reviewable

Krovatkin · 2017-08-18T21:58:17Z

@Cellule

Also, I would recommend doing an integration of master into wasm.simd after this PR

Lettuce do that next!

I don't remember are able to run the tests in the private repository ?

Could you please rephrase this question a bit; I can't seem to parse it 😕 ?

Cellule · 2017-08-18T22:00:17Z

I don't remember are able to run the tests in the private repository ?

Wow that was bad sorry.
The question is: Are you able to run unit tests from the private chakra repository or do you need me to do it for you ?

Krovatkin · 2017-08-18T22:27:20Z

The question is: Are you able to run unit tests from the private chakra repository or do you need me to do it for you ?

Ah! Let me talk to @arunetm , I think we should be able to run the tests in the private repo now.

arunetm-zz · 2017-08-18T22:30:04Z

Yes. We will run the private Tests on the branch.

int64x2 infra int64x2 formatting fixes using scratch area to pass simd vars into helpers sse2 fallbacks cleaned up a test file better seqs removing unused code clean up, add asserts to Extract/Replace, simplify tests add some comments & todos, fix add a header to a project group

Merge pull request #3490 from Krovatkin:wasm.simd.int64x2 This PR includes: * interpreter handlers + lowerer evaluators for most int64x2 ops (e.g. no conversion ops yet since we need float64x2 for that) * tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/microsoft/chakracore/3490)

Krovatkin · 2017-09-21T18:24:26Z

Merged... Closing this PR

msftclas added the cla-already-signed label Aug 4, 2017

Krovatkin force-pushed the wasm.simd.int64x2 branch 4 times, most recently from 79e4d21 to d6a3471 Compare August 7, 2017 16:39

Krovatkin force-pushed the wasm.simd.int64x2 branch from 4386f74 to 25d29a0 Compare August 11, 2017 17:53

dilijev added the External Contributor label Aug 18, 2017

Krovatkin force-pushed the wasm.simd.int64x2 branch from 8c98360 to 8f25757 Compare September 14, 2017 22:53

Krovatkin closed this Sep 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WASM.SIMD int64x2 operations #3490

WASM.SIMD int64x2 operations #3490

Krovatkin commented Aug 4, 2017 •

edited

Loading

Krovatkin commented Aug 4, 2017

Krovatkin commented Aug 8, 2017

Krovatkin commented Aug 11, 2017

Krovatkin commented Aug 14, 2017

Cellule commented Aug 16, 2017

Krovatkin commented Aug 17, 2017

Cellule commented Aug 18, 2017

Cellule commented Aug 18, 2017

Krovatkin commented Aug 18, 2017

Krovatkin commented Aug 18, 2017

Cellule commented Aug 18, 2017

Krovatkin commented Aug 18, 2017

arunetm-zz commented Aug 18, 2017

Krovatkin commented Sep 21, 2017

WASM.SIMD int64x2 operations #3490

WASM.SIMD int64x2 operations #3490

Conversation

Krovatkin commented Aug 4, 2017 • edited Loading

Krovatkin commented Aug 4, 2017

Krovatkin commented Aug 8, 2017

Krovatkin commented Aug 11, 2017

Krovatkin commented Aug 14, 2017

Cellule commented Aug 16, 2017

Krovatkin commented Aug 17, 2017

Cellule commented Aug 18, 2017

Cellule commented Aug 18, 2017

Krovatkin commented Aug 18, 2017

Krovatkin commented Aug 18, 2017

Cellule commented Aug 18, 2017

Krovatkin commented Aug 18, 2017

arunetm-zz commented Aug 18, 2017

Krovatkin commented Sep 21, 2017

Krovatkin commented Aug 4, 2017 •

edited

Loading