Fix bug in MIPS BoxInt32 instruction.

Fix bug in MIPS BoxInt32 instruction.

BranchEqual was called with two registers, but generated a compare-immediate
instead.

Add the 2-register version for BranchEqual and BranchNotEqual.

R=vegorov@google.com

Committed: https://code.google.com/p/dart/source/detail?r=40588

[Vyacheslav Egorov (Google), 2014-09-23 11:16:06]

lgtm

[Florian Schneider, 2014-09-23 11:29:55]

Committed patchset #1 (id:1) manually as r40588 (presubmit successful).

[Ivan Posva, 2014-09-23 14:31:42]

iposva@google.com changed reviewers:
+ iposva@google.com, zra@google.com

[Ivan Posva, 2014-09-23 14:31:43]

+zra

We need to evaluate the use of unwrapped scalar values in the macro
instructions.

-Ivan

https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h
File runtime/vm/assembler_mips.h (right):

https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h#...
runtime/vm/assembler_mips.h:935: void BranchEqual(Register rd, int32_t value,
Label* l) {
The real issue is that we are not using Immediate here and a lot of other
similar places.

[Vyacheslav Egorov (Google), 2014-09-23 14:42:31]

https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h
File runtime/vm/assembler_mips.h (right):

https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h#...
runtime/vm/assembler_mips.h:935: void BranchEqual(Register rd, int32_t value,
Label* l) {
On 2014/09/23 14:31:43, Ivan Posva wrote:
> The real issue is that we are not using Immediate here and a lot of other
> similar places.

Alternative interpretation could be: we should make Register itself non
coerciable to int (at least in DEBUG builds)?

This would allow us to catch issues like this at compilation time.

[zra, 2014-09-23 14:57:34]

On 2014/09/23 14:42:31, Vyacheslav Egorov (Google) wrote:
> https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h
> File runtime/vm/assembler_mips.h (right):
> 
>
https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h#...
> runtime/vm/assembler_mips.h:935: void BranchEqual(Register rd, int32_t value,
> Label* l) {
> On 2014/09/23 14:31:43, Ivan Posva wrote:
> > The real issue is that we are not using Immediate here and a lot of other
> > similar places.
> 
> Alternative interpretation could be: we should make Register itself non
> coerciable to int (at least in DEBUG builds)?
> 
> This would allow us to catch issues like this at compilation time.

I've been bitten by this in the arm64 port, as well. I'll go ahead and use
Immediate arguments in the macro instructions in mips and arm64.

[Florian Schneider, 2014-09-23 15:13:43]

On 2014/09/23 14:42:31, Vyacheslav Egorov (Google) wrote:
> https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h
> File runtime/vm/assembler_mips.h (right):
> 
>
https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h#...
> runtime/vm/assembler_mips.h:935: void BranchEqual(Register rd, int32_t value,
> Label* l) {
> On 2014/09/23 14:31:43, Ivan Posva wrote:
> > The real issue is that we are not using Immediate here and a lot of other
> > similar places.
> 
> Alternative interpretation could be: we should make Register itself non
> coerciable to int (at least in DEBUG builds)?
> 
> This would allow us to catch issues like this at compilation time.

+1

[Ivan Posva, 2014-09-23 15:22:42]

On 2014/09/23 14:57:34, zra wrote:
> On 2014/09/23 14:42:31, Vyacheslav Egorov (Google) wrote:
> > https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h
> > File runtime/vm/assembler_mips.h (right):
> > 
> >
>
https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h#...
> > runtime/vm/assembler_mips.h:935: void BranchEqual(Register rd, int32_t
value,
> > Label* l) {
> > On 2014/09/23 14:31:43, Ivan Posva wrote:
> > > The real issue is that we are not using Immediate here and a lot of other
> > > similar places.
> > 
> > Alternative interpretation could be: we should make Register itself non
> > coerciable to int (at least in DEBUG builds)?
> > 
> > This would allow us to catch issues like this at compilation time.
> 
> I've been bitten by this in the arm64 port, as well. I'll go ahead and use
> Immediate arguments in the macro instructions in mips and arm64.

Actually you probably want to verify all assemblers.

-Ivan

[Florian Schneider, 2014-09-23 15:24:26]

On 2014/09/23 14:57:34, zra wrote:
> On 2014/09/23 14:42:31, Vyacheslav Egorov (Google) wrote:
> > https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h
> > File runtime/vm/assembler_mips.h (right):
> > 
> >
>
https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h#...
> > runtime/vm/assembler_mips.h:935: void BranchEqual(Register rd, int32_t
value,
> > Label* l) {
> > On 2014/09/23 14:31:43, Ivan Posva wrote:
> > > The real issue is that we are not using Immediate here and a lot of other
> > > similar places.
> > 
> > Alternative interpretation could be: we should make Register itself non
> > coerciable to int (at least in DEBUG builds)?
> > 
> > This would allow us to catch issues like this at compilation time.
> 
> I've been bitten by this in the arm64 port, as well. I'll go ahead and use
> Immediate arguments in the macro instructions in mips and arm64.

I don't like this solution. This makes the code even more verbose - and you have
to find and change all places where a register in incorrectly accepted as
argument. This is an error-prone process. E.g. you would have to refactor
Address operands as well. LoadFromOffset(Register, Register, int32_t) silently
accepts a Register as third operand which could happen due to a typo.

[Ivan Posva, 2014-09-23 15:26:39]

On 2014/09/23 14:42:31, Vyacheslav Egorov (Google) wrote:
> https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h
> File runtime/vm/assembler_mips.h (right):
> 
>
https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h#...
> runtime/vm/assembler_mips.h:935: void BranchEqual(Register rd, int32_t value,
> Label* l) {
> On 2014/09/23 14:31:43, Ivan Posva wrote:
> > The real issue is that we are not using Immediate here and a lot of other
> > similar places.
> 
> Alternative interpretation could be: we should make Register itself non
> coerciable to int (at least in DEBUG builds)?
> 
> This would allow us to catch issues like this at compilation time.

Maybe you want to do both, but so far we have relied on the fast Register
handling (aka enum) and not passed any scalars. These escaped somehow...

-Ivan

[zra, 2014-09-23 16:14:38]

On 2014/09/23 15:26:39, Ivan Posva wrote:
> On 2014/09/23 14:42:31, Vyacheslav Egorov (Google) wrote:
> > https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h
> > File runtime/vm/assembler_mips.h (right):
> > 
> >
>
https://codereview.chromium.org/595833002/diff/1/runtime/vm/assembler_mips.h#...
> > runtime/vm/assembler_mips.h:935: void BranchEqual(Register rd, int32_t
value,
> > Label* l) {
> > On 2014/09/23 14:31:43, Ivan Posva wrote:
> > > The real issue is that we are not using Immediate here and a lot of other
> > > similar places.
> > 
> > Alternative interpretation could be: we should make Register itself non
> > coerciable to int (at least in DEBUG builds)?
> > 
> > This would allow us to catch issues like this at compilation time.
> 
> Maybe you want to do both, but so far we have relied on the fast Register
> handling (aka enum) and not passed any scalars. These escaped somehow...
> 
> -Ivan

I will start by wrapping the integer arguments to macros that don't have
Immediate in their name. Changing the Register type would be pretty invasive,
I'd think, but if there's an easy way to do it we can go ahead and give it a
try, too.