[wasm] fix Simd ExtractLane to take immediate instead of param

[wasm] fix Simd ExtractLane to take immediate instead of param

BUG=v8:4124
TEST:test-run-wasm-simd
R=titzer@chromium.org,bradnelson@chromium.org,gdeepti@chromium.org

Committed: https://crrev.com/7b3875d107d6813b458468d18850b452e03aeeea
Cr-Commit-Position: refs/heads/master@{#39288}

[aseemgarg, 2016-09-01 00:24:45]

Description was changed from

==========
[wasm] fix Simd ExtractLane to take immediate instead of param

BUG=v8:4124
TEST:test-run-wasm-simd
R=titzer@chromium.org,bradnelson@chromium.org,gdeepti@chromium.org
==========

to

==========
[wasm] fix Simd ExtractLane to take immediate instead of param

BUG=v8:4124
TEST:test-run-wasm-simd
R=titzer@chromium.org,bradnelson@chromium.org,gdeepti@chromium.org
==========

[aseemgarg, 2016-09-01 00:24:45]

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

[aseemgarg, 2016-09-01 00:32:17]

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

[commit-bot: I haz the power, 2016-09-01 00:32:24]

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

[bradnelson, 2016-09-01 00:51:35]

lgtm

https://codereview.chromium.org/2300753005/diff/1/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2300753005/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:465: FOREACH_SIMD_MISC_OPCODE(DECLARE_OPCODE_CASE) {
return 3; }
Not keen on the MISC name, _0OPERAND + _1OPERAND ?
The shuffles and swizzles will be in this category too.

[bradnelson, 2016-09-01 00:52:01]

You'll need src/compiler owner to sign off too.

[gdeepti, 2016-09-01 00:52:52]

https://codereview.chromium.org/2300753005/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2300753005/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2911: 
This can be merged into the function above by taking an optional lane argument,
IMO it's nicer to have one source of truth that can handle the different ops.

https://codereview.chromium.org/2300753005/diff/1/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2300753005/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:392: FOREACH_SIMD_MISC_OPCODE(DECLARE_OPCODE_CASE)
Nit: Maybe use a different name here?  FOREACH_SIMD_IMPLICIT_LANE_OPCODE maybe?
Actually that doesn't sound like the right name either, perhaps others can chime
in with a better name - it would be nice for the name to be more descriptive of
why these need to be defined separately instead of a Misc category.

https://codereview.chromium.org/2300753005/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:464: FOREACH_SIMD_OPCODE(DECLARE_OPCODE_CASE) { return
2; }
This doesn't seem right, for generic SIMD  opcodes shouldn't the length be
computed based on signature?  For example, this would fail for the Select
opcode. 

Also, define named constants for return values.

[aseemgarg, 2016-09-01 00:57:15]

https://codereview.chromium.org/2300753005/diff/1/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2300753005/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:465: FOREACH_SIMD_MISC_OPCODE(DECLARE_OPCODE_CASE) {
return 3; }
On 2016/09/01 00:51:34, bradnelson wrote:
> Not keen on the MISC name, _0OPERAND + _1OPERAND ?
> The shuffles and swizzles will be in this category too.

Changed name. Lot of opcodes will come in this category including replace lanes
etc. We can move them as we implement them.

[titzer, 2016-09-01 08:32:43]

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1411: DCHECK(lane >= 0 && lane <= 3);
Don't use DCHECK's, they will cause a crash. Instead signal an error.

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1418: default: {
You will need to signal an error for unknown opcodes here. Maybe you can tell if
it's an unknown opcode if the sig is null?

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/wasm-macro-gen.h
File src/wasm/wasm-macro-gen.h (right):

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/wasm-macro-gen...
src/wasm/wasm-macro-gen.h:592: #define WASM_SIMD_I32x4_EXTRACT_LANE(x, y) \
If y is the immediate, then generally it should come first in the macro. And
maybe "lane" is a better name.

[bbudge, 2016-09-01 22:06:49]

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/wasm-opcodes.h...
src/wasm/wasm-opcodes.h:276: #define FOREACH_SIMD_0_OPERAND_OPCODE(V) \
These names are confusing to me. These all return a SIMD result, while the next
macro is for ops that return scalars. Maybe the following?

FOREACH_SIMD_SIMD_RESULT_OPCODE here and
FOREACH_SIMD_SCALAR_RESULT_OPCODE below.

[aseemgarg, 2016-09-02 22:40:13]

https://codereview.chromium.org/2300753005/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2300753005/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2911: 
On 2016/09/01 00:52:52, gdeepti wrote:
> This can be merged into the function above by taking an optional lane
argument,
> IMO it's nicer to have one source of truth that can handle the different ops. 

I'm not keen on having a single function as there are a huge number of opcodes.
I was thinking grouping into few functions that would keep the function sizes
small and also allow for sharing code between cases. Or at least 2 functions for
the types with an immediate and without.

https://codereview.chromium.org/2300753005/diff/1/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2300753005/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:392: FOREACH_SIMD_MISC_OPCODE(DECLARE_OPCODE_CASE)
On 2016/09/01 00:52:52, gdeepti wrote:
> Nit: Maybe use a different name here?  FOREACH_SIMD_IMPLICIT_LANE_OPCODE
maybe?
> Actually that doesn't sound like the right name either, perhaps others can
chime
> in with a better name - it would be nice for the name to be more descriptive
of
> why these need to be defined separately instead of a Misc category. 

Done.

https://codereview.chromium.org/2300753005/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:464: FOREACH_SIMD_OPCODE(DECLARE_OPCODE_CASE) { return
2; }
On 2016/09/01 00:52:52, gdeepti wrote:
> This doesn't seem right, for generic SIMD  opcodes shouldn't the length be
> computed based on signature?  For example, this would fail for the Select
> opcode. 
> 
> Also, define named constants for return values. 

