[wasm]implement simd lowering for F32x4 and I32x4 binops

[wasm]implement simd lowering for F32x4 and I32x4 binops

BUG=v8:4124
R=bradnelson@chromium.org,bbudge@chromium.org,gdeepti@chromium.org

Review-Url: https://codereview.chromium.org/2713613005
Cr-Commit-Position: refs/heads/master@{#43465}
Committed: https://chromium.googlesource.com/v8/v8/+/7f5701507d96c1eddd2ba6e3a86ef1f382a5b4f8

[aseemgarg, 2017-02-24 05:50:10]

Description was changed from

==========
[wasm]implement simd lowering for F32x4 and I32x4 binops

BUG=v8:4124
R=bradnelson@chromium.org,bbudge@chromium.org,gdeepti@chromium.org
==========

to

==========
[wasm]implement simd lowering for F32x4 and I32x4 binops

BUG=v8:4124
R=bradnelson@chromium.org,bbudge@chromium.org,gdeepti@chromium.org
==========

[aseemgarg, 2017-02-24 05:50:10]

aseemgarg@chromium.org changed reviewers:
+ titzer@chromium.org

[aseemgarg, 2017-02-24 05:51:33]

I32x4 Min and max functions are yet to be implemented.

F32x4 min and max are not working. Any ideas?
I printed the case where the test failed and it printed the values as -0 and 0

[aseemgarg, 2017-02-24 05:51:46]

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

[commit-bot: I haz the power, 2017-02-24 05:51:54]

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

[commit-bot: I haz the power, 2017-02-24 05:55:03]

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

[commit-bot: I haz the power, 2017-02-24 05:55:04]

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

[bbudge, 2017-02-24 06:03:00]

On 2017/02/24 05:51:33, aseemgarg wrote:
> I32x4 Min and max functions are yet to be implemented.
> 
> F32x4 min and max are not working. Any ideas?
> I printed the case where the test failed and it printed the values as -0 and 0

The float lane tests use an integer compare (after bit casting) so 0 and -0 will
compare

[bbudge, 2017-02-24 06:03:16]

On 2017/02/24 06:03:00, bbudge wrote:
> On 2017/02/24 05:51:33, aseemgarg wrote:
> > I32x4 Min and max functions are yet to be implemented.
> > 
> > F32x4 min and max are not working. Any ideas?
> > I printed the case where the test failed and it printed the values as -0 and
0
> 
> The float lane tests use an integer compare (after bit casting) so 0 and -0
will
> compare

not equal

[aseemgarg, 2017-02-24 08:45:58]

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

[commit-bot: I haz the power, 2017-02-24 08:46:04]

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

[aseemgarg, 2017-02-24 08:49:57]

On 2017/02/24 06:03:16, bbudge wrote:
> On 2017/02/24 06:03:00, bbudge wrote:
> > On 2017/02/24 05:51:33, aseemgarg wrote:
> > > I32x4 Min and max functions are yet to be implemented.
> > > 
> > > F32x4 min and max are not working. Any ideas?
> > > I printed the case where the test failed and it printed the values as -0
and
> 0
> > 
> > The float lane tests use an integer compare (after bit casting) so 0 and -0
> will
> > compare
> 
> not equal

Thanks. Fixed by not running compare for zero inputs.

[aseemgarg, 2017-02-24 08:50:07]

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

[commit-bot: I haz the power, 2017-02-24 08:50:14]

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

[commit-bot: I haz the power, 2017-02-24 09:14:59]

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

[commit-bot: I haz the power, 2017-02-24 09:15:00]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-02-24 10:54:41]

aseemgarg@chromium.org changed reviewers:
+ mtrofin@chromium.org

[gdeepti, 2017-02-24 13:31:23]

https://codereview.chromium.org/2713613005/diff/40001/test/cctest/wasm/test-w...
File test/cctest/wasm/test-wasm-simd-common.h (right):

