Implements shifted offset register addressing mode for arm.

Implements shifted offset register addressing mode for arm.
Committed: https://code.google.com/p/dart/source/detail?r=19102

[regis, 2013-02-26 23:39:48]

Excellent!
I only have comments about coding style, which is expected at the beginning.
Please, upload a revised version and let me know when I can have another look.

Thanks,
Regis

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

https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:112: ASSERT(!(shift == LSL && shift_imm == 0));
You will notice that we like redundant parenthesis to make precedence extra
clear:
 ASSERT(!((shift == LSL) && (shift_imm == 0)));

It takes some getting use to...
Why are you forbidding LSL #0? Is it a no-op?
Please, add a comment.

https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:223: { }
The pair of braces cannot stand by themselves on a continuation line. Your
options are:
XXXXXX { }
XXXXXX {
}
XXXXXX
    : YYYYYY { }
XXXXXX
    : YYYYYY {
}
As shown above, the indentation for a continuation line is 4 spaces.
We also like to see the : at the beginning of the continuation line rather than
at the end of the previous line.

  Address(const Address& other)
      : ValueObject(), encoding_(other.encoding_), kind_(other.kind_) { }

https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:227: kind_     = other.kind_;
We generally do not align assignments, unless it really helps with reading (such
as in the enum above). Not necessary here.

https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:243: uint32_t shift_n = 0, Mode am = Offset) {
Instead of shift_t and shift_n, I would use the same names as in ShifterOperand:
Shift shift, uint32_t shift_imm

https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:246: kind_     = ShiftedRegister;
No alignment.

https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:262: enum OffsetKind kind() const { return kind_; }
We generally do not write "enum".

https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm.h#ne...
runtime/vm/assembler_arm.h:266: enum OffsetKind kind_;
ditto

https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm_test.cc
File runtime/vm/assembler_arm_test.cc (right):

https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm_test...
runtime/vm/assembler_arm_test.cc:136: // __ str(R0, Address(SP, (-kWordSize *
32), Address::PreIndex));
// Expressing __str(R0....) as:
...

Otherwise, it looks like you forgot to remove a commented out line :-)

https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm_test...
runtime/vm/assembler_arm_test.cc:142: // __ ldr(R0, Address(SP, (kWordSize *
32), Address::PostIndex));
ditto

[zra, 2013-02-27 00:04:24]

On 2013/02/26 23:39:48, regis wrote:
https://codereview.chromium.org/12321149/diff/1/runtime/vm/assembler_arm.h#ne...
>  ASSERT(!((shift == LSL) && (shift_imm == 0)));
> 
> It takes some getting use to...
> Why are you forbidding LSL #0? Is it a no-op?
> Please, add a comment.

I misunderstood the docs. It turns out this is the only way to use an unshifted
offset contained in a register, so it shouldn't be forbidden.

[zra, 2013-02-27 00:10:06]

Thanks =)

I've made the changes.

Cheers,
Zach

[regis, 2013-02-27 00:21:58]

LGTM!

[zra, 2013-02-27 00:36:02]

Committed patchset #2 manually as r19102 (presubmit successful).