Issue 2264533002: [wasm] Add native x64 implementations for I32x4Splat, I32x4ExtractLane

gdeepti

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-19 09:12:13 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/1

4 years, 4 months ago (2016-08-19 09:12:19 UTC) #2

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-19 09:13:29 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/builds/7222) v8_linux64_avx2_rel_ng on ...

4 years, 4 months ago (2016-08-19 09:13:30 UTC) #4

gdeepti

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-19 09:16:31 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/20001

4 years, 4 months ago (2016-08-19 09:16:39 UTC) #6

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-19 09:19:49 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/builds/21483) v8_mac_rel_ng on ...

4 years, 4 months ago (2016-08-19 09:19:50 UTC) #8

gdeepti

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-19 10:03:20 UTC) #9

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/60001

4 years, 4 months ago (2016-08-19 10:03:29 UTC) #10

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-19 10:07:57 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_rel/builds/19851)

4 years, 4 months ago (2016-08-19 10:08:01 UTC) #12

gdeepti

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-19 10:24:18 UTC) #13

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/80001

4 years, 4 months ago (2016-08-19 10:24:29 UTC) #14

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-19 10:40:39 UTC) #15

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/7036) v8_linux_arm_rel_ng_triggered on ...

4 years, 4 months ago (2016-08-19 10:40:40 UTC) #16

gdeepti

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-19 17:24:27 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/100001

4 years, 4 months ago (2016-08-19 17:24:38 UTC) #18

gdeepti

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-19 17:28:22 UTC) #19

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/120001

4 years, 4 months ago (2016-08-19 17:28:28 UTC) #20

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-19 17:47:42 UTC) #21

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/builds/7051) v8_linux_arm64_rel_ng_triggered on ...

4 years, 4 months ago (2016-08-19 17:47:46 UTC) #22

gdeepti

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-19 19:14:25 UTC) #23

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/140001

4 years, 4 months ago (2016-08-19 19:14:36 UTC) #24

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-19 19:45:12 UTC) #26

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-19 19:45:13 UTC) #27

bbudge

https://codereview.chromium.org/2264533002/diff/140001/src/compiler/x64/code-generator-x64.cc File src/compiler/x64/code-generator-x64.cc (right): https://codereview.chromium.org/2264533002/diff/140001/src/compiler/x64/code-generator-x64.cc#newcode1749 src/compiler/x64/code-generator-x64.cc:1749: case kSSECreateInt32x4: { Would this be the AVX implementation ...

4 years, 4 months ago (2016-08-22 19:09:53 UTC) #28

gdeepti

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-22 22:22:37 UTC) #29

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/160001

4 years, 4 months ago (2016-08-22 22:22:44 UTC) #30

gdeepti

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-22 22:29:54 UTC) #31

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/180001

4 years, 4 months ago (2016-08-22 22:30:03 UTC) #32

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-22 23:05:52 UTC) #33

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-22 23:05:53 UTC) #34

gdeepti

https://codereview.chromium.org/2264533002/diff/140001/src/compiler/x64/code-generator-x64.cc File src/compiler/x64/code-generator-x64.cc (right): https://codereview.chromium.org/2264533002/diff/140001/src/compiler/x64/code-generator-x64.cc#newcode1749 src/compiler/x64/code-generator-x64.cc:1749: case kSSECreateInt32x4: { On 2016/08/22 19:09:52, bbudge wrote: > ...

4 years, 4 months ago (2016-08-22 23:26:12 UTC) #35