https://codereview.chromium.org/2713613005/diff/40001/test/cctest/wasm/test-w...
test/cctest/wasm/test-wasm-simd-common.h:301: class TestWasmSimdCommon {
Maybe use a namespace here instead of a class with only static methods as per
the C++ styleguide?

[aseemgarg, 2017-02-24 14:19:42]

https://codereview.chromium.org/2713613005/diff/40001/test/cctest/wasm/test-w...
File test/cctest/wasm/test-wasm-simd-common.h (right):

https://codereview.chromium.org/2713613005/diff/40001/test/cctest/wasm/test-w...
test/cctest/wasm/test-wasm-simd-common.h:301: class TestWasmSimdCommon {
On 2017/02/24 13:31:23, gdeepti wrote:
> Maybe use a namespace here instead of a class with only static methods as per
> the C++ styleguide?

Done.

[aseemgarg, 2017-02-24 14:28:23]

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

[commit-bot: I haz the power, 2017-02-24 14:28:34]

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

[commit-bot: I haz the power, 2017-02-24 14:50:35]

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

[commit-bot: I haz the power, 2017-02-24 14:50:36]

Dry run: This issue passed the CQ dry run.

[bbudge, 2017-02-24 20:53:36]

I like your idea of merging the tests. Why not go further, and put all tests
into test-run-wasm-simd with appropriate #ifdef's so only the supported tests
run?

https://codereview.chromium.org/2713613005/diff/60001/src/compiler/simd-scala...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2713613005/diff/60001/src/compiler/simd-scala...
src/compiler/simd-scalar-lowering.cc:83: V(Int32x4ReplaceLane)
nit: Since we have many ops, I've been trying to order them, roughly:
Create
Extract/Replace lane
Unary ops
Binary ops
Relational ops
etc.

And types ordered:
Float32x4
Int32x4
Int16x8
Int8x16
Simd128
Simd32x4
Simd1x4 etc.

https://codereview.chromium.org/2713613005/diff/60001/src/compiler/simd-scala...
src/compiler/simd-scalar-lowering.cc:390: #define I32X4_BINOP_CASE(s_op, m_op)  
               \
Macro param names: Name, instruction or op.

https://codereview.chromium.org/2713613005/diff/60001/test/cctest/wasm/test-w...
File test/cctest/wasm/test-wasm-simd-common.h (right):

https://codereview.chromium.org/2713613005/diff/60001/test/cctest/wasm/test-w...
test/cctest/wasm/test-wasm-simd-common.h:286: 
Could you also move the SIMD macros from wasm-macro-gen.h here as part of this
CL?

[aseemgarg, 2017-02-25 03:25:16]

Got rid of test-run-wasm-simd-lowering.cc and merged into test-run-wasm-simd. We
don't need common anymore either

https://codereview.chromium.org/2713613005/diff/60001/src/compiler/simd-scala...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2713613005/diff/60001/src/compiler/simd-scala...
src/compiler/simd-scalar-lowering.cc:83: V(Int32x4ReplaceLane)
On 2017/02/24 20:53:35, bbudge wrote:
> nit: Since we have many ops, I've been trying to order them, roughly:
> Create
> Extract/Replace lane
> Unary ops
> Binary ops
> Relational ops
> etc.
> 
> And types ordered:
> Float32x4
> Int32x4
> Int16x8
> Int8x16
> Simd128
> Simd32x4
> Simd1x4 etc.

Done.

https://codereview.chromium.org/2713613005/diff/60001/src/compiler/simd-scala...
src/compiler/simd-scalar-lowering.cc:390: #define I32X4_BINOP_CASE(s_op, m_op)  
               \
On 2017/02/24 20:53:36, bbudge wrote:
> Macro param names: Name, instruction or op.

changed to opcode, instruction

https://codereview.chromium.org/2713613005/diff/60001/test/cctest/wasm/test-w...
File test/cctest/wasm/test-wasm-simd-common.h (right):

https://codereview.chromium.org/2713613005/diff/60001/test/cctest/wasm/test-w...
test/cctest/wasm/test-wasm-simd-common.h:286: 
On 2017/02/24 20:53:36, bbudge wrote:
> Could you also move the SIMD macros from wasm-macro-gen.h here as part of this
> CL?

Done.

[aseemgarg, 2017-02-25 03:26:55]

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

[commit-bot: I haz the power, 2017-02-25 03:27:00]

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

[commit-bot: I haz the power, 2017-02-25 03:31:49]

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

[commit-bot: I haz the power, 2017-02-25 03:31:50]

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

[aseemgarg, 2017-02-25 03:35:34]

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

[commit-bot: I haz the power, 2017-02-25 03:35:36]

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

[commit-bot: I haz the power, 2017-02-25 04:42:57]

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

[commit-bot: I haz the power, 2017-02-25 04:42:58]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-02-26 09:55:16]

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

[commit-bot: I haz the power, 2017-02-26 09:55:17]

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

[commit-bot: I haz the power, 2017-02-26 10:16:27]

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

[commit-bot: I haz the power, 2017-02-26 10:16:28]

Dry run: This issue passed the CQ dry run.

[bbudge, 2017-02-27 18:26:15]

Nice, LGTM with 1 comment.

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

https://codereview.chromium.org/2713613005/diff/120001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:231: x, kSimdPrefix,
static_cast<byte>(kExprF32x4Splat)
These can be defined as WASM_SIMD_UNOP( op, x) if you move them down with the
other splat/extract/replace macros.

[aseemgarg, 2017-02-27 22:09:47]

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

https://codereview.chromium.org/2713613005/diff/120001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:231: x, kSimdPrefix,
static_cast<byte>(kExprF32x4Splat)
On 2017/02/27 18:26:15, bbudge wrote:
> These can be defined as WASM_SIMD_UNOP( op, x) if you move them down with the
> other splat/extract/replace macros.

Got rid of add macros and moved down others.

[aseemgarg, 2017-02-27 22:10:03]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2017-02-27 22:10:03]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/2713613005/#ps140001
(title: "[wasm]implement simd lowering for F32x4 and I32x4 binops")

