[wasm] Int64Lowering of Int64Sub on ia32 and arm.

[wasm] Int64Lowering of Int64Sub on ia32 and arm.

Int64Sub is lowered to a new turbofan operator, Int32SubPair. 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 subtraction.

The implementation is very similar to the implementation of Int64Add.

@v8-arm-ports: please take a careful look at the implementation of sbc
in the simulator.

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

Committed: https://crrev.com/33c08596e10f8ed64f8097049ac6c424c0616ee1
Cr-Commit-Position: refs/heads/master@{#34808}

[jbramley, 2016-03-11 15:23:43]

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

[jbramley, 2016-03-11 15:23:43]

https://codereview.chromium.org/1778893005/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/1778893005/diff/1/src/arm/simulator-arm.cc#ne...
src/arm/simulator-arm.cc:1341: (carry && (((uright + 1) > uleft) || (uright >
(uleft - 1))));
This gets the wrong result if left == right.

Carry = 0:
- result = (left - right) - (carry ? 0 : 1) = 0xffffffff
- carry_out should be 0, but this method gives 1.

Carry = 1:
- result = (left - right) - (carry ? 0 : 1) = 0x00000000
- carry_out should be 1, but this method gives 0.

[titzer, 2016-03-14 09:36:06]

https://codereview.chromium.org/1778893005/diff/20001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/1778893005/diff/20001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:2499: alu_out = (rn_val - shifter_operand) -
(GetCarry() ? 0 : 1);
Why is carry inverted here?

https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4200: int64_t ToInt64(uint32_t low,
uint32_t high) {
You have to make this uint64_t to avoid undefined behavior on overflow.

https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4205: 
Stray blank line

https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4280: m.StoreToPointer(&low,
MachineRepresentation::kWord32,
Why do you need to use memory here?

[ahaas, 2016-03-15 11:01:43]

https://codereview.chromium.org/1778893005/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/1778893005/diff/1/src/arm/simulator-arm.cc#ne...
src/arm/simulator-arm.cc:1341: (carry && (((uright + 1) > uleft) || (uright >
(uleft - 1))));
On 2016/03/11 at 15:23:43, jbramley wrote:
> This gets the wrong result if left == right.
> 
> Carry = 0:
> - result = (left - right) - (carry ? 0 : 1) = 0xffffffff
> - carry_out should be 0, but this method gives 1.
> 
> Carry = 1:
> - result = (left - right) - (carry ? 0 : 1) = 0x00000000
> - carry_out should be 1, but this method gives 0.

Fixed. I use the negation of carry in the calculation now. Additionally it was
necessary to change the default value of carry to 1 instead of 0.

https://codereview.chromium.org/1778893005/diff/20001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/1778893005/diff/20001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:2499: alu_out = (rn_val - shifter_operand) -
(GetCarry() ? 0 : 1);
On 2016/03/14 at 09:36:06, titzer wrote:
> Why is carry inverted here?

Because the arm instruction manual says so.

https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4200: int64_t ToInt64(uint32_t low,
uint32_t high) {
On 2016/03/14 at 09:36:06, titzer wrote:
> You have to make this uint64_t to avoid undefined behavior on overflow.

I think this function works as intended. To clarify my intentions I renamed the
function to BitcastToInt64.

https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4205: 
On 2016/03/14 at 09:36:06, titzer wrote:
> Stray blank line

Removed.

https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4280: m.StoreToPointer(&low,
MachineRepresentation::kWord32,
On 2016/03/14 at 09:36:06, titzer wrote:
> Why do you need to use memory here?

The BufferedRawMachineAssemblerTester does not support 64 bit returns on 32 bit
platforms.

[titzer, 2016-03-15 11:05:39]

https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4200: int64_t ToInt64(uint32_t low,
uint32_t high) {
On 2016/03/15 11:01:43, ahaas wrote:
> On 2016/03/14 at 09:36:06, titzer wrote:
> > You have to make this uint64_t to avoid undefined behavior on overflow.
> 
> I think this function works as intended. To clarify my intentions I renamed
the
> function to BitcastToInt64.

Below, you are doing arithmetic on int64_t values without checking for overflow;
this is UB in C++, unfortunately.

[ahaas, 2016-03-15 11:49:45]

On 2016/03/15 at 11:05:39, titzer wrote:
>
https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
> File test/cctest/compiler/test-run-machops.cc (right):
> 
>
https://codereview.chromium.org/1778893005/diff/20001/test/cctest/compiler/te...
> test/cctest/compiler/test-run-machops.cc:4200: int64_t ToInt64(uint32_t low,
uint32_t high) {
> On 2016/03/15 11:01:43, ahaas wrote:
> > On 2016/03/14 at 09:36:06, titzer wrote:
> > > You have to make this uint64_t to avoid undefined behavior on overflow.
> > 
> > I think this function works as intended. To clarify my intentions I renamed
the
> > function to BitcastToInt64.
> 
> Below, you are doing arithmetic on int64_t values without checking for
overflow; this is UB in C++, unfortunately.

I changed everything to uint now.

[jbramley, 2016-03-15 11:52:14]

On 2016/03/15 11:01:43, ahaas wrote:
> https://codereview.chromium.org/1778893005/diff/1/src/arm/simulator-arm.cc
> File src/arm/simulator-arm.cc (right):
> 
>
https://codereview.chromium.org/1778893005/diff/1/src/arm/simulator-arm.cc#ne...
> src/arm/simulator-arm.cc:1341: (carry && (((uright + 1) > uleft) || (uright >
> (uleft - 1))));
> On 2016/03/11 at 15:23:43, jbramley wrote:
> > This gets the wrong result if left == right.
> > 
> > Carry = 0:
> > - result = (left - right) - (carry ? 0 : 1) = 0xffffffff
> > - carry_out should be 0, but this method gives 1.
> > 
> > Carry = 1:
> > - result = (left - right) - (carry ? 0 : 1) = 0x00000000
> > - carry_out should be 1, but this method gives 0.
> 
> Fixed. I use the negation of carry in the calculation now. Additionally it was
> necessary to change the default value of carry to 1 instead of 0.

Thanks!

> https://codereview.chromium.org/1778893005/diff/20001/src/arm/simulator-arm.cc
> File src/arm/simulator-arm.cc (right):
> 
>
https://codereview.chromium.org/1778893005/diff/20001/src/arm/simulator-arm.c...
> src/arm/simulator-arm.cc:2499: alu_out = (rn_val - shifter_operand) -
> (GetCarry() ? 0 : 1);
> On 2016/03/14 at 09:36:06, titzer wrote:
> > Why is carry inverted here?
> 
> Because the arm instruction manual says so.

Indeed. It's not immediately obviously, but carry == ¬borrow (in the context of
SBC).

In the ARM ARM pseudo-code, the operation is phrased more concisely, but it uses
the logical "~", which is awkward to think about in the context of these
arithmetic operations:

ADC: AddWithCarry(rn, rm, Cin);     // rn + rm + Cin
SBC: AddWithCarry(rn, ~rm, Cin);    // rn + ~rm + Cin

Note that:
  rn + ~rm + Cin == rn + (-rm - 1) + Cin
                 == rn - rm - (Cin ? 0 : 1)

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

On 2016/03/15 at 11:52:14, jacob.bramley wrote:
> On 2016/03/15 11:01:43, ahaas wrote:
> > https://codereview.chromium.org/1778893005/diff/1/src/arm/simulator-arm.cc
> > File src/arm/simulator-arm.cc (right):
> > 
> >
https://codereview.chromium.org/1778893005/diff/1/src/arm/simulator-arm.cc#ne...
> > src/arm/simulator-arm.cc:1341: (carry && (((uright + 1) > uleft) || (uright
>
> > (uleft - 1))));
> > On 2016/03/11 at 15:23:43, jbramley wrote:
> > > This gets the wrong result if left == right.
> > > 
> > > Carry = 0:
> > > - result = (left - right) - (carry ? 0 : 1) = 0xffffffff
> > > - carry_out should be 0, but this method gives 1.
> > > 
> > > Carry = 1:
> > > - result = (left - right) - (carry ? 0 : 1) = 0x00000000
> > > - carry_out should be 1, but this method gives 0.
> > 
> > Fixed. I use the negation of carry in the calculation now. Additionally it
was
> > necessary to change the default value of carry to 1 instead of 0.
> 
> Thanks!
> 
> >
https://codereview.chromium.org/1778893005/diff/20001/src/arm/simulator-arm.cc
> > File src/arm/simulator-arm.cc (right):
> > 
> >
https://codereview.chromium.org/1778893005/diff/20001/src/arm/simulator-arm.c...
> > src/arm/simulator-arm.cc:2499: alu_out = (rn_val - shifter_operand) -
> > (GetCarry() ? 0 : 1);
> > On 2016/03/14 at 09:36:06, titzer wrote:
> > > Why is carry inverted here?
> > 
> > Because the arm instruction manual says so.
> 
> Indeed. It's not immediately obviously, but carry == ¬borrow (in the context
of SBC).
> 
> In the ARM ARM pseudo-code, the operation is phrased more concisely, but it
uses the logical "~", which is awkward to think about in the context of these
arithmetic operations:
> 
> ADC: AddWithCarry(rn, rm, Cin);     // rn + rm + Cin
> SBC: AddWithCarry(rn, ~rm, Cin);    // rn + ~rm + Cin
> 
> Note that:
>   rn + ~rm + Cin == rn + (-rm - 1) + Cin
>                  == rn - rm - (Cin ? 0 : 1)

Rebase.

[titzer, 2016-03-16 09:56:06]

lgtm

[ahaas, 2016-03-16 10:02:38]

The CQ bit was checked by ahaas@chromium.org

[commit-bot: I haz the power, 2016-03-16 10:02:45]

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

[commit-bot: I haz the power, 2016-03-16 10:05:16]

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

[commit-bot: I haz the power, 2016-03-16 10:05:18]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/12418)

[ahaas, 2016-03-16 10:20:53]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-03-16 10:20:54]

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/1778893005/#ps140001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-03-16 10:20:59]

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

[commit-bot: I haz the power, 2016-03-16 10:56:36]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-16 10:58:15]

Description was changed from

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

Int64Sub is lowered to a new turbofan operator, Int32SubPair. 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 subtraction.

The implementation is very similar to the implementation of Int64Add.

@v8-arm-ports: please take a careful look at the implementation of sbc
in the simulator.

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

to

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

Int64Sub is lowered to a new turbofan operator, Int32SubPair. 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 subtraction.

The implementation is very similar to the implementation of Int64Add.

@v8-arm-ports: please take a careful look at the implementation of sbc
in the simulator.

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

Committed: https://crrev.com/33c08596e10f8ed64f8097049ac6c424c0616ee1
Cr-Commit-Position: refs/heads/master@{#34808}
==========

[commit-bot: I haz the power, 2016-03-16 10:58:16]

Patchset 8 (id:??) landed as
https://crrev.com/33c08596e10f8ed64f8097049ac6c424c0616ee1
Cr-Commit-Position: refs/heads/master@{#34808}