Convert SIMD wasm ops to runtime function calls

Convert SIMD wasm ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Decode SIMD prefix, wasm opcodes correctly
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test

LOG=N
BUG=v8:4124
R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org

Committed: https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Committed: https://crrev.com/18543ff1da34a7b12f583375b53320fcf5013931
Cr-Original-Commit-Position: refs/heads/master@{#37789}
Cr-Commit-Position: refs/heads/master@{#37807}

[gdeepti1, 2016-05-19 08:58:52]

gdeepti@google.com changed reviewers:
+ gdeepti@google.com

[bbudge, 2016-05-19 09:14:28]

https://codereview.chromium.org/1991143002/diff/1/src/compiler/simd-lowering.h
File src/compiler/simd-lowering.h (right):

https://codereview.chromium.org/1991143002/diff/1/src/compiler/simd-lowering....
src/compiler/simd-lowering.h:22: explicit SimdLowering(JSGraph* jsgraph,
WasmGraphBuilder* builder,
explicit not necessary

https://codereview.chromium.org/1991143002/diff/1/src/compiler/simd-lowering....
src/compiler/simd-lowering.h:25: ~SimdLowering() final;
virtual

https://codereview.chromium.org/1991143002/diff/1/src/compiler/simd-lowering....
src/compiler/simd-lowering.h:27: Reduction Reduce(Node* node) final;
override

[gdeepti, 2016-05-19 09:18:10]

Description was changed from

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Seperate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

to

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Seperate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

[gdeepti, 2016-05-19 09:18:10]

gdeepti@chromium.org changed reviewers:
- gdeepti@google.com

[gdeepti, 2016-05-19 09:34:09]

https://codereview.chromium.org/1991143002/diff/1/src/compiler/simd-lowering.h
File src/compiler/simd-lowering.h (right):

https://codereview.chromium.org/1991143002/diff/1/src/compiler/simd-lowering....
src/compiler/simd-lowering.h:22: explicit SimdLowering(JSGraph* jsgraph,
WasmGraphBuilder* builder,
On 2016/05/19 09:14:28, bbudge wrote:
> explicit not necessary

Done.

https://codereview.chromium.org/1991143002/diff/1/src/compiler/simd-lowering....
src/compiler/simd-lowering.h:25: ~SimdLowering() final;
On 2016/05/19 09:14:28, bbudge wrote:
> virtual

Done.

https://codereview.chromium.org/1991143002/diff/1/src/compiler/simd-lowering....
src/compiler/simd-lowering.h:27: Reduction Reduce(Node* node) final;
On 2016/05/19 09:14:28, bbudge wrote:
> override

Done.

[titzer, 2016-05-19 09:43:27]

Can we split the opcode-related stuff into a separate CL?

For opcodes, I think we'll need a prefix byte (SIMD) and then a separate list of
SIMD operations.

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.h:173: void set_is_simd_function(bool simd) {
this->is_simd_function_ = simd; }
I think this should be "has simd ops" and should be hidden in the private
implementation details (i.e. only set when internally a SIMD operation is
detected).

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

https://codereview.chromium.org/1991143002/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:607: if (builder_)
builder_->set_is_simd_function(WasmOpcodes::IsSimd(opcode));
That's too expensive to do for every bytecode.

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

https://codereview.chromium.org/1991143002/diff/20001/src/wasm/wasm-opcodes.c...
src/wasm/wasm-opcodes.cc:82: bool WasmOpcodes::IsSimd(WasmOpcode opcode) {
Could we make this into a range check instead of a switch?

https://codereview.chromium.org/1991143002/diff/20001/src/wasm/wasm-opcodes.c...
src/wasm/wasm-opcodes.cc:86: if (kExpr##name == opcode) {          \
The if here is redundant.

https://codereview.chromium.org/1991143002/diff/20001/test/unittests/wasm/ast...
File test/unittests/wasm/ast-decoder-unittest.cc (right):

https://codereview.chromium.org/1991143002/diff/20001/test/unittests/wasm/ast...
test/unittests/wasm/ast-decoder-unittest.cc:912: /*
Should this test pass?

[bbudge, 2016-05-19 09:53:14]

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
File src/compiler/simd-lowering.cc (right):

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
src/compiler/simd-lowering.cc:67: kNone, kNone, kNone, kNone, kNone, kNone,
kNone, kNone,        \
I'm not sure what the compiler does with this repeated in every block, but why
not move it outside the macro (and define it at top level function scope)?

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
src/compiler/simd-lowering.cc:87: Node* inputs[16 + 6];
Need some constants defined for these magic numbers, e.g.
kFunctionParams

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
src/compiler/simd-lowering.cc:128: switch (signature[0]) {
Maybe this would benefit with a well named local, i.e.
Conversion return_type = signature[0];
switch (return_type) ...

[titzer, 2016-05-19 09:58:38]

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
File src/compiler/simd-lowering.cc (right):

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
src/compiler/simd-lowering.cc:28: Conversion signature[] = {
You can use "static const" here to make the C++ compiler move this to the
read-only data part of the binary.

[gdeepti, 2016-07-01 22:24:48]

PTAL 

Wasm opcodes landed, there's a little bit of opcode related code mostly just to
decode Simd prefix, I can split it out to a different CL if needed. Kept it in
to test end to end.

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
File src/compiler/simd-lowering.cc (right):

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
src/compiler/simd-lowering.cc:28: Conversion signature[] = {
On 2016/05/19 09:58:38, titzer wrote:
> You can use "static const" here to make the C++ compiler move this to the
> read-only data part of the binary.

Done.

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
src/compiler/simd-lowering.cc:67: kNone, kNone, kNone, kNone, kNone, kNone,
kNone, kNone,        \
On 2016/05/19 09:53:13, bbudge wrote:
> I'm not sure what the compiler does with this repeated in every block, but why
> not move it outside the macro (and define it at top level function scope)?

Done.

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
src/compiler/simd-lowering.cc:87: Node* inputs[16 + 6];
On 2016/05/19 09:53:13, bbudge wrote:
> Need some constants defined for these magic numbers, e.g.
> kFunctionParams

Done.

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/simd-lower...
src/compiler/simd-lowering.cc:128: switch (signature[0]) {
On 2016/05/19 09:53:14, bbudge wrote:
> Maybe this would benefit with a well named local, i.e.
> Conversion return_type = signature[0];
> switch (return_type) ...

Done.

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/1991143002/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.h:173: void set_is_simd_function(bool simd) {
this->is_simd_function_ = simd; }
On 2016/05/19 09:43:27, titzer wrote:
> I think this should be "has simd ops" and should be hidden in the private
> implementation details (i.e. only set when internally a SIMD operation is
> detected).

Changed to HasSimdOps, and is now only set internally in the compiler.

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

https://codereview.chromium.org/1991143002/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:607: if (builder_)
builder_->set_is_simd_function(WasmOpcodes::IsSimd(opcode));
On 2016/05/19 09:43:27, titzer wrote:
> That's too expensive to do for every bytecode.

Converted to check if the opcode byte is a prefix.

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

https://codereview.chromium.org/1991143002/diff/20001/src/wasm/wasm-opcodes.c...
src/wasm/wasm-opcodes.cc:82: bool WasmOpcodes::IsSimd(WasmOpcode opcode) {
On 2016/05/19 09:43:27, titzer wrote:
> Could we make this into a range check instead of a switch?

Converted to a prefix check.

https://codereview.chromium.org/1991143002/diff/20001/src/wasm/wasm-opcodes.c...
src/wasm/wasm-opcodes.cc:86: if (kExpr##name == opcode) {          \
On 2016/05/19 09:43:27, titzer wrote:
> The if here is redundant.

Function now removed.

https://codereview.chromium.org/1991143002/diff/20001/test/unittests/wasm/ast...
File test/unittests/wasm/ast-decoder-unittest.cc (right):

https://codereview.chromium.org/1991143002/diff/20001/test/unittests/wasm/ast...
test/unittests/wasm/ast-decoder-unittest.cc:912: /*
On 2016/05/19 09:43:27, titzer wrote:
> Should this test pass?

Stale test, removed.

[bbudge, 2016-07-06 20:18:08]

https://codereview.chromium.org/1991143002/diff/120001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/1991143002/diff/120001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:167: Node* BuildChangeTaggedToFloat64(Node* value);
It's unfortunate we have to perturb this API, given that this may be a temporary
implementation of SIMD.

Looking at the SimdLowering class, it seems to me that you could just move all
of that code into WasmGraphBuilder. That way, you'd have access to all the
builder functions, and you wouldn't need the HasSimdOps/SetHasSimdOps methods or
a separate SimdLowering pass. You could even make the ChangeToRuntimeCall method
public on WasmGraphBuilder. It seems like it could be generally useful.

https://codereview.chromium.org/1991143002/diff/120001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:202: Node* DefaultS128Value();
This method logically belongs near Float64Constant. Then we don't need a
separate SIMD section.

https://codereview.chromium.org/1991143002/diff/120001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:203: bool HasSimdOps() { return has_simd_ops_; }
nit: simple getter style would be has_simd_ops

https://codereview.chromium.org/1991143002/diff/120001/test/mjsunit/wasm/test...
File test/mjsunit/wasm/test-wasm-simd-ops.js (right):

https://codereview.chromium.org/1991143002/diff/120001/test/mjsunit/wasm/test...
test/mjsunit/wasm/test-wasm-simd-ops.js:20: kExprGetLocal, 1, kExprSimdPrefix,
kExprI32x4ExtractLane])
If this is a parameter, then we can't determine its value at compile time, and
I32x4ExtractLane can't take a non-immediate argument (your runtime function
lowering works because that function can take a non-immediate argument.)

The wasm code should either create a switch and case the legal values, or just
make this kExprI32Const with the value 2.

[gdeepti, 2016-07-07 04:05:34]

https://codereview.chromium.org/1991143002/diff/120001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/1991143002/diff/120001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:167: Node* BuildChangeTaggedToFloat64(Node* value);
On 2016/07/06 20:18:08, bbudge wrote:
> It's unfortunate we have to perturb this API, given that this may be a
temporary
> implementation of SIMD.
> 
> Looking at the SimdLowering class, it seems to me that you could just move all
> of that code into WasmGraphBuilder. That way, you'd have access to all the
> builder functions, and you wouldn't need the HasSimdOps/SetHasSimdOps methods
or
> a separate SimdLowering pass. You could even make the ChangeToRuntimeCall
method
> public on WasmGraphBuilder. It seems like it could be generally useful.

Refactored SimdLowering class, ChangeToRuntimeCall is now a public method on
WasmGraphBuilder so that there are no API changes and still have access to
builder methods. Using the SimdLowering pass because there are a significant
number of opcodes that need to be added and I'm concerned that that means
conflating the wasm-compiler.cc file with a non trivial amount of temporary
code. Currently refactored so that only the code that would need to be removed
when moving away from the runtime function conversion is in its own file. Also,
in the future the getter/setter for has_simd_ops_ (with a name change) can be an
easy way to toggle between the runtime/native implementation.

https://codereview.chromium.org/1991143002/diff/120001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:202: Node* DefaultS128Value();
On 2016/07/06 20:18:08, bbudge wrote:
> This method logically belongs near Float64Constant. Then we don't need a
> separate SIMD section.

Moved method, removed SIMD section.

https://codereview.chromium.org/1991143002/diff/120001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:203: bool HasSimdOps() { return has_simd_ops_; }
On 2016/07/06 20:18:08, bbudge wrote:
> nit: simple getter style would be has_simd_ops

Done.

https://codereview.chromium.org/1991143002/diff/120001/test/mjsunit/wasm/test...
File test/mjsunit/wasm/test-wasm-simd-ops.js (right):

https://codereview.chromium.org/1991143002/diff/120001/test/mjsunit/wasm/test...
test/mjsunit/wasm/test-wasm-simd-ops.js:20: kExprGetLocal, 1, kExprSimdPrefix,
kExprI32x4ExtractLane])
On 2016/07/06 20:18:08, bbudge wrote:
> If this is a parameter, then we can't determine its value at compile time, and
> I32x4ExtractLane can't take a non-immediate argument (your runtime function
> lowering works because that function can take a non-immediate argument.)
> 
> The wasm code should either create a switch and case the legal values, or just
> make this kExprI32Const with the value 2.

Changed to kExprI32Const with the value 2. Added this as a sanity check to make
sure the runtime pass is working, will follow up with more comprehensive tests
to make sure the opcodes are working as intended.

[gdeepti, 2016-07-08 00:09:25]

https://codereview.chromium.org/1991143002/diff/120001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/1991143002/diff/120001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:167: Node* BuildChangeTaggedToFloat64(Node* value);
On 2016/07/07 04:05:34, gdeepti wrote:
> On 2016/07/06 20:18:08, bbudge wrote:
> > It's unfortunate we have to perturb this API, given that this may be a
> temporary
> > implementation of SIMD.
> > 
> > Looking at the SimdLowering class, it seems to me that you could just move
all
> > of that code into WasmGraphBuilder. That way, you'd have access to all the
> > builder functions, and you wouldn't need the HasSimdOps/SetHasSimdOps
methods
> or
> > a separate SimdLowering pass. You could even make the ChangeToRuntimeCall
> method
> > public on WasmGraphBuilder. It seems like it could be generally useful.
> 
> Refactored SimdLowering class, ChangeToRuntimeCall is now a public method on
> WasmGraphBuilder so that there are no API changes and still have access to
> builder methods. Using the SimdLowering pass because there are a significant
> number of opcodes that need to be added and I'm concerned that that means
> conflating the wasm-compiler.cc file with a non trivial amount of temporary
> code. Currently refactored so that only the code that would need to be removed
> when moving away from the runtime function conversion is in its own file.
Also,
> in the future the getter/setter for has_simd_ops_ (with a name change) can be
an
> easy way to toggle between the runtime/native implementation. 

The last part in my previous comment about the boolean is incorrect, in the
current implementation there is no way to toggle between native/runtime
implementations per opcode in the WasmGraphBuilder. The only way that can be
done is in the  opcode switch statement in the SimdLowering pass. That said I'm
leaning towards keeping it separate only for keeping wasm-compiler.cc clear of
the temp code, not sure what the right thing to do in this case. Ben, thoughts?

[titzer, 2016-07-08 14:16:03]

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

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:616: set_has_simd_ops(true);
Can you add a helper method called simd() that returns the MachineOperator
builder and also sets the has_simd_ops?

That way this code will be 

graph()->NewNode(simd()->Int32x4ExtractLane(), left, right);

And it will be harder to forget this in the future.
(also we should probably split the SIMD operations into their own
OperatorBuilder).

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2733: // Either there are control + effect or not.
When do runtime calls not have effect and control inputs?

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:167: const Conversion* signature);
Can we make this a Signature<Conversion>, or better yet, just pass in the
FunctionSig and have that done automatically by this method?

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

https://codereview.chromium.org/1991143002/diff/160001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:594: if (opcode == kSimdPrefix) {
Is it possible to make the prefix part of the main switch case below, and then
have a secondary switch in the function you mention in this comment?

https://codereview.chromium.org/1991143002/diff/160001/src/wasm/wasm-opcodes.cc
File src/wasm/wasm-opcodes.cc (right):

https://codereview.chromium.org/1991143002/diff/160001/src/wasm/wasm-opcodes....
src/wasm/wasm-opcodes.cc:89: // Initialize the signature table.
You can just merge this with the above function and rename it to
InitSigTables().

[bbudge, 2016-07-08 17:17:29]

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:361: void set_has_simd_ops(bool has_simd_ops) {
has_simd_ops_ = has_simd_ops; }
Forgot to comment here. I can't think of a use case for setting this to false,
so omit the parameter and always set 'true'.

[gdeepti, 2016-07-11 09:50:34]

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

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:616: set_has_simd_ops(true);
On 2016/07/08 14:16:03, titzer wrote:
> Can you add a helper method called simd() that returns the MachineOperator
> builder and also sets the has_simd_ops?
> 
> That way this code will be 
> 
> graph()->NewNode(simd()->Int32x4ExtractLane(), left, right);
> 
> And it will be harder to forget this in the future.
> (also we should probably split the SIMD operations into their own
> OperatorBuilder).

Added simd() helper method, a little fuzzy on what you mean when you say
OperatorBuilder. Do you mean an OperatorBuilder like
MachineOperatorBuilder/SimplifiedOperatorBuilder or just a method that builds
Simd ops? Added a SimdOp method that does not piggyback on Unop/Binop for now,
let me know if I've misunderstood.

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2733: // Either there are control + effect or not.
On 2016/07/08 14:16:03, titzer wrote:
> When do runtime calls not have effect and control inputs?

Runtime calls always have effect and control, this check is for input count on
the node. For everything except loads and stores these are the start nodes, but
loads and stores (not implemented yet) will need additional inputs for effect
and control, hence the check. Would you prefer that I defer checking this till
Loads/Stores are added?

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:167: const Conversion* signature);
On 2016/07/08 14:16:03, titzer wrote:
> Can we make this a Signature<Conversion>, or better yet, just pass in the
> FunctionSig and have that done automatically by this method?

Refactored this function and simd-lowering.cc to use Signature<Conversion>.
Experimented with passing in FunctionSig but that turns out to be quite verbose
because in some cases there is no 1-1 mapping between the runtime function
signatures and the wasm opcode signatures.

https://codereview.chromium.org/1991143002/diff/160001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.h:361: void set_has_simd_ops(bool has_simd_ops) {
has_simd_ops_ = has_simd_ops; }
On 2016/07/08 17:17:29, bbudge wrote:
> Forgot to comment here. I can't think of a use case for setting this to false,
> so omit the parameter and always set 'true'.

Replaced setter with simd() method that sets this variable and return the
MachineOperatorBuilder.

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

https://codereview.chromium.org/1991143002/diff/160001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:594: if (opcode == kSimdPrefix) {
On 2016/07/08 14:16:03, titzer wrote:
> Is it possible to make the prefix part of the main switch case below, and then
> have a secondary switch in the function you mention in this comment?

Done.

https://codereview.chromium.org/1991143002/diff/160001/src/wasm/wasm-opcodes.cc
File src/wasm/wasm-opcodes.cc (right):

https://codereview.chromium.org/1991143002/diff/160001/src/wasm/wasm-opcodes....
src/wasm/wasm-opcodes.cc:89: // Initialize the signature table.
On 2016/07/08 14:16:03, titzer wrote:
> You can just merge this with the above function and rename it to
> InitSigTables().

Done.

[gdeepti, 2016-07-12 16:29:24]

PTAL

[bradnelson, 2016-07-13 18:00:50]

Description was changed from

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Seperate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

to

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

[bradnelson, 2016-07-13 18:01:44]

Description was changed from

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

to

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.


LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

[bradnelson, 2016-07-13 18:01:52]

Description was changed from

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.


LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

to

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

[bradnelson, 2016-07-13 18:10:42]

I think this is close if you put it behind a flag.
But we do probably want someone with better knowledge of src/compiler to
sign-off.

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

https://codereview.chromium.org/1991143002/diff/220001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1003: DecodeSimdOpcode(opcode);
Add an --enable-wasm-simd flag, and error if this shows up without it on.

https://codereview.chromium.org/1991143002/diff/220001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1114: for (int i =
static_cast<int>(sig->parameter_count()); i > 0; i--) {
size_t drop cast?

[gdeepti, 2016-07-13 18:58:35]

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

https://codereview.chromium.org/1991143002/diff/220001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1003: DecodeSimdOpcode(opcode);
On 2016/07/13 18:10:42, bradnelson wrote:
> Add an --enable-wasm-simd flag, and error if this shows up without it on.


Added --enable-wasm-simd flag, falls through to invalid opcode case without the
flag.

https://codereview.chromium.org/1991143002/diff/220001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1114: for (int i =
static_cast<int>(sig->parameter_count()); i > 0; i--) {
On 2016/07/13 18:10:42, bradnelson wrote:
> size_t drop cast?

Done.

[titzer, 2016-07-13 19:45:10]

lgtm modulo remaining comments

https://codereview.chromium.org/1991143002/diff/260001/src/compiler/simd-lowe...
File src/compiler/simd-lowering.cc (right):

https://codereview.chromium.org/1991143002/diff/260001/src/compiler/simd-lowe...
src/compiler/simd-lowering.cc:23: ConversionSignature::Builder
conversions(zone_, kReturnCount, kSimd32x4);
Is it possible to cache these signatures? You could, e.g. store the arrays as
static data in this C++ file and just create the signatures around them.

https://codereview.chromium.org/1991143002/diff/260001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1991143002/diff/260001/src/flag-definitions.h...
src/flag-definitions.h:517: DEFINE_BOOL(enable_wasm_simd, false, "enable simd
opcodes for wasm")
s/enable_wasm_simd/wasm_simd_prototype/

Maybe there is a convention for harmony staging flags here that we should
follow?

https://codereview.chromium.org/1991143002/diff/260001/test/mjsunit/wasm/test...
File test/mjsunit/wasm/test-wasm-simd-ops.js (right):

https://codereview.chromium.org/1991143002/diff/260001/test/mjsunit/wasm/test...
test/mjsunit/wasm/test-wasm-simd-ops.js:15: (function SimdSplatExtractTest() {
Can we get some test/cctest/wasm/test-run-simd.cc tests as well?

That way we can easily test the interpreter as well (later).

[gdeepti, 2016-07-15 08:26:40]

https://codereview.chromium.org/1991143002/diff/260001/src/compiler/simd-lowe...
File src/compiler/simd-lowering.cc (right):

https://codereview.chromium.org/1991143002/diff/260001/src/compiler/simd-lowe...
src/compiler/simd-lowering.cc:23: ConversionSignature::Builder
conversions(zone_, kReturnCount, kSimd32x4);
On 2016/07/13 19:45:09, titzer wrote:
> Is it possible to cache these signatures? You could, e.g. store the arrays as
> static data in this C++ file and just create the signatures around them.

Added signatures as static variables, signatures are initialized in the
constructor and cached for the rest of the reduction pass.

https://codereview.chromium.org/1991143002/diff/260001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1991143002/diff/260001/src/flag-definitions.h...
src/flag-definitions.h:517: DEFINE_BOOL(enable_wasm_simd, false, "enable simd
opcodes for wasm")
On 2016/07/13 19:45:10, titzer wrote:
> s/enable_wasm_simd/wasm_simd_prototype/
> 
> Maybe there is a convention for harmony staging flags here that we should
> follow?

Done. There is a harmony_simd flag for simd.js, added wasm_simd_prototype to
keep them separate.

https://codereview.chromium.org/1991143002/diff/260001/test/mjsunit/wasm/test...
File test/mjsunit/wasm/test-wasm-simd-ops.js (right):

https://codereview.chromium.org/1991143002/diff/260001/test/mjsunit/wasm/test...
test/mjsunit/wasm/test-wasm-simd-ops.js:15: (function SimdSplatExtractTest() {
On 2016/07/13 19:45:10, titzer wrote:
> Can we get some test/cctest/wasm/test-run-simd.cc tests as well?
> 
> That way we can easily test the interpreter as well (later).

Added test-run-wasm-simd.cc with a splat/extract test.

[gdeepti, 2016-07-15 08:26:57]

The CQ bit was checked by gdeepti@chromium.org

[gdeepti, 2016-07-15 08:26:57]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/1991143002/#ps320001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-15 08:27: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, 2016-07-15 08:29:11]

Description was changed from

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

to

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

[commit-bot: I haz the power, 2016-07-15 08:29:12]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-07-15 08:30:33]

Description was changed from

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org
==========

to

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org

Committed: https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Cr-Commit-Position: refs/heads/master@{#37789}
==========

[commit-bot: I haz the power, 2016-07-15 08:30:34]

Patchset 17 (id:??) landed as
https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Cr-Commit-Position: refs/heads/master@{#37789}

[Michael Achenbach, 2016-07-15 08:59:22]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/2154723002/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Violates msan and tsan:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20...
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20TSAN/builds/10670.

[gdeepti, 2016-07-16 01:26:03]

Description was changed from

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org

Committed: https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Cr-Commit-Position: refs/heads/master@{#37789}
==========

to

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org

Committed: https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Cr-Commit-Position: refs/heads/master@{#37789}
==========

[gdeepti, 2016-07-16 01:27:09]

Description was changed from

==========
Convert SIMD machine ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 - Separate out generic SIMD Machine ops as these cannot be
 handled by runtime functions just yet.

LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org

Committed: https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Cr-Commit-Position: refs/heads/master@{#37789}
==========

to

==========
Convert SIMD wasm ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Decode SIMD prefix, wasm opcodes correctly
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 
LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org

Committed: https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Cr-Commit-Position: refs/heads/master@{#37789}
==========

[gdeepti, 2016-07-16 01:29:02]

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

[commit-bot: I haz the power, 2016-07-16 01:29:09]

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, 2016-07-16 02:57:47]

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

[commit-bot: I haz the power, 2016-07-16 02:57:48]

Dry run: This issue passed the CQ dry run.

[gdeepti, 2016-07-16 03:47:12]

On 2016/07/15 08:59:22, Michael Achenbach wrote:
> A revert of this CL (patchset #17 id:320001) has been created in
> https://codereview.chromium.org/2154723002/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is: [Sheriff] Violates msan and tsan:
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20...
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20TSAN/builds/10670.

Fixed failures, rerun of msan and tsan bots are green. Relanding this patch.

[gdeepti, 2016-07-16 03:47:22]

The CQ bit was checked by gdeepti@chromium.org

[gdeepti, 2016-07-16 03:47:22]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/1991143002/#ps340001
(title: "Fix bot fails")

[commit-bot: I haz the power, 2016-07-16 03:47:33]

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-07-16 03:49:52]

Description was changed from

==========
Convert SIMD wasm ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Decode SIMD prefix, wasm opcodes correctly
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 
LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org

Committed: https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Cr-Commit-Position: refs/heads/master@{#37789}
==========

to

==========
Convert SIMD wasm ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Decode SIMD prefix, wasm opcodes correctly
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 
LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org

Committed: https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Cr-Commit-Position: refs/heads/master@{#37789}
==========

[commit-bot: I haz the power, 2016-07-16 03:49:53]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-07-16 03:50:59]

Description was changed from

==========
Convert SIMD wasm ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Decode SIMD prefix, wasm opcodes correctly
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test
 
LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org

Committed: https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Cr-Commit-Position: refs/heads/master@{#37789}
==========

to

==========
Convert SIMD wasm ops to runtime function calls
 - Add Simd128 type to Wasm AST types
 - Decode SIMD prefix, wasm opcodes correctly
 - Add a pass that converts SIMD machine ops to runtime calls
 - Sample opcodes Int32x4Splat, Int32x4ExtractLane and test

LOG=N
BUG=v8:4124

R=bradnelson@chromium.org, bbudge@chromium.org, titzer@chromium.org

Committed: https://crrev.com/73df92fc2fdbbfadc17e8ab4e58ec56ae2b3d91a
Committed: https://crrev.com/18543ff1da34a7b12f583375b53320fcf5013931
Cr-Original-Commit-Position: refs/heads/master@{#37789}
Cr-Commit-Position: refs/heads/master@{#37807}
==========

[commit-bot: I haz the power, 2016-07-16 03:51:01]

Patchset 18 (id:??) landed as
https://crrev.com/18543ff1da34a7b12f583375b53320fcf5013931
Cr-Commit-Position: refs/heads/master@{#37807}