[wasm] implement simd lowering for replaceLane, load, store and test for phi

[wasm] implement simd lowering for replaceLane, load, store and test for phi

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

Committed: https://crrev.com/ff8bbe245abfd28074d1145ac7f88818eb47c73a
Cr-Commit-Position: refs/heads/master@{#41443}

[aseemgarg, 2016-11-15 06:35:55]

@Ben, the Simd_F32x4_Set_Global test is not working. I get the following error:
# Fatal error in ../../src/compiler/instruction.h, line 448
# Check failed: IsRegister().
Can you take a look and explain the problem.

https://codereview.chromium.org/2498283002/diff/1/test/cctest/wasm/test-run-w...
File test/cctest/wasm/test-run-wasm-simd-lowering.cc (right):

https://codereview.chromium.org/2498283002/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-simd-lowering.cc:229:
WASM_EXEC_TEST(Simd_F32x4_Set_Global) {
This is not working.

[aseemgarg, 2016-11-15 06:36:32]

Description was changed from

==========
[wasm] implement simd lowering for replaceLane, load, store and test for phi

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

to

==========
[wasm] implement simd lowering for replaceLane, load, store and test for phi

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

[aseemgarg, 2016-11-15 06:36:32]

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

[titzer, 2016-11-15 09:21:17]

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:139:
graph()->NewNode(common()->Int32Constant(static_cast<int>(i) * 4)));
Should this be 16 / kMaxLanes for clarity?

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:203: if (node->opcode() == IrOpcode::kLoad
&& rep_type == SimdType::kInt32) {
Can you factor out a subroutines GetLoadOp() and GetStoreOp()?

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:295: rep_nodes[0]->ReplaceInput(3,
rep_nodes[1]);
I think the effect chain is messed up here. I think you should feed the effect
ouput of rep_nodes[0] into the effect of rep_nodes[3] instead of effect_high
above.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:361: #define BINOP_CASE(opcode,
replacementOp)                                     \
Can you make this into a call to a subroutine? AFAICT the inline code is mostly
the same.

[Mircea Trofin, 2016-11-15 16:21:27]

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:136: for (size_t i = 1; i < kMaxLanes; i++)
{
++i

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:139:
graph()->NewNode(common()->Int32Constant(static_cast<int>(i) * 4)));
Consider using the relevant constant for "4" (or "16", after Ben's feedback)
I'd add a DCHECK that 16%kMaxLanes == 0

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:221: Node* control_high = node->InputAt(3);
aren't inputs 0 indexed? in that case, wouldn't InputAt(3) potentially fail?

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:222: rep_nodes[3] =
graph()->NewNode(load_op, base, indices[3],
This all hinges on kMaxLanes > 3 (a DCHECK?)

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:230: for (size_t i = 1; i < kMaxLanes; i++)
{
++i

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:253: Node* indices[4];
what's "4"?

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:255: Node* value = node->InputAt(2);
DCHECK node's input count

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:284: Node* effect_high = node->InputAt(3);
same question about input being 0 indexed

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:367: for (int i = 0; i < kMaxLanes; i++) { 
                                   \
++i

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:380: for (int i = 0; i < kMaxLanes; i++) {
++i

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:404: Node* repNode = node->InputAt(2);
dcheck for input count

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:406: int32_t lane =
OpParameter<int32_t>(laneNode);
dcheck lane is in bounds for rep_node

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

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.h:56: void GetIndexNodes(Node* index, Node**
new_indices);
Is 'Index' part of the SIMD nomenclature? It sounds a bit too general.

[aseemgarg, 2016-11-19 01:56:54]

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:136: for (size_t i = 1; i < kMaxLanes; i++)
{
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> ++i

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:139:
graph()->NewNode(common()->Int32Constant(static_cast<int>(i) * 4)));
On 2016/11/15 09:21:17, titzer wrote:
> Should this be 16 / kMaxLanes for clarity?

Done. But I don't get how it is clearer? 4 is just the number of bytes in
int32_t.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:203: if (node->opcode() == IrOpcode::kLoad
&& rep_type == SimdType::kInt32) {
On 2016/11/15 09:21:17, titzer wrote:
> Can you factor out a subroutines GetLoadOp() and GetStoreOp()?

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:221: Node* control_high = node->InputAt(3);
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> aren't inputs 0 indexed? in that case, wouldn't InputAt(3) potentially fail?

The check for greater than 2 just tells that there are control and effect nodes
(apart from base and index).

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:222: rep_nodes[3] =
graph()->NewNode(load_op, base, indices[3],
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> This all hinges on kMaxLanes > 3 (a DCHECK?)

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:230: for (size_t i = 1; i < kMaxLanes; i++)
{
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> ++i

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:253: Node* indices[4];
On 2016/11/15 16:21:26, Mircea Trofin wrote:
> what's "4"?

changed to kMaxLanes

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:255: Node* value = node->InputAt(2);
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> DCHECK node's input count

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:284: Node* effect_high = node->InputAt(3);
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> same question about input being 0 indexed

see above. Added Dcheck

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:295: rep_nodes[0]->ReplaceInput(3,
rep_nodes[1]);
On 2016/11/15 09:21:17, titzer wrote:
> I think the effect chain is messed up here. I think you should feed the effect
> ouput of rep_nodes[0] into the effect of rep_nodes[3] instead of effect_high
> above.

I am not quite sure I understand how effect nodes work. As I understand, effect
input basically ensures that effect node is scheduled before the current node.
In that case, it shouldn't really matter how the 4 replacements are ordered.
With the current system if we had something like effect->node->Others, it would
turn to effect->rep[3]->rep[2]->rep[1]->rep[0]->Others

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:361: #define BINOP_CASE(opcode,
replacementOp)                                     \
On 2016/11/15 09:21:17, titzer wrote:
> Can you make this into a call to a subroutine? AFAICT the inline code is
mostly
> the same.

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:367: for (int i = 0; i < kMaxLanes; i++) { 
                                   \
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> ++i

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:380: for (int i = 0; i < kMaxLanes; i++) {
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> ++i

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:404: Node* repNode = node->InputAt(2);
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> dcheck for input count

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:406: int32_t lane =
OpParameter<int32_t>(laneNode);
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> dcheck lane is in bounds for rep_node

Done.

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

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.h:56: void GetIndexNodes(Node* index, Node**
new_indices);
On 2016/11/15 16:21:27, Mircea Trofin wrote:
> Is 'Index' part of the SIMD nomenclature? It sounds a bit too general.

It isn't. it represents index for load or store. This function simply gives
replacement (in case of simd, it will be original index, and 3 more 4 bytes
apart).

[titzer, 2016-11-22 12:01:53]

lgtm with nit

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:139:
graph()->NewNode(common()->Int32Constant(static_cast<int>(i) * 4)));
On 2016/11/19 01:56:54, aseemgarg wrote:
> On 2016/11/15 09:21:17, titzer wrote:
> > Should this be 16 / kMaxLanes for clarity?
> 
> Done. But I don't get how it is clearer? 4 is just the number of bytes in
> int32_t. 

I meant, please introduce a constant that defines the width of a lane. the size
of a vector divided by the number of lanes is the width of a lane.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:284: Node* effect_high = node->InputAt(3);
Can you just name this effect_input and control_input?

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:295: rep_nodes[0]->ReplaceInput(3,
rep_nodes[1]);
On 2016/11/19 01:56:54, aseemgarg wrote:
> On 2016/11/15 09:21:17, titzer wrote:
> > I think the effect chain is messed up here. I think you should feed the
effect
> > ouput of rep_nodes[0] into the effect of rep_nodes[3] instead of effect_high
> > above.
> 
> I am not quite sure I understand how effect nodes work. 

Let me clarify a bit here. There isn't really an "effect node", there are effect
edges. Nodes can be "effectful" which means that they either have incoming or
outgoing effect edges (usually both).

> As I understand, effect
> input basically ensures that effect node is scheduled before the current node.

Yes.

> In that case, it shouldn't really matter how the 4 replacements are ordered.

Yep.

> With the current system if we had something like effect->node->Others, it
would
> turn to effect->rep[3]->rep[2]->rep[1]->rep[0]->Others

Ok. I see. I usually think of the edges going in the other direction:

effect<-rep[3]<-rep[2]<-rep[1]<-rep[0]<-Others

Upon a second look (not sure if you changed anything here), yes, this should
work. I was concerned that the effect chain you had left one of them out.

https://codereview.chromium.org/2498283002/diff/20001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-simd-lowering.cc (right):

https://codereview.chromium.org/2498283002/diff/20001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-simd-lowering.cc:98:
WASM_EXEC_TEST(Simd_I32x4_Local) {
You should just check whether the execution_mode is kExecuteInterpreted, and if
true, return. Otherwise you are just running the same test in both modes.

[aseemgarg, 2016-11-23 23:07:56]

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
File src/compiler/simd-scalar-lowering.cc (right):

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:139:
graph()->NewNode(common()->Int32Constant(static_cast<int>(i) * 4)));
On 2016/11/22 12:01:52, titzer wrote:
> On 2016/11/19 01:56:54, aseemgarg wrote:
> > On 2016/11/15 09:21:17, titzer wrote:
> > > Should this be 16 / kMaxLanes for clarity?
> > 
> > Done. But I don't get how it is clearer? 4 is just the number of bytes in
> > int32_t. 
> 
> I meant, please introduce a constant that defines the width of a lane. the
size
> of a vector divided by the number of lanes is the width of a lane.

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:284: Node* effect_high = node->InputAt(3);
On 2016/11/22 12:01:52, titzer wrote:
> Can you just name this effect_input and control_input?

Done.

https://codereview.chromium.org/2498283002/diff/1/src/compiler/simd-scalar-lo...
src/compiler/simd-scalar-lowering.cc:295: rep_nodes[0]->ReplaceInput(3,
rep_nodes[1]);
Didn't change anything. Leaving as is

On 2016/11/22 12:01:52, titzer wrote:
> On 2016/11/19 01:56:54, aseemgarg wrote:
> > On 2016/11/15 09:21:17, titzer wrote:
> > > I think the effect chain is messed up here. I think you should feed the
> effect
> > > ouput of rep_nodes[0] into the effect of rep_nodes[3] instead of
effect_high
> > > above.
> > 
> > I am not quite sure I understand how effect nodes work. 
> 
> Let me clarify a bit here. There isn't really an "effect node", there are
effect
> edges. Nodes can be "effectful" which means that they either have incoming or
> outgoing effect edges (usually both).
> 
> > As I understand, effect
> > input basically ensures that effect node is scheduled before the current
node.
> 
> Yes.
> 
> > In that case, it shouldn't really matter how the 4 replacements are ordered.
> 
> Yep.
> 
> > With the current system if we had something like effect->node->Others, it
> would
> > turn to effect->rep[3]->rep[2]->rep[1]->rep[0]->Others
> 
> Ok. I see. I usually think of the edges going in the other direction:
> 
> effect<-rep[3]<-rep[2]<-rep[1]<-rep[0]<-Others
> 
> Upon a second look (not sure if you changed anything here), yes, this should
> work. I was concerned that the effect chain you had left one of them out.

https://codereview.chromium.org/2498283002/diff/20001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-simd-lowering.cc (right):

https://codereview.chromium.org/2498283002/diff/20001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-simd-lowering.cc:98:
WASM_EXEC_TEST(Simd_I32x4_Local) {
On 2016/11/22 12:01:52, titzer wrote:
> You should just check whether the execution_mode is kExecuteInterpreted, and
if
> true, return. Otherwise you are just running the same test in both modes.

Instead of checking and returning, I created a new macro
WASM_EXEC_COMPILED_TEST.

https://codereview.chromium.org/2498283002/diff/40001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-simd-lowering.cc (right):

https://codereview.chromium.org/2498283002/diff/40001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-simd-lowering.cc:229:
WASM_EXEC_COMPILED_TEST(Simd_F32x4_Set_Global) {
This test fails with the following error:
#
# Fatal error in ../../src/compiler/instruction.h, line 450
# Check failed: IsRegister().
#

[titzer, 2016-11-24 16:51:09]

lgtm

[aseemgarg, 2016-12-02 03:12:28]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2016-12-02 03:12:28]

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/2498283002/#ps60001
(title: "[wasm] implement simd lowering for replaceLane, load, store and test
for phi")

[commit-bot: I haz the power, 2016-12-02 03:12:36]

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-12-02 03:47:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480648348133670,
"parent_rev": "260db19db134d0c447049de08200a15cd14818db", "commit_rev":
"dc2522e51a5b99c2262b11078b2d49e502515277"}

[commit-bot: I haz the power, 2016-12-02 03:47:26]

Description was changed from

==========
[wasm] implement simd lowering for replaceLane, load, store and test for phi

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

to

==========
[wasm] implement simd lowering for replaceLane, load, store and test for phi

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

[commit-bot: I haz the power, 2016-12-02 03:47:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-02 03:47:55]

Description was changed from

==========
[wasm] implement simd lowering for replaceLane, load, store and test for phi

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

to

==========
[wasm] implement simd lowering for replaceLane, load, store and test for phi

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

Committed: https://crrev.com/ff8bbe245abfd28074d1145ac7f88818eb47c73a
Cr-Commit-Position: refs/heads/master@{#41443}
==========

[commit-bot: I haz the power, 2016-12-02 03:47:56]

Patchset 4 (id:??) landed as
https://crrev.com/ff8bbe245abfd28074d1145ac7f88818eb47c73a
Cr-Commit-Position: refs/heads/master@{#41443}