[commit-bot: I haz the power, 2017-02-27 22:10:08]

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

[commit-bot: I haz the power, 2017-02-27 22:13:18]

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

[commit-bot: I haz the power, 2017-02-27 22:13:19]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/35627)

[aseemgarg, 2017-02-27 22:21:00]

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

[commit-bot: I haz the power, 2017-02-27 22:21:07]

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

[Mircea Trofin, 2017-02-27 22:30:57]

On 2017/02/27 22:13:19, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_presubmit on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/35627)

Rubber stamp lgtm

[commit-bot: I haz the power, 2017-02-27 23:12:51]

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

[commit-bot: I haz the power, 2017-02-27 23:12:52]

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

[aseemgarg, 2017-02-27 23:15:21]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2017-02-27 23:15:21]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
mtrofin@chromium.org
Link to the patchset: https://codereview.chromium.org/2713613005/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-27 23:15:32]

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

[commit-bot: I haz the power, 2017-02-27 23:45:04]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1488237321014910,
"parent_rev": "ef2a9e2debc8b8888348a4231d155e7ddcc86bda", "commit_rev":
"7f5701507d96c1eddd2ba6e3a86ef1f382a5b4f8"}

[commit-bot: I haz the power, 2017-02-27 23:45:11]

Description was changed from

==========
[wasm]implement simd lowering for F32x4 and I32x4 binops

BUG=v8:4124
R=bradnelson@chromium.org,bbudge@chromium.org,gdeepti@chromium.org
==========

to

==========
[wasm]implement simd lowering for F32x4 and I32x4 binops

BUG=v8:4124
R=bradnelson@chromium.org,bbudge@chromium.org,gdeepti@chromium.org

Review-Url: https://codereview.chromium.org/2713613005
Cr-Commit-Position: refs/heads/master@{#43465}
Committed:
https://chromium.googlesource.com/v8/v8/+/7f5701507d96c1eddd2ba6e3a86ef1f382a...
==========

[commit-bot: I haz the power, 2017-02-27 23:45:12]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/v8/v8/+/7f5701507d96c1eddd2ba6e3a86ef1f382a...

[Michael Achenbach, 2017-02-28 16:20:08]

Note: This fails on arm chromebooks:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm/builds/2300

Please have a look.

[bbudge, 2017-02-28 16:38:32]

On 2017/02/28 16:20:08, Michael Achenbach wrote:
> Note: This fails on arm chromebooks:
> https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm/builds/2300
> 
> Please have a look.

These are probably failing because ARM Neon arithmetic is slightly non-standard.
For now, disable these tests in:

test/cctest/cctest.status

I'm working on a fix for our FP tests:

https://codereview.chromium.org/2594043002/

[aseemgarg, 2017-02-28 21:28:12]

On 2017/02/28 16:20:08, Michael Achenbach wrote:
> Note: This fails on arm chromebooks:
> https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm/builds/2300
> 
> Please have a look.

This patch should fix it (disabled the failing tests for arm)
https://codereview.chromium.org/2723823002/