MIPS: Use BOVC/BNVC for overflow checking on r6.

MIPS: Use BOVC/BNVC for overflow checking on r6.

BUG=

Committed: https://crrev.com/21b331e3ff9f4da302d31627db7ca265adf1a767
Cr-Commit-Position: refs/heads/master@{#32630}

[paul.l..., 2015-11-13 02:16:20]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[paul.l..., 2015-11-13 02:16:20]

First round of comments. More to come ...

https://codereview.chromium.org/1434263003/diff/1/src/compiler/mips/code-gene...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/1434263003/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:587: // Noop, AdduAndCheckForOverflow()
executed at branch/result processing.
Other similar opcodes use:
// Pseudo-instruction used for tst/branch. No opcode emitted here.
// Pseudo-instruction used for cmp/branch. No opcode emitted here.
... etc.

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
File src/mips/macro-assembler-mips.cc (right):

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.cc:4333: if (no_overflow_label)
bc(no_overflow_label);
This is not r6, you cannot use 'bc' here can you?

I think you have to use Branch(), (which I believe would emit bc() on r6 if no
delay-slot usage, you;ve already tested for r6 above, and we are not there).

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
File src/mips/macro-assembler-mips.h (right):

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.h:1193: BranchDelaySlot bd = PROTECT);
I don't see any existing usages of BranchDelaySlot in the BranchOn[No]Overflow.
So ;et's get rid of it. (We can't implement it properly with compact branches
anyway.)

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.h:1201: void SubuAndCheckForOverflow(Register dst,
I'm assuming that you will refactor the SubuAndCheck... tfor consistency, even
though we do not have the equivalent of BOVC/BNVC for subtract....)

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.h:1211: void BranchOnOverflow(Label* label,
Once you refactor SubuAnd...(), please remove all these BranchOn[No]Overlfow.

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.h:1223: void RetOnOverflow(Register
overflow_check, BranchDelaySlot bd = PROTECT) {
Looks like we have no users of these RetOn... ops. Please remove them.

[Alan Li, 2015-11-20 01:28:39]

alan.li@imgtec.com changed reviewers:
+ alan.li@imgtec.com

[Alan Li, 2015-11-20 01:28:39]

https://codereview.chromium.org/1434263003/diff/1/src/compiler/mips/code-gene...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/1434263003/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:1124: __ li(result, 1);
So I guess another very easy way to detect unsigned overflow addition and set
the result bit is like this:

Consider two unsigned 32bit values A, and B, and we do an unsigned add on both,
resulting value C. then C value is:

C = A + B

And the actual value C' that will be stored in the register is:

C' = (A + B) mod 2 ^32, which is: C' + 2^32 = A + B, since we know C' is greater
than 2^32

since that both A and B are less than 2^32, we will have:

C' + 2^32 = A + B < A + 2^32

So we will have the following:

C' < A and C' < B

So the easiest way to test whether there is an overflow, is to see if the
unsigned result C' is less than either operand A and B.

So we might be able to shrink the whole AdduAndCheckForOverflow, to be:

Addu(i.OutputRegister(), i.InputRegister(0), i.InputOperand(1))
sltu(result, i.OutputRegister(), i.OutputRegister())

[balazs.kilvady, 2015-11-20 19:23:20]

Patchset #2 (id:20001) has been deleted

[balazs.kilvady, 2015-11-20 19:28:08]

https://codereview.chromium.org/1434263003/diff/1/src/compiler/mips/code-gene...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/1434263003/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:587: // Noop, AdduAndCheckForOverflow()
executed at branch/result processing.
On 2015/11/13 02:16:20, paul.l... wrote:
> Other similar opcodes use:
> // Pseudo-instruction used for tst/branch. No opcode emitted here.
> // Pseudo-instruction used for cmp/branch. No opcode emitted here.
> ... etc.

Done.

https://codereview.chromium.org/1434263003/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:1124: __ li(result, 1);
On 2015/11/20 01:28:39, Alan Li wrote:
> So I guess another very easy way to detect unsigned overflow addition and set
> the result bit is like this:
> 
> Consider two unsigned 32bit values A, and B, and we do an unsigned add on
both,
> resulting value C. then C value is:
> 
> C = A + B
> 
> And the actual value C' that will be stored in the register is:
> 
> C' = (A + B) mod 2 ^32, which is: C' + 2^32 = A + B, since we know C' is
greater
> than 2^32
> 
> since that both A and B are less than 2^32, we will have:
> 
> C' + 2^32 = A + B < A + 2^32
> 
> So we will have the following:
> 
> C' < A and C' < B
> 
> So the easiest way to test whether there is an overflow, is to see if the
> unsigned result C' is less than either operand A and B.
> 
> So we might be able to shrink the whole AdduAndCheckForOverflow, to be:
> 
> Addu(i.OutputRegister(), i.InputRegister(0), i.InputOperand(1))
> sltu(result, i.OutputRegister(), i.OutputRegister())
> 
> 

i.OutputRegister() is never (unsigned) less then itself so result is always zero
in sltu(result, i.OutputRegister(), i.OutputRegister());

I think if we compare with both operands then got something like
sltu(result, i.OutputRegister(), i.InputRegister(0));
Branch(&no_overflow , ne, result, zero_reg);
sltu(result, i.OutputRegister(), i.InputRegister(1));
Branch(&no_overflow , ne, result, zero_reg);

So we would have more instructions than currently.
And addu performs signed addition on MIPS.

But please do a followup CL and if that performs well then you can replace those
codes with your CL. But also do that CL following the final version of this one
(hopefully I upload that today) beacuse add/subOverflow functions will be
slighly changed.

Also please put these ideas to the github issue page or slack beacuse it is
easier to discuss ideas there.

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
File src/mips/macro-assembler-mips.h (right):

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.h:1193: BranchDelaySlot bd = PROTECT);
On 2015/11/13 02:16:20, paul.l... wrote:
> I don't see any existing usages of BranchDelaySlot in the
BranchOn[No]Overflow.
> So ;et's get rid of it. (We can't implement it properly with compact branches
> anyway.)

