First two codegen tests passing on SIMMIPS

First two codegen tests passing on SIMMIPS
Committed: https://code.google.com/p/dart/source/detail?r=20734

[zra, 2013-03-28 21:18:32]

Sorry for the big CL. It is mostly copied from ARM, though, with small fix-ups
for MIPS.

Cheers,
Zach

[regis, 2013-03-28 21:43:25]

First few comments.
I think you should address these, upload, and I'll continue with the review.

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

https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.cc:148: void Assembler::PushList(RegList regs) {
I am not sure why you need this, since it is really ARM-specific.
I understand that it makes it easier to port from ARM to MIPS, but it would be
better to manually generate the code.
The sequence you generate here is not efficient, since you adjust the stack
pointer for each push.

https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.cc:158: void Assembler::PopList(RegList regs) {
ditto

https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.cc:167: static int NumRegsBelowFP(RegList regs) {
I do not think you will need this either if you do not use the above
ARM-specific PushList and PopList (stm/ldm).

https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.cc:189: addiu(FP, SP, Immediate(1 * kWordSize));
The sequence above is not optimal. Do not use Push. Adjust the stack pointer and
then emit stores.

https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.cc:196: Pop(FP);
Do not use Pop. Emit loads and only then adjust the stack pointer.

[zra, 2013-03-28 23:04:32]

On 2013/03/28 21:43:25, regis wrote:
> First few comments.
> I think you should address these, upload, and I'll continue with the review.
> 
>
https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips.cc
> File runtime/vm/assembler_mips.cc (right):
> 
>
https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips....
> runtime/vm/assembler_mips.cc:148: void Assembler::PushList(RegList regs) {
> I am not sure why you need this, since it is really ARM-specific.
> I understand that it makes it easier to port from ARM to MIPS, but it would be
> better to manually generate the code.
> The sequence you generate here is not efficient, since you adjust the stack
> pointer for each push.
> 
>
https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips....
> runtime/vm/assembler_mips.cc:158: void Assembler::PopList(RegList regs) {
> ditto
> 
>
https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips....
> runtime/vm/assembler_mips.cc:167: static int NumRegsBelowFP(RegList regs) {
> I do not think you will need this either if you do not use the above
> ARM-specific PushList and PopList (stm/ldm).
> 
>
https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips....
> runtime/vm/assembler_mips.cc:189: addiu(FP, SP, Immediate(1 * kWordSize));
> The sequence above is not optimal. Do not use Push. Adjust the stack pointer
and
> then emit stores.
> 
>
https://codereview.chromium.org/13228002/diff/8001/runtime/vm/assembler_mips....
> runtime/vm/assembler_mips.cc:196: Pop(FP);
> Do not use Pop. Emit loads and only then adjust the stack pointer.

Thanks. Please have another look. I could also rewrite PushList and PopList to
look like the code in FlowGraphCompiler::SaveLiveRegisters and
FlowGraphCompiler::RestoreLiveRegisters, but maybe doing everything explicitly
is clear enough?

[regis, 2013-03-29 00:23:32]

Good progress! Next round of comments below.
I'll be off tomorrow. See you on Monday. Happy Easter!

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

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:134: mov(TMP, RA);  // Save the return address.
As mentioned, this may not always be necessary.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:140: mov(rd, RA);
Couldn't this mov be in the delay slot?

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:150: mov(RA, ZR);
These moves are not required. No need to emulate ARM instructions.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:155: sw(TMP, Address(SP, 0 * kWordSize));
I am not sure why you store the return address below the frame pointer. I think
this whole EnterStubFrame macro should be:

addiu(SP, SP, Immediate(-3 * kWordSize));
sw(ZR, Address(SP, 2 * kWordSize));  // PC marker is 0 in stubs.
sw(RA, Address(SP, 1 * kWordSize));
sw(FP, Address(SP, 0 * kWordSize));
mov(FP, SP);

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:166: addiu(SP, SP, Immediate(3 * kWordSize));
And this one should be:
mov(SP, FP);
lw(FP, Address(SP, 0 * kWordSize));
lw(RA, Address(SP, 1 * kWordSize));
addiu(SP, SP, Immediate(3 * kWordSize));

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:174: GetPC(T0);
You can get the PC after saving RA in the frame and avoid preserving/restoring
RA.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:194: GetPC(T0);
Not necessary. You still have the PC in T0.

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

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:610: void GetPC(Register rd);
I think it may be clearer to inline this where you need it, because RA may
already be saved.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/disassembler_m...
File runtime/vm/disassembler_mips.cc (right):

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:145: reinterpret_cast<uword>(instr) + off +
Instr::kInstrSize;
Shouldn't it be + 2 * Instr::kInstrSize?

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_mips.cc:210: }
The sequence above is rather expensive. I am not happy with the ARM version
either. Leave it as is for now, but I think we should find a better solution,
maybe by moving this code after the EnterDartFrame. Let's chat next week.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/stub_code_mips.cc
File runtime/vm/stub_code_mips.cc (right):

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/stub_code_mips...
runtime/vm/stub_code_mips.cc:123: __ addiu(SP, SP, Immediate(-2 * kWordSize));
The SP could be adjusted only once above.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/stub_code_mips...
runtime/vm/stub_code_mips.cc:177: __ addiu(SP, SP, Immediate(2 * kWordSize));
ditto

[zra, 2013-03-29 17:10:44]

Thanks! Enjoy your day off. See you Monday.

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

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:134: mov(TMP, RA);  // Save the return address.
On 2013/03/29 00:23:32, regis wrote:
> As mentioned, this may not always be necessary.

I have removed this function and inlined it.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:140: mov(rd, RA);
On 2013/03/29 00:23:32, regis wrote:
> Couldn't this mov be in the delay slot?

Done.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:150: mov(RA, ZR);
On 2013/03/29 00:23:32, regis wrote:
> These moves are not required. No need to emulate ARM instructions.

Done.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:155: sw(TMP, Address(SP, 0 * kWordSize));
On 2013/03/29 00:23:32, regis wrote:
> I am not sure why you store the return address below the frame pointer. I
think
> this whole EnterStubFrame macro should be:
> 
> addiu(SP, SP, Immediate(-3 * kWordSize));
> sw(ZR, Address(SP, 2 * kWordSize));  // PC marker is 0 in stubs.
> sw(RA, Address(SP, 1 * kWordSize));
> sw(FP, Address(SP, 0 * kWordSize));
> mov(FP, SP);

Done.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:166: addiu(SP, SP, Immediate(3 * kWordSize));
On 2013/03/29 00:23:32, regis wrote:
> And this one should be:
> mov(SP, FP);
> lw(FP, Address(SP, 0 * kWordSize));
> lw(RA, Address(SP, 1 * kWordSize));
> addiu(SP, SP, Immediate(3 * kWordSize));

Done.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:174: GetPC(T0);
On 2013/03/29 00:23:32, regis wrote:
> You can get the PC after saving RA in the frame and avoid preserving/restoring
> RA.

Done.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:194: GetPC(T0);
On 2013/03/29 00:23:32, regis wrote:
> Not necessary. You still have the PC in T0.

Done.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/disassembler_m...
File runtime/vm/disassembler_mips.cc (right):

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:145: reinterpret_cast<uword>(instr) + off +
Instr::kInstrSize;
On 2013/03/29 00:23:32, regis wrote:
> Shouldn't it be + 2 * Instr::kInstrSize?

The offset is relative to the instruction after the branch, so I think is should
just be one Instr::kInstrSize.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_mips.cc:210: }
On 2013/03/29 00:23:32, regis wrote:
> The sequence above is rather expensive. I am not happy with the ARM version
> either. Leave it as is for now, but I think we should find a better solution,
> maybe by moving this code after the EnterDartFrame. Let's chat next week. 

I've inlined GetPC here, too. Yah, I guess if we did this after EnterDartFrame,
the right PP would already be loaded.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/stub_code_mips.cc
File runtime/vm/stub_code_mips.cc (right):

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/stub_code_mips...
runtime/vm/stub_code_mips.cc:123: __ addiu(SP, SP, Immediate(-2 * kWordSize));
On 2013/03/29 00:23:32, regis wrote:
> The SP could be adjusted only once above.

Done.

https://codereview.chromium.org/13228002/diff/19001/runtime/vm/stub_code_mips...
runtime/vm/stub_code_mips.cc:177: __ addiu(SP, SP, Immediate(2 * kWordSize));
On 2013/03/29 00:23:32, regis wrote:
> ditto

Done.

[regis, 2013-04-01 16:44:05]

LGTM with one comment.

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

https://codereview.chromium.org/13228002/diff/39001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:180: lw(PP, Address(FP, 2 * kWordSize));
You do not need to load the pc marker, since you have it in T0. You should also
wait before storing T0 until you have adjusted here.

[zra, 2013-04-01 17:02:33]

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

https://codereview.chromium.org/13228002/diff/39001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.cc:180: lw(PP, Address(FP, 2 * kWordSize));
On 2013/04/01 16:44:05, regis wrote:
> You do not need to load the pc marker, since you have it in T0. You should
also
> wait before storing T0 until you have adjusted here.

Done.

[zra, 2013-04-01 17:03:17]

Committed patchset #6 manually as r20734 (presubmit successful).