This just checks the size of opcode. That is 2 (simd_prefix + opcode) for 0
operand opcodes and 3 (extra immediate) for 1 operand opcodes.
Also, seems like this function doesn't really use named constants..

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1411: DCHECK(lane >= 0 && lane <= 3);
On 2016/09/01 08:32:43, titzer wrote:
> Don't use DCHECK's, they will cause a crash. Instead signal an error.

Done.

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1418: default: {
On 2016/09/01 08:32:43, titzer wrote:
> You will need to signal an error for unknown opcodes here. Maybe you can tell
if
> it's an unknown opcode if the sig is null?

Done.

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/wasm-macro-gen.h
File src/wasm/wasm-macro-gen.h (right):

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/wasm-macro-gen...
src/wasm/wasm-macro-gen.h:592: #define WASM_SIMD_I32x4_EXTRACT_LANE(x, y) \
On 2016/09/01 08:32:43, titzer wrote:
> If y is the immediate, then generally it should come first in the macro. And
> maybe "lane" is a better name.

Done.

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/2300753005/diff/20001/src/wasm/wasm-opcodes.h...
src/wasm/wasm-opcodes.h:276: #define FOREACH_SIMD_0_OPERAND_OPCODE(V) \
On 2016/09/01 22:06:49, bbudge wrote:
> These names are confusing to me. These all return a SIMD result, while the
next
> macro is for ops that return scalars. Maybe the following?
> 
> FOREACH_SIMD_SIMD_RESULT_OPCODE here and
> FOREACH_SIMD_SCALAR_RESULT_OPCODE below.

Those names are not quite correct as the next set of opcodes might not return or
return simd. For instance, ReplaceLane should move to the next set but will
return a simd. These macro names just tell us if there's an immediate.

[titzer, 2016-09-05 12:49:50]

https://codereview.chromium.org/2300753005/diff/40001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2300753005/diff/40001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1423: error(pc_, pc_, "invalid simd opcode");
You'll have to break here, or you'll crash on the next line. Better if you just
put the work in an if (sig != nullptr)

[aseemgarg, 2016-09-06 02:49:15]

https://codereview.chromium.org/2300753005/diff/40001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2300753005/diff/40001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1423: error(pc_, pc_, "invalid simd opcode");
On 2016/09/05 12:49:50, titzer wrote:
> You'll have to break here, or you'll crash on the next line. Better if you
just
> put the work in an if (sig != nullptr)

Done.

[titzer, 2016-09-08 14:46:19]

lgtm

[aseemgarg, 2016-09-08 16:02:19]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2016-09-08 16:02:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2300753005/#ps60001
(title: "[wasm] fix Simd ExtractLane to take immediate instead of param")

[commit-bot: I haz the power, 2016-09-08 16:02:27]

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, 2016-09-08 16:03:59]

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

[commit-bot: I haz the power, 2016-09-08 16:04:01]

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/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12396)
  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/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23631)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/13912)
  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, 2016-09-08 16:36:08]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2016-09-08 16:36:08]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org,
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2300753005/#ps80001
(title: "[wasm] fix Simd ExtractLane to take immediate instead of param")

[commit-bot: I haz the power, 2016-09-08 16:36:15]

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, 2016-09-08 17:15:24]

Description was changed from

==========
[wasm] fix Simd ExtractLane to take immediate instead of param

BUG=v8:4124
TEST:test-run-wasm-simd
R=titzer@chromium.org,bradnelson@chromium.org,gdeepti@chromium.org
==========

to

==========
[wasm] fix Simd ExtractLane to take immediate instead of param

BUG=v8:4124
TEST:test-run-wasm-simd
R=titzer@chromium.org,bradnelson@chromium.org,gdeepti@chromium.org
==========

[commit-bot: I haz the power, 2016-09-08 17:15:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-08 17:16:11]

Description was changed from

==========
[wasm] fix Simd ExtractLane to take immediate instead of param

BUG=v8:4124
TEST:test-run-wasm-simd
R=titzer@chromium.org,bradnelson@chromium.org,gdeepti@chromium.org
==========

to

==========
[wasm] fix Simd ExtractLane to take immediate instead of param

BUG=v8:4124
TEST:test-run-wasm-simd
R=titzer@chromium.org,bradnelson@chromium.org,gdeepti@chromium.org

Committed: https://crrev.com/7b3875d107d6813b458468d18850b452e03aeeea
Cr-Commit-Position: refs/heads/master@{#39288}
==========

[commit-bot: I haz the power, 2016-09-08 17:16:12]

Patchset 5 (id:??) landed as
https://crrev.com/7b3875d107d6813b458468d18850b452e03aeeea
Cr-Commit-Position: refs/heads/master@{#39288}