[wasm] Int64Lowering of Int64Add on ia32 and arm.

[wasm] Int64Lowering of Int64Add on ia32 and arm.

Int64Add is lowered to a new turbofan operator, Int32AddPair. The new
operator takes 4 inputs an generates 2 outputs. The inputs are the low
word of the left input, high word of the left input, the low word of the
right input, and high word of the right input. The ouputs are the low
and high word of the result of the addition.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com

Committed: https://crrev.com/1b230799364b7a6fbff3b0ffaee38bc06e19504b
Cr-Commit-Position: refs/heads/master@{#34747}

[jbramley, 2016-03-09 10:40:17]

jacob.bramley@arm.com changed reviewers:
+ jacob.bramley@arm.com

[jbramley, 2016-03-09 10:40:18]

Other than these minor details, the ARM parts look good.

Thanks,
Jacob

https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/code-gener...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/code-gener...
src/compiler/arm/code-generator-arm.cc:791: Operand(i.InputRegister(3)));
To match other ARM instructions that can't set the flags, consider:
  DCHECK_EQ(LeaveCC, i.OutputSBit());

https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/code-gener...
src/compiler/arm/code-generator-arm.cc:791: Operand(i.InputRegister(3)));
Consider:
  DCHECK(i.OutputRegister(0), i.InputRegister(1));
  DCHECK(i.OutputRegister(0), i.InputRegister(3));

https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/instructio...
File src/compiler/arm/instruction-selector-arm.cc (right):

https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/instructio...
src/compiler/arm/instruction-selector-arm.cc:780: g.DefineSameAsFirst(node),
Is DefineSameAsFirst necessary to stop inputs 1 and 3 from being overwritten?
Otherwise, could this just be another DefineAsRegister?

[ahaas, 2016-03-09 17:28:06]

Jacob, PTAL.

https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/code-gener...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/code-gener...
src/compiler/arm/code-generator-arm.cc:791: Operand(i.InputRegister(3)));
On 2016/03/09 at 10:40:17, jbramley wrote:
> To match other ARM instructions that can't set the flags, consider:
>   DCHECK_EQ(LeaveCC, i.OutputSBit());

Done.

> DCHECK(i.OutputRegister(0), i.InputRegister(1));
> DCHECK(i.OutputRegister(0), i.InputRegister(3));

These constraints are not needed anymore.

https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/instructio...
File src/compiler/arm/instruction-selector-arm.cc (right):

https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/instructio...
src/compiler/arm/instruction-selector-arm.cc:780: g.DefineSameAsFirst(node),
On 2016/03/09 at 10:40:17, jbramley wrote:
> Is DefineSameAsFirst necessary to stop inputs 1 and 3 from being overwritten?
Otherwise, could this just be another DefineAsRegister?

I did some experiments with the register allocation now and found out that
DefineSameAsFirst is not sufficient. The only solutions I found was using fixed
registers or a temporary register. I decided for the temporary register. Jacob,
PTAL.

[jbramley, 2016-03-10 11:15:42]

On 2016/03/09 17:28:06, ahaas wrote:
>
https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/instructio...
> File src/compiler/arm/instruction-selector-arm.cc (right):
> 
>
https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/instructio...
> src/compiler/arm/instruction-selector-arm.cc:780: g.DefineSameAsFirst(node),
> On 2016/03/09 at 10:40:17, jbramley wrote:
> > Is DefineSameAsFirst necessary to stop inputs 1 and 3 from being
overwritten?
> Otherwise, could this just be another DefineAsRegister?
> 
> I did some experiments with the register allocation now and found out that
> DefineSameAsFirst is not sufficient. The only solutions I found was using
fixed
> registers or a temporary register. I decided for the temporary register.
Jacob,
> PTAL.

The description of UseUniqueRegister states that it also prevents aliasing with
output registers. Is that correct? If so, you can write directly into
OutputRegister(0) and avoid the temporary (and the extra move).

[ahaas, 2016-03-10 11:45:44]

On 2016/03/10 at 11:15:42, jacob.bramley wrote:
> On 2016/03/09 17:28:06, ahaas wrote:
> >
https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/instructio...
> > File src/compiler/arm/instruction-selector-arm.cc (right):
> > 
> >
https://codereview.chromium.org/1778493004/diff/1/src/compiler/arm/instructio...
> > src/compiler/arm/instruction-selector-arm.cc:780: g.DefineSameAsFirst(node),
> > On 2016/03/09 at 10:40:17, jbramley wrote:
> > > Is DefineSameAsFirst necessary to stop inputs 1 and 3 from being
overwritten?
> > Otherwise, could this just be another DefineAsRegister?
> > 
> > I did some experiments with the register allocation now and found out that
> > DefineSameAsFirst is not sufficient. The only solutions I found was using
fixed
> > registers or a temporary register. I decided for the temporary register.
Jacob,
> > PTAL.
> 
> The description of UseUniqueRegister states that it also prevents aliasing
with output registers. Is that correct? If so, you can write directly into
OutputRegister(0) and avoid the temporary (and the extra move).

