[wasm] Implement I32x4ReplaceLane, I32x4Add, I32x4Sub.

[wasm] Implement I32x4ReplaceLane, I32x4Add, I32x4Sub.

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

Committed: https://crrev.com/02c6b04179a632a3d7a493e60086e5f4c5f47f95
Cr-Commit-Position: refs/heads/master@{#41505}

[gdeepti, 2016-10-03 22:40:42]

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

[commit-bot: I haz the power, 2016-10-03 22:40:48]

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-10-03 23:15:11]

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

[commit-bot: I haz the power, 2016-10-03 23:15:12]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-10-04 20:34:26]

lgtm w/comment

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

https://codereview.chromium.org/2385393002/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:1256: error(pc_, pc_ + 2, "invalid extract lane value");
s/extract/replace

I think there are enough SIMD instructions that take immediate values for lanes
that it would be worthwhile to extract a helper method to read an immediate
value, range check, and create the Int32Constant input. This will eliminate a
lot of repetitive code. The only downside is that the error message would have
to be more generic, e.g. "invalid immediate value" but I think the error message
and pc information is enough information.

Also, you should clamp out-of-range values to the legal range here so we don't
have to worry about weird behavior in the assembler. Or does the call to 'error'
make that unnecessary?

[titzer, 2016-10-07 12:20:14]

https://codereview.chromium.org/2385393002/diff/20001/src/compiler/x64/instru...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2385393002/diff/20001/src/compiler/x64/instru...
src/compiler/x64/instruction-selector-x64.cc:2283: Emit(kX64Int32x4Sub,
g.DefineAsRegister(node),
As mentioned offline, these will have to be DefineSameAsFirst.

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

https://codereview.chromium.org/2385393002/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1248: uint8_t LaneValue(uint8_t max_lanes) {
Maybe you should introduce a LaneOperand, since if we eventually do implement
this in the interpreter, we can use it there, too?

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

https://codereview.chromium.org/2385393002/diff/20001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-simd.cc:89: WASM_EXEC_TEST(I32x4Add) {
I think these tests can get a bit simpler. E.g. what if we have a test that only
checks a single lane (but that lane is a parameter), and then we run the test 4
times, each with one lane?

It will also get simpler when we have load/store to memory, because we can then
just write the results to memory and check them from outside.

[gdeepti, 2016-11-22 00:42:11]

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

[commit-bot: I haz the power, 2016-11-22 00:42:18]

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

[gdeepti, 2016-11-22 00:57:56]

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

[commit-bot: I haz the power, 2016-11-22 00:57:58]

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

[gdeepti, 2016-11-22 01:15:20]

Resurrecting this CL, rebased after ScalarLowering, removed additional move
instructions. PTAL

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

https://codereview.chromium.org/2385393002/diff/1/src/wasm/ast-decoder.cc#new...
src/wasm/ast-decoder.cc:1256: error(pc_, pc_ + 2, "invalid extract lane value");
On 2016/10/04 20:34:26, bbudge wrote:
> s/extract/replace
> 
> I think there are enough SIMD instructions that take immediate values for
lanes
> that it would be worthwhile to extract a helper method to read an immediate
> value, range check, and create the Int32Constant input. This will eliminate a
> lot of repetitive code. The only downside is that the error message would have
> to be more generic, e.g. "invalid immediate value" but I think the error
message
> and pc information is enough information.
> 
> Also, you should clamp out-of-range values to the legal range here so we don't
> have to worry about weird behavior in the assembler. Or does the call to
'error'
> make that unnecessary?

Using LaneOperand as introduced in the ScalarLowering pass that introduces
LaneOperand. The validate function verifies that the lane values are in the
right range. I think error returns a decode error so it does not go to the
assembler.

https://codereview.chromium.org/2385393002/diff/20001/src/compiler/x64/instru...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2385393002/diff/20001/src/compiler/x64/instru...
src/compiler/x64/instruction-selector-x64.cc:2283: Emit(kX64Int32x4Sub,
g.DefineAsRegister(node),
On 2016/10/07 12:20:14, titzer wrote:
> As mentioned offline, these will have to be DefineSameAsFirst.

Done.

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

https://codereview.chromium.org/2385393002/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1248: uint8_t LaneValue(uint8_t max_lanes) {
On 2016/10/07 12:20:14, titzer wrote:
> Maybe you should introduce a LaneOperand, since if we eventually do implement
> this in the interpreter, we can use it there, too?

LaneOperand introduced by ScalarLowering, merged to use here as well.

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

https://codereview.chromium.org/2385393002/diff/20001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-simd.cc:89: WASM_EXEC_TEST(I32x4Add) {
On 2016/10/07 12:20:14, titzer wrote:
> I think these tests can get a bit simpler. E.g. what if we have a test that
only
> checks a single lane (but that lane is a parameter), and then we run the test
4
> times, each with one lane?
> 
> It will also get simpler when we have load/store to memory, because we can
then
> just write the results to memory and check them from outside.

bbudge@ suggested using only splat values instead of splat+replace values and
testing for a range of Int32 values for improving tests, incorporated tests from
-
https://codereview.chromium.org/2454193002/diff/20001/test/cctest/wasm/test-r...

[commit-bot: I haz the power, 2016-11-22 01:42:55]

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

[commit-bot: I haz the power, 2016-11-22 01:42:57]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-11-22 18:06:42]

A comment, otherwise still LGTM

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

https://codereview.chromium.org/2385393002/diff/80001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-simd.cc:31: WASM_RETURN1(WASM_ONE)
This is my fault, but if we return 1 here, then we can't have multiple checks in
a function. So move this out and add it at the end of the test functions. I
discovered this while examining the code. All but the first check are removed by
the compiler as dead code. At least we know that works!

[gdeepti, 2016-11-22 22:30:33]

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

https://codereview.chromium.org/2385393002/diff/80001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-simd.cc:31: WASM_RETURN1(WASM_ONE)
On 2016/11/22 18:06:42, bbudge wrote:
> This is my fault, but if we return 1 here, then we can't have multiple checks
in
> a function. So move this out and add it at the end of the test functions. I
> discovered this while examining the code. All but the first check are removed
by
> the compiler as dead code. At least we know that works!

Sorry I did not catch this, fixed now to return in test functions instead of
here.

[gdeepti, 2016-11-22 22:30:37]

The CQ bit was checked by gdeepti@chromium.org

[gdeepti, 2016-11-22 22:30:38]

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/2385393002/#ps100001
(title: "Bill's review")

[commit-bot: I haz the power, 2016-11-22 22:30:50]

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-11-22 22:35:08]

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

