Adds MIPS SPECIAL instructions to simulator, assembler, debugger.

Adds MIPS SPECIAL instructions to simulator, assembler, debugger.
Committed: https://code.google.com/p/dart/source/detail?r=20321

[zra, 2013-03-15 23:15:39]

Apologies for doing so many instructions in the same CL, but hopefully they are
pretty straightforward. Also, there's no hurry.

[Ivan Posva, 2013-03-20 01:54:51]

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

https://codereview.chromium.org/12837009/diff/8/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:274: Emit(Instr::kNopInstruction);  // Branch delay
NOP.
How about EmitBranchDelayNop() which expands to:
Emit(Instr::kNopInstruction);
delay_slot_available_ = true;

https://codereview.chromium.org/12837009/diff/8/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:276: }
jalr(Register rs, Register rd)

https://codereview.chromium.org/12837009/diff/8/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:415: void Mov(Register rd, Register rs) {
Move

https://codereview.chromium.org/12837009/diff/8/runtime/vm/assembler_mips_tes...
File runtime/vm/assembler_mips_test.cc (right):

https://codereview.chromium.org/12837009/diff/8/runtime/vm/assembler_mips_tes...
runtime/vm/assembler_mips_test.cc:651: __ jalr(R2);
Why are you not using RA here?

https://codereview.chromium.org/12837009/diff/8/runtime/vm/disassembler_mips.cc
File runtime/vm/disassembler_mips.cc (right):

https://codereview.chromium.org/12837009/diff/8/runtime/vm/disassembler_mips....
runtime/vm/disassembler_mips.cc:270: Format(instr, "mov 'rd, 'rt");
Is the expectation that if it is "or RD, R0, R0" that we print "mov RD, R0", or
should we print "mov RD, 0"? That would be closer to "addi RT, RS, 1".

https://codereview.chromium.org/12837009/diff/8/runtime/vm/disassembler_mips....
runtime/vm/disassembler_mips.cc:301: Format(instr, "sra 'rd, 'rt, 'sa");
Here and others: The SRA instruction expects the RS field to be 0. We should
either assert or have some other safeguard against bad decodings.

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

https://codereview.chromium.org/12837009/diff/8/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:761: ASSERT(instr->RtField() == R0);
I would expect some of these asserts also in the disassembler.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:874: int32_t rs_val =
get_register(instr->RsField());
SLLV takes only bits 0..4 from RS. Please add an appropriate test. As far as I
remember C++ shifting with a value greater than 32 in this case is undefined
behavior.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:889: uint32_t rs_val =
get_register(instr->RsField());
Should this include a static_cast<uint32_t> to clarify?

https://codereview.chromium.org/12837009/diff/8/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:906: int32_t rs_val =
get_register(instr->RsField());
ditto.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:922: uint32_t rs_val =
get_register(instr->RsField());
ditto.

[zra, 2013-03-20 15:53:50]

Thanks!

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

https://codereview.chromium.org/12837009/diff/8/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:274: Emit(Instr::kNopInstruction);  // Branch delay
NOP.
On 2013/03/20 01:54:51, Ivan Posva wrote:
> How about EmitBranchDelayNop() which expands to:
> Emit(Instr::kNopInstruction);
> delay_slot_available_ = true;

Done.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:276: }
On 2013/03/20 01:54:51, Ivan Posva wrote:
> jalr(Register rs, Register rd)

