Adds Stop to MIPS. Starts on MIPS call patcher.

Adds Stop to MIPS. Starts on MIPS call patcher.
Committed: https://code.google.com/p/dart/source/detail?r=20527

[zra, 2013-03-25 22:39:16]

I've tried to follow the ARM stuff as much as possible, but in the call patcher,
I used the Instr class because it seemed easier to write.

[regis, 2013-03-25 23:03:40]

I think you need to sync again with ARM.
However, Srdjan pointed out that we never need to decode the loading of the
arguments descriptor, only of the IcData. Therefore, I should simplify
CallPattern before you sync again.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips.cc
File runtime/vm/assembler_mips.cc (right):

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:78: object_pool_ = GrowableObjectArray::New();
You should sync. You will notice that we allocate the array in the old space on
ARM.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:85: object_pool_.Add(obj);
ditto

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:94: object_pool_ = GrowableObjectArray::New();
ditto

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:102: object_pool_.Add(smi);
ditto

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

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:514: lui(TMP, Immediate((label->address() >> 16) &
0xffff));
It would be a bit more readable to use Utils::Low16Bits and Utils::High16Bits
here and below, as we do on ARM.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:574: void Stop(const char* message) {
I would not implement this in the header.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:575: Label stop;
You are missing support for FLAG_print_stop_message.
You should change the default for the flag to false, with a TODO, and add an
UNIMPLEMENTED here if the flag is true.
Otherwise, you are going to forget to support this flag.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/instructions_m...
File runtime/vm/instructions_mips.cc (right):

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/instructions_m...
runtime/vm/instructions_mips.cc:43: if (instr->OpcodeField() == LW &&
instr->RsField() == PP) {
You are missing parenthesis pairs here and below.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/instructions_m...
File runtime/vm/instructions_mips.h (right):

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/instructions_m...
runtime/vm/instructions_mips.h:30: int DecodeLoadWordFromPool(int end, Register*
reg, int* index);
You need to sync. This has changed on ARM.

[zra, 2013-03-25 23:39:17]

Waiting to sync up with ARM =)

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips.cc
File runtime/vm/assembler_mips.cc (right):

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:78: object_pool_ = GrowableObjectArray::New();
On 2013/03/25 23:03:40, regis wrote:
> You should sync. You will notice that we allocate the array in the old space
on
> ARM.

Done.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:85: object_pool_.Add(obj);
On 2013/03/25 23:03:40, regis wrote:
> ditto

Done.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:94: object_pool_ = GrowableObjectArray::New();
On 2013/03/25 23:03:40, regis wrote:
> ditto

Done.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:102: object_pool_.Add(smi);
On 2013/03/25 23:03:40, regis wrote:
> ditto

Done.

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

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:514: lui(TMP, Immediate((label->address() >> 16) &
0xffff));
On 2013/03/25 23:03:40, regis wrote:
> It would be a bit more readable to use Utils::Low16Bits and Utils::High16Bits
> here and below, as we do on ARM.

Okay. So you've got to cast the result of High16Bits to (u)int16_t or you get
unwanted sign-extension because it returns an int32_t. Maybe {High,Low}16Bits
should just return a uint16_t?

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:574: void Stop(const char* message) {
On 2013/03/25 23:03:40, regis wrote:
> I would not implement this in the header.

Done.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:575: Label stop;
On 2013/03/25 23:03:40, regis wrote:
> You are missing support for FLAG_print_stop_message.
> You should change the default for the flag to false, with a TODO, and add an
> UNIMPLEMENTED here if the flag is true.
> Otherwise, you are going to forget to support this flag.

Done.

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/instructions_m...
File runtime/vm/instructions_mips.cc (right):

https://codereview.chromium.org/12703028/diff/14001/runtime/vm/instructions_m...
runtime/vm/instructions_mips.cc:43: if (instr->OpcodeField() == LW &&
instr->RsField() == PP) {
On 2013/03/25 23:03:40, regis wrote:
> You are missing parenthesis pairs here and below.

Done.

[regis, 2013-03-26 00:49:06]

LGTM

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/assembler_mips.cc
File runtime/vm/assembler_mips.cc (right):

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:67: lw(rd, Address(rd));
I think you can do slightly better here, by using the offset in the last
instruction. This could sometimes save one instruction. You can call
LoadImmediate with offset - offset_lo, with offset_lo used in the last
instruction. See ARM for similar code, but remember that the offset is unsigned
on ARM.

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

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:516: uint16_t high =
Utils::High16Bits(label->address());
You could add const.

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:561: uint16_t high = Utils::High16Bits(value);
const, or just inline below.

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:585: // Subtracts rn from location of object and
places result in rd.
I do not understand what you mean by 'location'. How about: set register rd to
zero if the object is equal to register rn, set to non-zero otherwise.

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/instructions_m...
File runtime/vm/instructions_mips.cc (right):

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/instructions_m...
runtime/vm/instructions_mips.cc:58: offset = instr->SImmField();
Shouldn't it be offset |= ?

[zra, 2013-03-26 17:29:07]

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/assembler_mips.cc
File runtime/vm/assembler_mips.cc (right):

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:67: lw(rd, Address(rd));
On 2013/03/26 00:49:06, regis wrote:
> I think you can do slightly better here, by using the offset in the last
> instruction. This could sometimes save one instruction. You can call
> LoadImmediate with offset - offset_lo, with offset_lo used in the last
> instruction. See ARM for similar code, but remember that the offset is
unsigned
> on ARM.

Done.

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

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:516: uint16_t high =
Utils::High16Bits(label->address());
On 2013/03/26 00:49:06, regis wrote:
> You could add const.

Done.

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:561: uint16_t high = Utils::High16Bits(value);
On 2013/03/26 00:49:06, regis wrote:
> const, or just inline below.

Done.

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:585: // Subtracts rn from location of object and
places result in rd.
On 2013/03/26 00:49:06, regis wrote:
> I do not understand what you mean by 'location'. How about: set register rd to
> zero if the object is equal to register rn, set to non-zero otherwise.

Done.

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/instructions_m...
File runtime/vm/instructions_mips.cc (right):

https://codereview.chromium.org/12703028/diff/24001/runtime/vm/instructions_m...
runtime/vm/instructions_mips.cc:58: offset = instr->SImmField();
On 2013/03/26 00:49:06, regis wrote:
> Shouldn't it be offset |= ?

Fixed-up to match LoadWordFromPoolOffset

[zra, 2013-03-26 17:50:49]

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

[regis, 2013-03-26 17:52:35]

LGTM, with a few comments.
Please, have a look at the suggested simpler code and try it.

Thanks,
Regis

https://codereview.chromium.org/12703028/diff/18003/runtime/vm/assembler_mips.cc
File runtime/vm/assembler_mips.cc (right):

https://codereview.chromium.org/12703028/diff/18003/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:32: // Reletive destination from an instruction
after the branch.
Relative

https://codereview.chromium.org/12703028/diff/18003/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:69: if (Address::CanHoldOffset(offset_low)) {
You need this test, because you chopped off 16 bits of the offset (unsigned),
and the lw instruction takes a signed offset.
Could you subtract the signed 16 lower bits of the 32-bit offset instead?
Something like this:

const int16_t offset_low = Utils::Low16Bits(offset);  // Signed.
offset -= offset_low;
const uint16_t offset_high = Utils::High16Bits(offset);  // Unsigned.
if (offset_high != 0) {
  lui(rd, Immediate(offset_high));
  addu(rd, rd, PP);
  lw(rd, Address(rd, offset_low));
} else {
  lw(rd, Address(PP, offset_low));
}

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

https://codereview.chromium.org/12703028/diff/18003/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:675: // Reletive destination from an instruction
after the branch.
ditto

https://codereview.chromium.org/12703028/diff/18003/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:675: // Reletive destination from an instruction
after the branch.
ditto

https://codereview.chromium.org/12703028/diff/18003/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:689: // Reletive destination from an instruction
after the branch.
ditto