Fixes bugs in arm and mips intrinsifier.

Fixes bugs in arm and mips intrinsifier.

R=regis@google.com

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

[zra, 2013-06-25 16:13:17]

I haven't run the tests on ARM hardware, yet. The easiest thing might be to let
the buildbot run them, and then fix up the status files.

[regis, 2013-06-25 17:05:16]

Yes, it is OK to let the arm buildbot sort this out.
I have questions about overflow detection, though.

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_mips.cc:3139: __ BranchUnsignedLess(result,
0xC0000000, deopt);
Why not BranchSignedLess?
Don't we check for < 0xC0000000 (a negative number)? For example, 0x0 would be
ok to tag (no deopt).
Also, shouldn't we check for >= 0x40000000, signed as well?

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intrinsifier_ar...
File runtime/vm/intrinsifier_arm.cc (right):

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intrinsifier_ar...
runtime/vm/intrinsifier_arm.cc:876: // Pull off high bits that will be shifted
off of R6 by making a mask
R6 -> R1 here and all occurrences below.

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intrinsifier_ar...
runtime/vm/intrinsifier_arm.cc:888: __ mov(R1, ShifterOperand(R1, LSL, R0));  //
R1 gets the low bits.
That should work better :-)

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intrinsifier_mi...
File runtime/vm/intrinsifier_mips.cc (right):

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intrinsifier_mi...
runtime/vm/intrinsifier_mips.cc:1381: __ BranchUnsignedLess(V0, 0xC0000000,
&fall_through);
Why change this? See my other comment.

[zra, 2013-06-25 17:34:30]

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_mips.cc:3139: __ BranchUnsignedLess(result,
0xC0000000, deopt);
On 2013/06/25 17:05:16, regis wrote:
> Why not BranchSignedLess?
> Don't we check for < 0xC0000000 (a negative number)? For example, 0x0 would be
> ok to tag (no deopt).
> Also, shouldn't we check for >= 0x40000000, signed as well?

Whoops. BranchSignedLess isn't the same as MI on ARM or NEGATIVE on ia32 because
it uses the slt instruction rather than a subtraction. I missed the subtlety in
this code on the first pass, and just assumed I got the signedness of the
comparison wrong. Changing to a subtraction fixes things.

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intrinsifier_ar...
File runtime/vm/intrinsifier_arm.cc (right):

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intrinsifier_ar...
runtime/vm/intrinsifier_arm.cc:876: // Pull off high bits that will be shifted
off of R6 by making a mask
On 2013/06/25 17:05:16, regis wrote:
> R6 -> R1 here and all occurrences below.

Done.

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intrinsifier_mi...
File runtime/vm/intrinsifier_mips.cc (right):

https://codereview.chromium.org/17634003/diff/9001/runtime/vm/intrinsifier_mi...
runtime/vm/intrinsifier_mips.cc:1381: __ BranchUnsignedLess(V0, 0xC0000000,
&fall_through);
On 2013/06/25 17:05:16, regis wrote:
> Why change this? See my other comment.

Done.

[regis, 2013-06-25 18:00:56]

LGTM

It is still not clear to me why we do not check for positive overflow in
Intrinsifier::Double_toInt. It is not done on IA32 either.

[zra, 2013-06-25 18:15:09]

On 2013/06/25 18:00:56, regis wrote:
> LGTM
> 
> It is still not clear to me why we do not check for positive overflow in
> Intrinsifier::Double_toInt. It is not done on IA32 either.

If we have 4-bit arithmetic:

0100 will overflow if we try to tag it.

  0100       4
 -1100   - (-4)
 -----   ------
  1000       8

The result is negative so we'll deopt.

1100 should not overflow when we tag it.

 1100        (-4)
-1100      - (-4)
-----      ------
 0000          0

1000 should overflow

 1000        (-8)
-1100      - (-4)
-----      ------
 1100         -4

So that will overflow, too.

[regis, 2013-06-25 18:51:59]

On 2013/06/25 18:15:09, zra wrote:
> On 2013/06/25 18:00:56, regis wrote:
> > LGTM
> > 
> > It is still not clear to me why we do not check for positive overflow in
> > Intrinsifier::Double_toInt. It is not done on IA32 either.
> 
> If we have 4-bit arithmetic:
> 
> 0100 will overflow if we try to tag it.
> 
>   0100       4
>  -1100   - (-4)
>  -----   ------
>   1000       8
> 
> The result is negative so we'll deopt.
> 
> 1100 should not overflow when we tag it.
> 
>  1100        (-4)
> -1100      - (-4)
> -----      ------
>  0000          0
> 
> 1000 should overflow
> 
>  1000        (-8)
> -1100      - (-4)
> -----      ------
>  1100         -4
> 
> So that will overflow, too.

That is indeed quite subtle. Thanks for the explanation!

[zra, 2013-06-25 18:58:51]

Committed patchset #5 manually as r24427 (presubmit successful).