https://codereview.chromium.org/2264533002/diff/140001/src/compiler/x64/code-...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2264533002/diff/140001/src/compiler/x64/code-...
src/compiler/x64/code-generator-x64.cc:1749: case kSSECreateInt32x4: {
On 2016/08/22 19:09:52, bbudge wrote:
> Would this be the AVX implementation too? In that case, it would be best if we
> didn't have kSSE* values, i.e. this would be kX64CreateInt32x4.

I was conflicted about calling these kX64 ops because vshufps/vpextrd/vextractps
are not implemented in the x64 assembler, so these are essentially SSE ops.

https://codereview.chromium.org/2264533002/diff/140001/src/compiler/x64/code-...
src/compiler/x64/code-generator-x64.cc:1758: __ pextrd(i.OutputRegister(),
i.InputSimd128Register(0), i.InputInt8(1));
On 2016/08/22 19:09:52, bbudge wrote:
> Rather than defining kSSE and kAVX values, I think the desired pattern going
> forward is to use the macro-assembler to handle this. So call MacroAssembler
> Pextrd here:
> 
>
https://cs.chromium.org/chromium/src/v8/src/x64/macro-assembler-x64.cc?rcl=0&...
> 
> Then rename the value kX64Int32x4ExtractLane.

Changed to kX64Int32x4ExtractLane, kX64CreateInt32x4, calling Pextrd, Movd in
the macro-assembler to fit the desired pattern. Shufps does not have a macro
assembler equivalent that switches between vshufps and shufps.

https://codereview.chromium.org/2264533002/diff/140001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm-simd.cc (right):

https://codereview.chromium.org/2264533002/diff/140001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:31: WASM_BLOCK(WASM_SET_LOCAL(1,
WASM_SIMD_I32x4_SPLAT(WASM_GET_LOCAL(0))),
On 2016/08/22 19:09:52, bbudge wrote:
> Using splat, it's not really possible to write a good test of ExtractLane,
since
> all lane values are equal. So I would look at this as a temporary test, until
we
> have a way to generate more interesting SIMD values. Given that, you should
> probably just test a single lane, and leave a comment so no one thinks these
are
> the tests for extract.

Agree that it is not possible to have a good test for ExtractLane with just
Splat, changed test name to be Splat, added a comment as well. I was mostly
experimenting with testing lane values in Wasm code and put the test in for
feedback and as a comprehensive test for splat with a range of int values. 

As we won't have meaningful SIMD values till shuffle/swizzle are added, would it
be a bad idea to expose Simd.Create as a (debug/regular) opcode?

titzer

lgtm with comments https://codereview.chromium.org/2264533002/diff/180001/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2264533002/diff/180001/src/compiler/wasm-compiler.cc#newcode2870 src/compiler/wasm-compiler.cc:2870: return graph()->NewNode(jsgraph()->machine()->CreateInt32x4(), Can you add a ...

4 years, 4 months ago (2016-08-23 13:06:40 UTC) #36

bbudge

LGTM w/change to code generator. https://codereview.chromium.org/2264533002/diff/140001/test/cctest/wasm/test-run-wasm-simd.cc File test/cctest/wasm/test-run-wasm-simd.cc (right): https://codereview.chromium.org/2264533002/diff/140001/test/cctest/wasm/test-run-wasm-simd.cc#newcode31 test/cctest/wasm/test-run-wasm-simd.cc:31: WASM_BLOCK(WASM_SET_LOCAL(1, WASM_SIMD_I32x4_SPLAT(WASM_GET_LOCAL(0))), On 2016/08/22 ...

4 years, 4 months ago (2016-08-23 13:57:13 UTC) #37

gdeepti

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-23 19:24:10 UTC) #38

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/200001

4 years, 4 months ago (2016-08-23 19:24:13 UTC) #39

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-23 19:52:22 UTC) #40

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-23 19:52:23 UTC) #41

gdeepti

https://codereview.chromium.org/2264533002/diff/140001/test/cctest/wasm/test-run-wasm-simd.cc File test/cctest/wasm/test-run-wasm-simd.cc (right): https://codereview.chromium.org/2264533002/diff/140001/test/cctest/wasm/test-run-wasm-simd.cc#newcode31 test/cctest/wasm/test-run-wasm-simd.cc:31: WASM_BLOCK(WASM_SET_LOCAL(1, WASM_SIMD_I32x4_SPLAT(WASM_GET_LOCAL(0))), On 2016/08/23 13:57:13, bbudge wrote: > On ...

4 years, 4 months ago (2016-08-23 19:53:34 UTC) #42

https://codereview.chromium.org/2264533002/diff/140001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm-simd.cc (right):