[commit-bot: I haz the power, 2016-11-22 22:35:10]

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/29158)

[gdeepti, 2016-11-22 23:27:54]

CC aseemgarg@ as it touches the same files as ScalarLowering CL.

[gdeepti, 2016-11-28 06:16:28]

Ping. Ben, I was hoping you could take a look at this CL, need an owner's review
for src/compiler.

[titzer, 2016-11-28 09:28:37]

https://codereview.chromium.org/2385393002/diff/100001/src/compiler/x64/instr...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2385393002/diff/100001/src/compiler/x64/instr...
src/compiler/x64/instruction-selector-x64.cc:2365:
g.UseRegister(node->InputAt(0)), g.UseImmediate(node->InputAt(1)),
This is going to be brittle. You're assuming that an input is a constant (and
therefore can be an immediate). If it is required to be a compile-time constant,
it should be part of the Operator.

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

https://codereview.chromium.org/2385393002/diff/100001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:66: BUILD(r, WASM_BLOCK(
I think it'd be better if we made this test into a loop over the lane index so
that the test code is very small instead of one giant generated function.

[gdeepti, 2016-12-01 23:59:09]

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

[commit-bot: I haz the power, 2016-12-01 23:59:12]

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-12-02 00:25:52]

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

[commit-bot: I haz the power, 2016-12-02 00:25:53]

Dry run: This issue passed the CQ dry run.

[gdeepti, 2016-12-02 00:43:45]

https://codereview.chromium.org/2385393002/diff/100001/src/compiler/x64/instr...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/2385393002/diff/100001/src/compiler/x64/instr...
src/compiler/x64/instruction-selector-x64.cc:2365:
g.UseRegister(node->InputAt(0)), g.UseImmediate(node->InputAt(1)),
On 2016/11/28 09:28:37, titzer wrote:
> This is going to be brittle. You're assuming that an input is a constant (and
> therefore can be an immediate). If it is required to be a compile-time
constant,
> it should be part of the Operator.