You are right. I use UseUniqueRegister now for all inputs and removed the use of
the temporary register. Thanks.

[jbramley, 2016-03-11 09:17:05]

I just have some small nits, but otherwise the ARM parts look good to me.
Thanks!

https://codereview.chromium.org/1778493004/diff/80001/src/compiler/arm/instru...
File src/compiler/arm/instruction-selector-arm.cc (right):

https://codereview.chromium.org/1778493004/diff/80001/src/compiler/arm/instru...
src/compiler/arm/instruction-selector-arm.cc:774: // register.
In the comment: "... with the output registers."

(There isn't a temp any more.)

https://codereview.chromium.org/1778493004/diff/80001/src/compiler/arm/instru...
src/compiler/arm/instruction-selector-arm.cc:778:
g.UseUniqueRegister(node->InputAt(3))};
I don't think inputs 0 and 2 have to be unique, since they're only read in the
first instruction. (I could be mistaken.)

[ahaas, 2016-03-14 11:07:50]

https://codereview.chromium.org/1778493004/diff/80001/src/compiler/arm/instru...
File src/compiler/arm/instruction-selector-arm.cc (right):

https://codereview.chromium.org/1778493004/diff/80001/src/compiler/arm/instru...
src/compiler/arm/instruction-selector-arm.cc:774: // register.
On 2016/03/11 at 09:17:05, jbramley wrote:
> In the comment: "... with the output registers."
> 
> (There isn't a temp any more.)

Done.

https://codereview.chromium.org/1778493004/diff/80001/src/compiler/arm/instru...
src/compiler/arm/instruction-selector-arm.cc:778:
g.UseUniqueRegister(node->InputAt(3))};
On 2016/03/11 at 09:17:05, jbramley wrote:
> I don't think inputs 0 and 2 have to be unique, since they're only read in the
first instruction. (I could be mistaken.)

Done.

[titzer, 2016-03-14 14:50:45]

lgtm other than test rebasing

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

https://codereview.chromium.org/1778493004/diff/100001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-64.cc:23: TEST(Run_WasmI64Add) {
You'll need to rebase and REQUIRE(I64Add)

[ahaas, 2016-03-14 15:15:05]

On 2016/03/14 at 14:50:45, titzer wrote:
> lgtm other than test rebasing
> 
>
https://codereview.chromium.org/1778493004/diff/100001/test/cctest/wasm/test-...
> File test/cctest/wasm/test-run-wasm-64.cc (right):
> 
>
https://codereview.chromium.org/1778493004/diff/100001/test/cctest/wasm/test-...
> test/cctest/wasm/test-run-wasm-64.cc:23: TEST(Run_WasmI64Add) {
> You'll need to rebase and REQUIRE(I64Add)

Done.

[ahaas, 2016-03-14 15:15:16]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-03-14 15:15:17]

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/1778493004/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-14 15:15:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778493004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778493004/120001

[commit-bot: I haz the power, 2016-03-14 15:33:22]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-14 15:34:28]

Description was changed from

==========
[wasm] Int64Lowering of Int64Add on ia32 and arm.

Int64Add is lowered to a new turbofan operator, Int32AddPair. The new
operator takes 4 inputs an generates 2 outputs. The inputs are the low
word of the left input, high word of the left input, the low word of the
right input, and high word of the right input. The ouputs are the low
and high word of the result of the addition.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com
==========

to

==========
[wasm] Int64Lowering of Int64Add on ia32 and arm.

Int64Add is lowered to a new turbofan operator, Int32AddPair. The new
operator takes 4 inputs an generates 2 outputs. The inputs are the low
word of the left input, high word of the left input, the low word of the
right input, and high word of the right input. The ouputs are the low
and high word of the result of the addition.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com

Committed: https://crrev.com/1b230799364b7a6fbff3b0ffaee38bc06e19504b
Cr-Commit-Position: refs/heads/master@{#34747}
==========

[commit-bot: I haz the power, 2016-03-14 15:34:29]

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