- Add a skeleton MIPS assembler, disassembler and simulator.

- Add a skeleton MIPS assembler, disassembler and simulator.
- Remove unused fields and methods from ARM simulator.
- Remove unused 5th parameter from Simulator::Call.
Committed: https://code.google.com/p/dart/source/detail?r=19606

[regis, 2013-03-06 21:18:42]

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

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:204: void ori(Register rt, Register rs, const
Immediate& imm) {
Are we accepting negative immediate values? See comments in EmitIType below.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:205: EmitIType(ORI, rs, rt, imm);
The body should be moved to the cc file. Ditto for other functions below.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:208: void jr(Register rs) {
This should be Jr, with a capital letter, because it generates more than one
instruction. Alternatively, the NOP should not be emitted as part of jr, but as
part of a Ret() macro instruction.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:241: // Figures 4-1, 4-2 and 4-3.
What figures are you referring to?

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:247: ASSERT(Utils::IsInt(16, imm_value));
I am not sure this is correct. It depends on what the instruction is passing in
as imm value. I suppose that EmitIType will be used for both sign-extended
values and zero-extended values. Passing -1 as the immediate value would
silently generate a bad instruction without the missing mask below.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:251: imm_value);
imm_value & 0xffff

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:265: ASSERT(Utils::IsInt(5, sa_value));
IsUint()?

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:270: sa_value << kSaShift |
(sa_value & 0x1f) << kSaShift

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips_...
File runtime/vm/assembler_mips_test.cc (right):

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips_...
runtime/vm/assembler_mips_test.cc:20: __ jr(RA);
Either
__ Jr(RA);
or
__ jr(RA);
__ nop();

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/constants_mips.h
File runtime/vm/constants_mips.h (right):

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/constants_mips....
runtime/vm/constants_mips.h:235: MFHI,
= 16
etc...

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/constants_mips....
runtime/vm/constants_mips.h:264: BGEZ,
fill in enum values

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/disassembler_mi...
File runtime/vm/disassembler_mips.cc (right):

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/disassembler_mi...
runtime/vm/disassembler_mips.cc:50: bool delay_slot_;  // Previous instruction
uses a delay slot.
How will you use this info in the disassembler?
I can understand the simulator will need it.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/disassembler_mi...
runtime/vm/disassembler_mips.cc:78: };
For better readability of disassembly, we should use the alias names, sp, fp,
ctx, etc...
See arm disassembler.
Not critical now.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/disassembler_mi...
runtime/vm/disassembler_mips.cc:160: // if (buffer_pos_ < (buffer_size_ - 1)) {
Commented out code?

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

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:144: set_register(instr->RtField(), rs_val |
instr->ImmField());
The immediate field should be zero extended, i.e. you need masking with 0xffff.

[Ivan Posva, 2013-03-07 11:03:22]

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

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:204: void ori(Register rt, Register rs, const
Immediate& imm) {
On 2013/03/06 21:18:42, regis wrote:
> Are we accepting negative immediate values? See comments in EmitIType below.

I fixed the imm parameter of EmitIType to be uint16_t. As you point out the
checking for the correct size needs to be done here. For ori it needs to be
Utils::Uint(16, value), while for others the isntruction set has other
requirements.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:205: EmitIType(ORI, rs, rt, imm);
On 2013/03/06 21:18:42, regis wrote:
> The body should be moved to the cc file. Ditto for other functions below.
There is no reason to prevent inlining this function.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:208: void jr(Register rs) {
On 2013/03/06 21:18:42, regis wrote:
> This should be Jr, with a capital letter, because it generates more than one
> instruction. Alternatively, the NOP should not be emitted as part of jr, but
as
> part of a Ret() macro instruction.

As discussed offline the only sane way to look at jr in light of branch delays
is to consider it being a two-word wide instruction.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:241: // Figures 4-1, 4-2 and 4-3.
On 2013/03/06 21:18:42, regis wrote:
> What figures are you referring to?

Done.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:247: ASSERT(Utils::IsInt(16, imm_value));
On 2013/03/06 21:18:42, regis wrote:
> I am not sure this is correct. It depends on what the instruction is passing
in
> as imm value. I suppose that EmitIType will be used for both sign-extended
> values and zero-extended values. Passing -1 as the immediate value would
> silently generate a bad instruction without the missing mask below.

Fixed. EmitIType expects a uint16_t imm now.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:265: ASSERT(Utils::IsInt(5, sa_value));
On 2013/03/06 21:18:42, regis wrote:
> IsUint()?

Done.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:270: sa_value << kSaShift |
On 2013/03/06 21:18:42, regis wrote:
> (sa_value & 0x1f) << kSaShift

Similar fix to EmitIType. We now pass sa as a uint8_t and assert that only 5
bits are used.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips_...
File runtime/vm/assembler_mips_test.cc (right):

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips_...
runtime/vm/assembler_mips_test.cc:20: __ jr(RA);
On 2013/03/06 21:18:42, regis wrote:
> Either
> __ Jr(RA);
> or
> __ jr(RA);
> __ nop();

Manually tracking branch delay slots in the general code is too error prone.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/constants_mips.h
File runtime/vm/constants_mips.h (right):

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/constants_mips....
runtime/vm/constants_mips.h:235: MFHI,
On 2013/03/06 21:18:42, regis wrote:
> = 16
> etc...

Done. I wasn't sure which one is more readable. I guess it is better to not
leave any questions.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/constants_mips....
runtime/vm/constants_mips.h:264: BGEZ,
On 2013/03/06 21:18:42, regis wrote:
> fill in enum values

Done.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/disassembler_mi...
File runtime/vm/disassembler_mips.cc (right):

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/disassembler_mi...
runtime/vm/disassembler_mips.cc:50: bool delay_slot_;  // Previous instruction
uses a delay slot.
On 2013/03/06 21:18:42, regis wrote:
> How will you use this info in the disassembler?
> I can understand the simulator will need it.

I was toying with the idea of not creating an instruction decoder for every
instruction and then you could track this. Removing for now.

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/disassembler_mi...
runtime/vm/disassembler_mips.cc:160: // if (buffer_pos_ < (buffer_size_ - 1)) {
On 2013/03/06 21:18:42, regis wrote:
> Commented out code?
 Yes, another left over from the experiment with printing delay slot specially.
Sorry.

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

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:144: set_register(instr->RtField(), rs_val |
instr->ImmField());
On 2013/03/06 21:18:42, regis wrote:
> The immediate field should be zero extended, i.e. you need masking with
0xffff.

The result of ImmField() is already unsigned.

[Ivan Posva, 2013-03-07 11:04:39]

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

[regis, 2013-03-07 17:56:56]

LGTM

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

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:241: // Figures 4-1, 4-2 and 4-3.
On 2013/03/07 11:03:22, Ivan Posva wrote:
> On 2013/03/06 21:18:42, regis wrote:
> > What figures are you referring to?
> 
> Done.

Done what?

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/constants_mips.h
File runtime/vm/constants_mips.h (right):

https://codereview.chromium.org/12541003/diff/9002/runtime/vm/constants_mips....
runtime/vm/constants_mips.h:235: MFHI,
On 2013/03/07 11:03:22, Ivan Posva wrote:
> On 2013/03/06 21:18:42, regis wrote:
> > = 16
> > etc...
> 
> Done. I wasn't sure which one is more readable. I guess it is better to not
> leave any questions.

It looks like you forgot to upload before committing.