Adds MIPS instructions to simulator, assembler, disassembler

Adds MIPS instructions to simulator, assembler, disassembler

ins - bitfield insert
ext - bitfield extract
l{b,bu,h,hu,w} - load byte, unsigned byte, etc.
s{b,h,w} - store byte, halfword, word
Committed: https://code.google.com/p/dart/source/detail?r=19819

[Ivan Posva, 2013-03-11 17:29:04]

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

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:270: EmitRType(SPECIAL3, rs, rt,
static_cast<Register>(size - 1), pos, EXT);
Maybe we should have a EmitRType which takes a int32_t instead of a Register in
the 3rd register slot?

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:282: void lb(Register rt, const Immediate& off,
Register base) {
The correct parameters here should be
void lb(Register rt, const Address& addr)
For an example see ldr in assembler_arm.h.

Don't you think?

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:339: void sb(Register rt, const Immediate& off,
Register base) {
ditto.

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

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips_...
runtime/vm/assembler_mips_test.cc:252: EXPECT_EQ(static_cast<int32_t>(0xff),
Try getting only a part of the set bits.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips_...
runtime/vm/assembler_mips_test.cc:267: EXPECT_EQ(static_cast<int32_t>(0xff0ff),
How about two different values in R1 and V0? Otherwise you will not notice if
you use the wrong source register.

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

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/disassembler_mi...
runtime/vm/disassembler_mips.cc:255: Format(instr, "ext 'rt, 'rs, 'sa, 'rd");
'rd is certainly incorrect here. It will print the register and not the size.
Additionally you need to do some math on the values to map them back to pos and
size from their encoding. I suggest you create a new format option for this,
such as 'pos.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/disassembler_mi...
runtime/vm/disassembler_mips.cc:259: Format(instr, "ins 'rt, 'rs, 'sa, 'rd");
ditto

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

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:121: // TODO(zra) unaligned access. Trap into
debugger.
Please use UNIMPLEMENTED() until it is.

ditto below.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:347: int32_t size = instr->Bits(11, 5) + 1;
What are Bits(11,5)? In the assembler you did cast an integer to a register, you
could do the reverse thing here. Or we could add an int typed accessor which
gets the RdField.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:358: int32_t size = instr->Bits(11, 5) - pos + 1;
ditto.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:400: // TODO(zra): trap into debugger on overflow.
Personally I would remove this instruction entirely. Because it is not done with
the trapping into the debugger. You will actually have to emulate OS signal
dispatching.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:424: // TODO(zra) trap into debugger.
UNIMPLEMENTED() here and other places.

[regis, 2013-03-11 17:31:11]

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

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:223: void addi(Register rt, Register rs, const
Immediate& imm) {
As discussed, we may never use this instruction, because it traps on overflow.
We will use addiu instead. So you may as well remove support for it.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:269: ASSERT((0 < pos + size) && (pos + size <= 32));
You could express these 3 asserts using Utils::IsUint().
int32_t msdb = size - 1;
ASSERT(Utils::IsUint(5, pos));
ASSERT(Utils::IsUint(5, msdb));
ASSERT(Utils::IsUint(5, pos + msdb));

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:277: ASSERT((0 < pos + size) && (pos + size <= 32));
ditto

Btw, I am not sure we are going to use ext and ins instructions in Dart.

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

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips_...
runtime/vm/assembler_mips_test.cc:245: __ ext(V0, R1, 13, 8);
I would expect pos = 12, since the caller expects 0xff.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips_...
runtime/vm/assembler_mips_test.cc:260: __ ins(V0, R1, 13, 8);
12?

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

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:348: ASSERT((0 < pos + size) && (pos + size <=32));
Use Utils::IsUint. See my other comments.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:350: int32_t mask = ((1 << size) - 1) << (pos - 1);
It should be pos, instead of pos - 1.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:359: ASSERT((0 < pos + size) && (pos + size <=32));
Use Utils::IsUint.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:361: int32_t rs_mask = ((1 << size) - 1);
Outer () not needed.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:362: int32_t rt_mask = rs_mask << (pos - 1);
pos, not pos - 1.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:365: rt_tmp | ((rs_val & rs_mask) << (pos - 1)));
pos, not pos - 1.

[zra, 2013-03-11 18:36:06]

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

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:277: ASSERT((0 < pos + size) && (pos + size <= 32));
On 2013/03/11 17:31:11, regis wrote:
> ditto
> 
> Btw, I am not sure we are going to use ext and ins instructions in Dart.

If these won't be used, as with addi, I'd be happy to remove them. But maybe it
would be useful for me to get a list of MIPS instructions that are likely to be
needed before I implement any more.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:282: void lb(Register rt, const Immediate& off,
Register base) {
On 2013/03/11 17:29:04, Ivan Posva wrote:
> The correct parameters here should be
> void lb(Register rt, const Address& addr)
> For an example see ldr in assembler_arm.h.
> 
> Don't you think?

As far as I understand, the addressing modes in MIPS are a bit simpler, but I've
filled out the Address class so we can see what it looks like, anyway. I think
it is a bit cleaner.

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

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:121: // TODO(zra) unaligned access. Trap into
debugger.
On 2013/03/11 17:29:04, Ivan Posva wrote:
> Please use UNIMPLEMENTED() until it is.
> 
> ditto below.

Done.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:400: // TODO(zra): trap into debugger on overflow.
On 2013/03/11 17:29:04, Ivan Posva wrote:
> Personally I would remove this instruction entirely. Because it is not done
with
> the trapping into the debugger. You will actually have to emulate OS signal
> dispatching.

Done.

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/simulator_mips....
runtime/vm/simulator_mips.cc:424: // TODO(zra) trap into debugger.
On 2013/03/11 17:29:04, Ivan Posva wrote:
> UNIMPLEMENTED() here and other places.

Done.

[zra, 2013-03-11 20:00:11]

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

https://codereview.chromium.org/12519007/diff/7001/runtime/vm/assembler_mips....
runtime/vm/assembler_mips.h:277: ASSERT((0 < pos + size) && (pos + size <= 32));
On 2013/03/11 17:31:11, regis wrote:
> ditto
> 
> Btw, I am not sure we are going to use ext and ins instructions in Dart.

As discussed, I have removed ext and ins.

[regis, 2013-03-11 20:14:01]

LGTM with 2 comments

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

https://codereview.chromium.org/12519007/diff/11002/runtime/vm/constants_mips...
runtime/vm/constants_mips.h:267: INS = 4,
Will there be other SPECIAL3 opcodes? If not, we may as well remove this
section.

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

https://codereview.chromium.org/12519007/diff/11002/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:281: case ANDI: {
Remove

[zra, 2013-03-11 20:25:01]

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

https://codereview.chromium.org/12519007/diff/11002/runtime/vm/constants_mips...
runtime/vm/constants_mips.h:267: INS = 4,
On 2013/03/11 20:14:01, regis wrote:
> Will there be other SPECIAL3 opcodes? If not, we may as well remove this
> section.

I don't think so. I have removed it.

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

https://codereview.chromium.org/12519007/diff/11002/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:281: case ANDI: {
On 2013/03/11 20:14:01, regis wrote:
> Remove

I think we probably want to keep this one. It doesn't cause a trap. (NB: This is
bitwise and with immediate. Not add immediate.)

[regis, 2013-03-11 20:26:22]

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

https://codereview.chromium.org/12519007/diff/11002/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:281: case ANDI: {
On 2013/03/11 20:25:01, zra wrote:
> On 2013/03/11 20:14:01, regis wrote:
> > Remove
> 
> I think we probably want to keep this one. It doesn't cause a trap. (NB: This
is
> bitwise and with immediate. Not add immediate.)

Oops, I need glasses.

[zra, 2013-03-11 20:50:58]

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