https://codereview.chromium.org/2264533002/diff/140001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:31: WASM_BLOCK(WASM_SET_LOCAL(1,
WASM_SIMD_I32x4_SPLAT(WASM_GET_LOCAL(0))),
On 2016/08/23 13:57:13, bbudge wrote:
> On 2016/08/22 23:26:11, gdeepti wrote:
> > On 2016/08/22 19:09:52, bbudge wrote:
> > > Using splat, it's not really possible to write a good test of ExtractLane,
> > since
> > > all lane values are equal. So I would look at this as a temporary test,
> until
> > we
> > > have a way to generate more interesting SIMD values. Given that, you
should
> > > probably just test a single lane, and leave a comment so no one thinks
these
> > are
> > > the tests for extract.
> > 
> > Agree that it is not possible to have a good test for ExtractLane with just
> > Splat, changed test name to be Splat, added a comment as well. I was mostly
> > experimenting with testing lane values in Wasm code and put the test in for
> > feedback and as a comprehensive test for splat with a range of int values. 
> > 
> > As we won't have meaningful SIMD values till shuffle/swizzle are added,
would
> it
> > be a bad idea to expose Simd.Create as a (debug/regular) opcode? 
> 
> If it's not too hard, implementing ReplaceLane would allow you to test
> everything. Of course, Create does too.

Did not think of replace lane, I'll add replace lane + extract lane test with
replace lane in a follow up CL.

https://codereview.chromium.org/2264533002/diff/180001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2264533002/diff/180001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2870: return
graph()->NewNode(jsgraph()->machine()->CreateInt32x4(),
On 2016/08/23 13:06:40, titzer wrote:
> Can you add a TODO that we should just introduce a SimdConstant to
> src/compiler/common-operator.h?

I have a partical CL that introduces a Simd128Constant, but the decision was to
defer till we see a strong need for it. Looked at some SIMD code with
bradnelson@, bbudge@ - conclusion was that SIMD constants are rarely used in
practice and we would only be introducing them for ease of
representation/testing. Added a TODO for now. I can clean up the experimental
patch and send it out for review if we decide that having a Simd128Constant is
necessary.

https://codereview.chromium.org/2264533002/diff/180001/src/compiler/x64/code-...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2264533002/diff/180001/src/compiler/x64/code-...
src/compiler/x64/code-generator-x64.cc:2044: __ pextrd(i.OutputRegister(),
i.InputSimd128Register(0), i.InputInt8(1));
On 2016/08/23 13:57:13, bbudge wrote:
> Call macro-assembler here: Pextrd

Sorry, missed this. Calling macro-assembler for Pextrd, Movd.

https://codereview.chromium.org/2264533002/diff/180001/src/compiler/x64/instr...
File src/compiler/x64/instruction-codes-x64.h (right):

https://codereview.chromium.org/2264533002/diff/180001/src/compiler/x64/instr...
src/compiler/x64/instruction-codes-x64.h:149: V(X64CreateInt32x4)              \
On 2016/08/23 13:06:40, titzer wrote:
> We should probably just follow the existing naming conventions for this.

Done.

gdeepti

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org, bbudge@chromium.org Link to ...

4 years, 4 months ago (2016-08-23 19:54:34 UTC) #44

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2264533002/200001

4 years, 4 months ago (2016-08-23 19:54:43 UTC) #45

commit-bot: I haz the power

Description was changed from ========== [wasm] Add native x64 implementations for I32x4Splat, I32x4ExtractLane - Instruction ...

4 years, 4 months ago (2016-08-23 19:59:52 UTC) #47

commit-bot: I haz the power

4 years, 4 months ago (2016-08-23 19:59:53 UTC) #48

Message was sent while issue was closed.

Patchset 11 (id:??) landed as
https://crrev.com/6122465c88a7dbaf2f5a46d61e12a6d1a5ce08ce
Cr-Commit-Position: refs/heads/master@{#38843}

Issue 2264533002: [wasm] Add native x64 implementations for I32x4Splat, I32x4ExtractLane (Closed)

Description

Patch Set 1 #

Patch Set 2 #

Patch Set 3 : Formatting #

Patch Set 4 : Fix build #

Patch Set 5 : Fix mips build #

Patch Set 6 : Fix cctest.status #

Patch Set 7 : Formatting #

Patch Set 8 : cctest #

Patch Set 9 : Review changes #

Patch Set 10 : Fix test comment #

Patch Set 11 : Review changes #

Messages