Fixed so that lane values are compile time constants, added ExtractLane,
ReplaceLane operators to common-operator.h/.cc.

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

https://codereview.chromium.org/2385393002/diff/100001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-simd.cc:66: BUILD(r, WASM_BLOCK(
On 2016/11/28 09:28:37, titzer wrote:
> I think it'd be better if we made this test into a loop over the lane index so
> that the test code is very small instead of one giant generated function.

So i tried putting this in a loop, and not sure if i'm doing it right but it
seems to be more complex to remember which positions have been replaced when
checking all the lane values - or did you mean not use the macro to check and
have a nested loop?

[gdeepti, 2016-12-05 06:33:39]

Ping

[titzer, 2016-12-05 09:40:08]

lgtm with nits

https://codereview.chromium.org/2385393002/diff/120001/src/compiler/common-op...
File src/compiler/common-operator.cc (right):

https://codereview.chromium.org/2385393002/diff/120001/src/compiler/common-op...
src/compiler/common-operator.cc:1134: const Operator*
CommonOperatorBuilder::Int32x4ExtractLane(int32_t value) {
Can we call this lane_number?

https://codereview.chromium.org/2385393002/diff/120001/src/compiler/common-op...
src/compiler/common-operator.cc:1135: return new (zone()) Operator1<int32_t>(   
          // --
Can we get a DCHECK that 0 <= lane_number < 4?

[gdeepti, 2016-12-05 23:25:21]

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

[commit-bot: I haz the power, 2016-12-05 23:25:26]

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-12-05 23:54:54]

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

[commit-bot: I haz the power, 2016-12-05 23:54:54]

Dry run: This issue passed the CQ dry run.

[gdeepti, 2016-12-06 00:41:00]

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

[commit-bot: I haz the power, 2016-12-06 00:41: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-12-06 01:09:27]

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

[commit-bot: I haz the power, 2016-12-06 01:09:28]

Dry run: This issue passed the CQ dry run.

[gdeepti, 2016-12-06 01:10:08]

https://codereview.chromium.org/2385393002/diff/120001/src/compiler/common-op...
File src/compiler/common-operator.cc (right):

https://codereview.chromium.org/2385393002/diff/120001/src/compiler/common-op...
src/compiler/common-operator.cc:1134: const Operator*
CommonOperatorBuilder::Int32x4ExtractLane(int32_t value) {
On 2016/12/05 09:40:08, titzer wrote:
> Can we call this lane_number?

Done.

https://codereview.chromium.org/2385393002/diff/120001/src/compiler/common-op...
src/compiler/common-operator.cc:1135: return new (zone()) Operator1<int32_t>(   
          // --
On 2016/12/05 09:40:08, titzer wrote:
> Can we get a DCHECK that 0 <= lane_number < 4?

Done.

[gdeepti, 2016-12-06 01:10:26]

The CQ bit was checked by gdeepti@chromium.org

[gdeepti, 2016-12-06 01:10:26]

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

[commit-bot: I haz the power, 2016-12-06 01:10:37]

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-06 01:12:15]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1480986626255880,
"parent_rev": "ffd58bf713f30d24a4e70bb98feafa9c797598ff", "commit_rev":
"1df4412c3e288dc88c1ebccf01e97972d9c03946"}

[commit-bot: I haz the power, 2016-12-06 01:12:23]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-12-06 01:13:01]

Description was changed from

==========
[wasm] Implement I32x4ReplaceLane, I32x4Add, I32x4Sub.

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

to

==========
[wasm] Implement I32x4ReplaceLane, I32x4Add, I32x4Sub.

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

Committed: https://crrev.com/02c6b04179a632a3d7a493e60086e5f4c5f47f95
Cr-Commit-Position: refs/heads/master@{#41505}
==========

[commit-bot: I haz the power, 2016-12-06 01:13:02]

Patchset 10 (id:??) landed as
https://crrev.com/02c6b04179a632a3d7a493e60086e5f4c5f47f95
Cr-Commit-Position: refs/heads/master@{#41505}