Fix an undetected Smi overflow on ARM.

Fix an undetected Smi overflow on ARM.
Fix wrongly triggered overflow in optimized code on Intel.
Add a test.
Address a few TODOs.

R=zra@google.com

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

[zra, 2014-05-13 21:54:38]

lgtm

[regis, 2014-05-13 22:14:31]

Thanks!

After doing a bit more cleanup, I realize that the bug exists on ia32 and x64 as
well, but my new test does not unveil it there. Unoptimized code does not take
the faulty path and it is tricky to get the right blend of optimizations.

Let me improve the test and fix the bug on ia32 and x64 as well before
submitting.

[regis, 2014-05-13 22:17:37]

On 2014/05/13 22:14:31, regis wrote:
> Thanks!
> 
> After doing a bit more cleanup, I realize that the bug exists on ia32 and x64
as
> well, but my new test does not unveil it there. Unoptimized code does not take
> the faulty path and it is tricky to get the right blend of optimizations.
> 
> Let me improve the test and fix the bug on ia32 and x64 as well before
> submitting.

Taking back half: ia32 is correct. x64 is wrong.

[regis, 2014-05-14 18:38:57]

There are several reasons preventing the test from unveiling the bug on 64-bit
architectures.

First, LiteralNode::ApplyUnaryOp does not recognize that a negated Mint can
become a Smi. I fixed that. Now, the new test fails on simarm64, without the
overflow fix.

Then, adding and subtracting kMinInt64 on x64 generates the same code which
happily triggers overflow when there is none. This causes deoptimization, which
then generates correct code. I fixed that as well.

PTAL

[zra, 2014-05-14 19:40:55]

lgtm

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm.cc
File runtime/vm/assembler_arm.cc (right):

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2501: if (ShifterOperand::CanHold(value,
&shifter_op)) {
Maybe add a comment that value == kMinInt32 will be handled in this case.

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2526: if (ShifterOperand::CanHold(value,
&shifter_op)) {
ditto

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
File runtime/vm/assembler_arm64.cc (right):

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.cc:710: if (Operand::CanHold(imm, kXRegSizeInBits,
&op) == Operand::Immediate) {
ditto

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.cc:716: } else {
ASSERT(imm != kMinInt64);

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.cc:728: if (Operand::CanHold(imm, kXRegSizeInBits,
&op) == Operand::Immediate) {
ditto

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.cc:734: } else {
ASSERT(imm != kMinInt64);

[regis, 2014-05-14 19:54:52]

Thanks!

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm.cc
File runtime/vm/assembler_arm.cc (right):

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2501: if (ShifterOperand::CanHold(value,
&shifter_op)) {
On 2014/05/14 19:40:55, zra wrote:
> Maybe add a comment that value == kMinInt32 will be handled in this case.

Done.

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm.cc:2526: if (ShifterOperand::CanHold(value,
&shifter_op)) {
On 2014/05/14 19:40:55, zra wrote:
> ditto

Done.

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
File runtime/vm/assembler_arm64.cc (right):

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.cc:710: if (Operand::CanHold(imm, kXRegSizeInBits,
&op) == Operand::Immediate) {
On 2014/05/14 19:40:55, zra wrote:
> ditto

Done.

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.cc:716: } else {
On 2014/05/14 19:40:55, zra wrote:
> ASSERT(imm != kMinInt64);

It would not be a problem here, since we emit an adds, not a subs.

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.cc:728: if (Operand::CanHold(imm, kXRegSizeInBits,
&op) == Operand::Immediate) {
On 2014/05/14 19:40:55, zra wrote:
> ditto

Done.

https://codereview.chromium.org/281823002/diff/40001/runtime/vm/assembler_arm...
runtime/vm/assembler_arm64.cc:734: } else {
On 2014/05/14 19:40:55, zra wrote:
> ASSERT(imm != kMinInt64);

It would not be a problem here, since we emit a subs, not an adds.

[regis, 2014-05-14 19:57:22]

Committed patchset #4 manually as r36186 (presubmit successful).