Done.

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.h:1201: void SubuAndCheckForOverflow(Register dst,
On 2015/11/13 02:16:20, paul.l... wrote:
> I'm assuming that you will refactor the SubuAndCheck... tfor consistency, even
> though we do not have the equivalent of BOVC/BNVC for subtract....)
I thought we could negate one of the arguments of subu and then we could use
bovc/bnvc but in that case MIN_INT would cause one more overflow :) and checking
it is expensive. I am thinking about it.

https://codereview.chromium.org/1434263003/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.h:1223: void RetOnOverflow(Register
overflow_check, BranchDelaySlot bd = PROTECT) {
On 2015/11/13 02:16:20, paul.l... wrote:
> Looks like we have no users of these RetOn... ops. Please remove them.

Done.

[balazs.kilvady, 2015-11-20 19:29:42]

The CQ bit was checked by balazs.kilvady@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-20 19:29:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1434263003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1434263003/40001

[commit-bot: I haz the power, 2015-11-20 19:51:06]

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

[commit-bot: I haz the power, 2015-11-20 19:51:07]

Dry run: This issue passed the CQ dry run.

[balazs.kilvady, 2015-11-21 12:32:47]

Description was changed from

==========
MIPS: Use BOVC/BNVC for overflow checking on r6.

BUG=
==========

to

==========
MIPS: Use BOVC/BNVC for overflow checking on r6.

BUG=
==========

[balazs.kilvady, 2015-11-21 12:32:47]

balazs.kilvady@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com

[balazs.kilvady, 2015-11-30 17:35:58]

The CQ bit was checked by balazs.kilvady@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-30 17:36:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1434263003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1434263003/60001

[commit-bot: I haz the power, 2015-11-30 18:02:25]

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

[commit-bot: I haz the power, 2015-11-30 18:02:25]

Dry run: This issue passed the CQ dry run.

[balazs.kilvady, 2015-12-01 11:59:07]

The CQ bit was checked by balazs.kilvady@imgtec.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-01 11:59:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1434263003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1434263003/60001

[commit-bot: I haz the power, 2015-12-01 12:00:56]

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

[commit-bot: I haz the power, 2015-12-01 12:00:56]

Dry run: This issue passed the CQ dry run.

[paul.l..., 2015-12-04 20:04:12]

LGTM, but one nit to fix before landing.

https://codereview.chromium.org/1434263003/diff/60001/src/full-codegen/mips/f...
File src/full-codegen/mips/full-codegen-mips.cc (right):

https://codereview.chromium.org/1434263003/diff/60001/src/full-codegen/mips/f...
src/full-codegen/mips/full-codegen-mips.cc:2304: __ BranchOnOverflow(&stub_call,
scratch1);
No need for this macro-instr anymore, handled in previous instr.

[balazs.kilvady, 2015-12-05 11:58:44]

https://codereview.chromium.org/1434263003/diff/60001/src/full-codegen/mips/f...
File src/full-codegen/mips/full-codegen-mips.cc (right):

https://codereview.chromium.org/1434263003/diff/60001/src/full-codegen/mips/f...
src/full-codegen/mips/full-codegen-mips.cc:2304: __ BranchOnOverflow(&stub_call,
scratch1);
On 2015/12/04 20:04:12, paul.l... wrote:
> No need for this macro-instr anymore, handled in previous instr.
Thanks for catching it.
Done.

[balazs.kilvady, 2015-12-05 11:58:51]

The CQ bit was checked by balazs.kilvady@imgtec.com

[balazs.kilvady, 2015-12-05 11:58:51]

The patchset sent to the CQ was uploaded after l-g-t-m from paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1434263003/#ps80001
(title: "Remove unnecessary BranchOnOverflow() call.")

[commit-bot: I haz the power, 2015-12-05 11:59:01]

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

[commit-bot: I haz the power, 2015-12-05 12:18:34]

Description was changed from

==========
MIPS: Use BOVC/BNVC for overflow checking on r6.

BUG=
==========

to

==========
MIPS: Use BOVC/BNVC for overflow checking on r6.

BUG=
==========

[commit-bot: I haz the power, 2015-12-05 12:18:35]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-12-05 12:18:58]

Description was changed from

==========
MIPS: Use BOVC/BNVC for overflow checking on r6.

BUG=
==========

to

==========
MIPS: Use BOVC/BNVC for overflow checking on r6.

BUG=

Committed: https://crrev.com/21b331e3ff9f4da302d31627db7ca265adf1a767
Cr-Commit-Position: refs/heads/master@{#32630}
==========

[commit-bot: I haz the power, 2015-12-05 12:18:58]

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