Done.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:415: void Mov(Register rd, Register rs) {
On 2013/03/20 01:54:51, Ivan Posva wrote:
> Move

Done.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/assembler_mips_tes...
File runtime/vm/assembler_mips_test.cc (right):

https://codereview.chromium.org/12837009/diff/8/runtime/vm/assembler_mips_tes...
runtime/vm/assembler_mips_test.cc:651: __ jalr(R2);
On 2013/03/20 01:54:51, Ivan Posva wrote:
> Why are you not using RA here?

For jalr, rs and rd can't be the same. So, you can't say jalr(RA, RA), for
example. So, I had jalr taking only one register, and always storing the return
address in RA. Now, I have it taking two, and asserting in the assembler that rs
!= rd, which probably more clear.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/disassembler_mips.cc
File runtime/vm/disassembler_mips.cc (right):

https://codereview.chromium.org/12837009/diff/8/runtime/vm/disassembler_mips....
runtime/vm/disassembler_mips.cc:270: Format(instr, "mov 'rd, 'rt");
On 2013/03/20 01:54:51, Ivan Posva wrote:
> Is the expectation that if it is "or RD, R0, R0" that we print "mov RD, R0",
or
> should we print "mov RD, 0"? That would be closer to "addi RT, RS, 1".

Done.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/disassembler_mips....
runtime/vm/disassembler_mips.cc:301: Format(instr, "sra 'rd, 'rt, 'sa");
On 2013/03/20 01:54:51, Ivan Posva wrote:
> Here and others: The SRA instruction expects the RS field to be 0. We should
> either assert or have some other safeguard against bad decodings.

In the assembler, sra() passes 0 for rs to EmitRType, and the simulator asserts
that the RS field is 0. We can't put that assert into the disassembler because
dying while decoding an instruction would cause the SimulatorDebugger to die in
the middle of a debugging session (e.g. if you disasm  something that isn't
really instructions).

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

https://codereview.chromium.org/12837009/diff/8/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:761: ASSERT(instr->RtField() == R0);
On 2013/03/20 01:54:51, Ivan Posva wrote:
> I would expect some of these asserts also in the disassembler.

See explanation for sra.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:874: int32_t rs_val =
get_register(instr->RsField());
On 2013/03/20 01:54:51, Ivan Posva wrote:
> SLLV takes only bits 0..4 from RS. Please add an appropriate test. As far as I
> remember C++ shifting with a value greater than 32 in this case is undefined
> behavior.

Done.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:889: uint32_t rs_val =
get_register(instr->RsField());
On 2013/03/20 01:54:51, Ivan Posva wrote:
> Should this include a static_cast<uint32_t> to clarify?

Done.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:906: int32_t rs_val =
get_register(instr->RsField());
On 2013/03/20 01:54:51, Ivan Posva wrote:
> ditto.

Done.

https://codereview.chromium.org/12837009/diff/8/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:922: uint32_t rs_val =
get_register(instr->RsField());
On 2013/03/20 01:54:51, Ivan Posva wrote:
> ditto.

Done.

[Ivan Posva, 2013-03-21 00:08:06]

LGTM -ip

https://codereview.chromium.org/12837009/diff/8/runtime/vm/disassembler_mips.cc
File runtime/vm/disassembler_mips.cc (right):

https://codereview.chromium.org/12837009/diff/8/runtime/vm/disassembler_mips....
runtime/vm/disassembler_mips.cc:301: Format(instr, "sra 'rd, 'rt, 'sa");
On 2013/03/20 15:53:50, zra wrote:
> On 2013/03/20 01:54:51, Ivan Posva wrote:
> > Here and others: The SRA instruction expects the RS field to be 0. We should
> > either assert or have some other safeguard against bad decodings.
> 
> In the assembler, sra() passes 0 for rs to EmitRType, and the simulator
asserts
> that the RS field is 0. We can't put that assert into the disassembler because
> dying while decoding an instruction would cause the SimulatorDebugger to die
in
> the middle of a debugging session (e.g. if you disasm  something that isn't
> really instructions).

You could do Unkown(instr) like you do in SRL.

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

https://codereview.chromium.org/12837009/diff/13001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:271: void jalr(Register rs, Register rd) {
If you do this:
void jalr(Register rs, Register rd = RA)

Then you can have your cake and eat it too.

https://codereview.chromium.org/12837009/diff/13001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:418: void EmitBranchDelayNop() {
I expect this to be private:.

https://codereview.chromium.org/12837009/diff/13001/runtime/vm/assembler_mips...
File runtime/vm/assembler_mips_test.cc (right):

https://codereview.chromium.org/12837009/diff/13001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips_test.cc:650: __ Mov(R2, RA);
__ Mov will not compile.

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

https://codereview.chromium.org/12837009/diff/13001/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:270: Format(instr, "mov 'rd 0");
'rd, 0

[zra, 2013-03-21 15:49:08]

Thanks!

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

https://codereview.chromium.org/12837009/diff/13001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:271: void jalr(Register rs, Register rd) {
On 2013/03/21 00:08:06, Ivan Posva wrote:
> If you do this:
> void jalr(Register rs, Register rd = RA)
> 
> Then you can have your cake and eat it too.

Done.

https://codereview.chromium.org/12837009/diff/13001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:418: void EmitBranchDelayNop() {
On 2013/03/21 00:08:06, Ivan Posva wrote:
> I expect this to be private:.

Done.

https://codereview.chromium.org/12837009/diff/13001/runtime/vm/assembler_mips...
File runtime/vm/assembler_mips_test.cc (right):

https://codereview.chromium.org/12837009/diff/13001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips_test.cc:650: __ Mov(R2, RA);
On 2013/03/21 00:08:06, Ivan Posva wrote:
> __ Mov will not compile.

Whoops! Still getting the hang of all this =)

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

https://codereview.chromium.org/12837009/diff/13001/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:270: Format(instr, "mov 'rd 0");
On 2013/03/21 00:08:06, Ivan Posva wrote:
> 'rd, 0

Done.

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

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