Adds branch instructions and labels to the MIPS simulator, assembler, disassembler.

Adds branch instructions and labels to the MIPS simulator, assembler, disassembler.
Committed: https://code.google.com/p/dart/source/detail?r=20419

[regis, 2013-03-22 19:51:16]

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

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.cc#...
runtime/vm/assembler_mips.cc:40: int32_t Assembler::EncodeBranchOffset(int32_t
offset, int32_t inst) {
inst -> instr

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.cc#...
runtime/vm/assembler_mips.cc:42: ASSERT(Utils::IsInt(16, offset));
Not 16, but 18.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.cc#...
runtime/vm/assembler_mips.cc:51: int Assembler::DecodeBranchOffset(int32_t inst)
{
Do you use this call while assembling instructions? I think this belongs to
class Instr, not Assembler.

inst -> instr

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

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:248: ASSERT(!in_delay_slot_);  // Jump within a
delay slot is not supported.
Either remove this comment or add it consistently below.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:261: // Brach if rs >= 0.
Branch

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:276: // Brach if rs > 0.
Branch

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:291: // Brach if rs <= 0.
Branch

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:306: // Brach if rs < 0.
Branch

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:495: void B(Label* l) {
I would use a lower case 'b', since the MIPS assembler understands it. It is not
really our own macro, and it maps to a single instruction. It is more like a
pseudo instruction than a macro.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:508: void Move(Register rd, Register rs) {
This one is debatable, but this is also a pseudo instruction. We should use
'mov' for readability, since we have movn, movz, movt, etc.. But either way is
fine with me.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:512: void Nop() {
nop?

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:619: static int32_t EncodeBranchOffset(int32_t
offset, int32_t inst);
inst -> instr

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:620: static int DecodeBranchOffset(int32_t inst);
inst -> instr

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

https://codereview.chromium.org/12634030/diff/1/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:1044: UNIMPLEMENTED();
Why not UnimplementedInstruction(instr)?

[zra, 2013-03-22 20:16:01]

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

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.cc#...
runtime/vm/assembler_mips.cc:40: int32_t Assembler::EncodeBranchOffset(int32_t
offset, int32_t inst) {
On 2013/03/22 19:51:16, regis wrote:
> inst -> instr

Done.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.cc#...
runtime/vm/assembler_mips.cc:42: ASSERT(Utils::IsInt(16, offset));
On 2013/03/22 19:51:16, regis wrote:
> Not 16, but 18.

Done.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.cc#...
runtime/vm/assembler_mips.cc:51: int Assembler::DecodeBranchOffset(int32_t inst)
{
On 2013/03/22 19:51:16, regis wrote:
> Do you use this call while assembling instructions? I think this belongs to
> class Instr, not Assembler.
> 
> inst -> instr

It's used in Bind above. This also mirrors the ARM implementation.

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

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:248: ASSERT(!in_delay_slot_);  // Jump within a
delay slot is not supported.
On 2013/03/22 19:51:16, regis wrote:
> Either remove this comment or add it consistently below.

Done.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:261: // Brach if rs >= 0.
On 2013/03/22 19:51:16, regis wrote:
> Branch

Done.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:276: // Brach if rs > 0.
On 2013/03/22 19:51:16, regis wrote:
> Branch

Done.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:291: // Brach if rs <= 0.
On 2013/03/22 19:51:16, regis wrote:
> Branch

Done.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:306: // Brach if rs < 0.
On 2013/03/22 19:51:16, regis wrote:
> Branch

Done.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:495: void B(Label* l) {
On 2013/03/22 19:51:16, regis wrote:
> I would use a lower case 'b', since the MIPS assembler understands it. It is
not
> really our own macro, and it maps to a single instruction. It is more like a
> pseudo instruction than a macro.

That makes sense. I've made it lowercase and moved it up out of the Macros
section.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:508: void Move(Register rd, Register rs) {
On 2013/03/22 19:51:16, regis wrote:
> This one is debatable, but this is also a pseudo instruction. We should use
> 'mov' for readability, since we have movn, movz, movt, etc.. But either way is
> fine with me.

I've made it mov, and moved it up, too, since mov is also how it is
disassembled.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:512: void Nop() {
On 2013/03/22 19:51:16, regis wrote:
> nop?

Same as with mov.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:619: static int32_t EncodeBranchOffset(int32_t
offset, int32_t inst);
On 2013/03/22 19:51:16, regis wrote:
> inst -> instr

Done.

https://codereview.chromium.org/12634030/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:620: static int DecodeBranchOffset(int32_t inst);
On 2013/03/22 19:51:16, regis wrote:
> inst -> instr

Done.

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

https://codereview.chromium.org/12634030/diff/1/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:1044: UNIMPLEMENTED();
On 2013/03/22 19:51:16, regis wrote:
> Why not UnimplementedInstruction(instr)?

Ah right. Thanks. I missed this one.

[regis, 2013-03-22 20:33:12]

LGTM with one comment.

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

https://codereview.chromium.org/12634030/diff/9/runtime/vm/assembler_mips.cc#...
runtime/vm/assembler_mips.cc:36: label->BindTo(bound_pc);
The delay slot should not be used after a Bind:
delay_slot_available_ = false;

[zra, 2013-03-22 20:47:02]

On 2013/03/22 20:33:12, regis wrote:
> LGTM with one comment.
> 
> https://codereview.chromium.org/12634030/diff/9/runtime/vm/assembler_mips.cc
> File runtime/vm/assembler_mips.cc (right):
> 
>
https://codereview.chromium.org/12634030/diff/9/runtime/vm/assembler_mips.cc#...
> runtime/vm/assembler_mips.cc:36: label->BindTo(bound_pc);
> The delay slot should not be used after a Bind:
> delay_slot_available_ = false;

Is this in the spec, or is there some other reason that we should avoid this? I
think it would preclude an instruction in the delay slot being the target of
some other branch. If we won't ever need that, then maybe it makes the assembler
code more readable to forbid it?

[regis, 2013-03-22 21:17:18]

On 2013/03/22 20:47:02, zra wrote:
> On 2013/03/22 20:33:12, regis wrote:
> > LGTM with one comment.
> > 
> > https://codereview.chromium.org/12634030/diff/9/runtime/vm/assembler_mips.cc
> > File runtime/vm/assembler_mips.cc (right):
> > 
> >
>
https://codereview.chromium.org/12634030/diff/9/runtime/vm/assembler_mips.cc#...
> > runtime/vm/assembler_mips.cc:36: label->BindTo(bound_pc);
> > The delay slot should not be used after a Bind:
> > delay_slot_available_ = false;
> 
> Is this in the spec, or is there some other reason that we should avoid this?
I
> think it would preclude an instruction in the delay slot being the target of
> some other branch. If we won't ever need that, then maybe it makes the
assembler
> code more readable to forbid it?

This is not in the spec. This is a limitation of our implementation.
Consider this code:
__ beq(R1, R2, &label);
__ Bind(&other_label);
__ delay_slot()->addiu(R1, R1, Immediate(1));

The label other_label will point to the wrong position, because the nop was
removed from the delay slot.

[zra, 2013-03-22 21:24:37]

Got it. I also fixed an off by one error; the branch offsets are relative to the
instruction *after* the branch.

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

https://codereview.chromium.org/12634030/diff/9/runtime/vm/assembler_mips.cc#...
runtime/vm/assembler_mips.cc:36: label->BindTo(bound_pc);
On 2013/03/22 20:33:12, regis wrote:
> The delay slot should not be used after a Bind:
> delay_slot_available_ = false;

Done.

[zra, 2013-03-22 21:37:15]

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