Adds a few MIPS arithmetic instructions to the simulator, assembler, disassembler

Adds a few MIPS arithmetic instructions to the simulator, assembler, disassembler
Committed: https://code.google.com/p/dart/source/detail?r=19730

[zra, 2013-03-08 19:02:13]

There are lots of instructions, but I'll try to keep the CL's small, and just
submit a few at a time.

[regis, 2013-03-08 21:09:03]

LGTM with a few things to fix.

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

https://codereview.chromium.org/12545024/diff/1/runtime/vm/assembler_mips.h#n...
runtime/vm/assembler_mips.h:293: sll(rd, rd, 16);
The 2 instructions above can be replaced by a single LUI instruction.

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

https://codereview.chromium.org/12545024/diff/1/runtime/vm/assembler_mips_tes...
runtime/vm/assembler_mips_test.cc:156: // Move lo into V0
We like periods at the end of comments.
But this comment is not really necessary.

https://codereview.chromium.org/12545024/diff/1/runtime/vm/assembler_mips_tes...
runtime/vm/assembler_mips_test.cc:172: // Move lo into V0
ditto

https://codereview.chromium.org/12545024/diff/1/runtime/vm/assembler_mips_tes...
runtime/vm/assembler_mips_test.cc:188: // Move lo into V0
ditto

https://codereview.chromium.org/12545024/diff/1/runtime/vm/assembler_mips_tes...
runtime/vm/assembler_mips_test.cc:204: // Move lo into V0
ditto

https://codereview.chromium.org/12545024/diff/1/runtime/vm/assembler_mips_tes...
runtime/vm/assembler_mips_test.cc:258: 
No extra blank line needed here.

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

https://codereview.chromium.org/12545024/diff/1/runtime/vm/constants_mips.h#n...
runtime/vm/constants_mips.h:221: // SPECIAL
// SPECIAL opcodes.

https://codereview.chromium.org/12545024/diff/1/runtime/vm/constants_mips.h#n...
runtime/vm/constants_mips.h:261: // SPECIAL2
// SPECIAL2 opcodes.

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

https://codereview.chromium.org/12545024/diff/1/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:131: return;
Why return instead of break?
Maybe we should drop in the debugger if division by zero is not allowed.
Add a TODO(zra) for now.

https://codereview.chromium.org/12545024/diff/1/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:154: return;
ditto

https://codereview.chromium.org/12545024/diff/1/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:266: // Rt is not set on overflow
period

https://codereview.chromium.org/12545024/diff/1/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:268: ((res > 0) && (rs_val < 0) && (imm_val < 0))))
{
Is this correct? Can't you produce an overflow with a zero result? If res == 0,
you do not detect an overflow, e.g. MIN + MIN == 0.
The second line should probably be (res >= 0) &&...
Actually, all 3 '> 0' can be '>= 0'.

Oh, wait, you cannot represent 0x80000000 with a 16-bit immediate, so the code
may be OK. I still think it's clearer with >= 0.

https://codereview.chromium.org/12545024/diff/1/runtime/vm/simulator_mips.cc#...
runtime/vm/simulator_mips.cc:278: // Rt is set even on overflow
period

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

https://codereview.chromium.org/12545024/diff/1/runtime/vm/simulator_mips.h#n...
runtime/vm/simulator_mips.h:38: // Accessors for hi, lo registers
period

https://codereview.chromium.org/12545024/diff/1/runtime/vm/simulator_mips.h#n...
runtime/vm/simulator_mips.h:66: // special registers for the results of div,
divu.
// Special ...

[zra, 2013-03-08 21:45:14]

On 2013/03/08 21:09:03, regis wrote:
>
https://codereview.chromium.org/12545024/diff/1/runtime/vm/assembler_mips.h#n...
> runtime/vm/assembler_mips.h:293: sll(rd, rd, 16);
> The 2 instructions above can be replaced by a single LUI instruction.

Added the lui instruction and replaced these two with it.

>
https://codereview.chromium.org/12545024/diff/1/runtime/vm/simulator_mips.cc#...
> runtime/vm/simulator_mips.cc:268: ((res > 0) && (rs_val < 0) && (imm_val <
0))))
> {
> Is this correct? Can't you produce an overflow with a zero result? If res ==
0,
> you do not detect an overflow, e.g. MIN + MIN == 0.
> The second line should probably be (res >= 0) &&...
> Actually, all 3 '> 0' can be '>= 0'.
> 
> Oh, wait, you cannot represent 0x80000000 with a 16-bit immediate, so the code
> may be OK. I still think it's clearer with >= 0.

I copied over the overflow detection code from the arm simulator and used it
instead.

[zra, 2013-03-08 21:47:08]

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

[regis, 2013-03-08 22:26:24]

On 2013/03/08 21:47:08, zra wrote:
> Committed patchset #4 manually as r19730 (presubmit successful).

For next cl:
It is considered proper to acknowledge each review comment by replying to it or
simply clicking on "Done". It shows to the reviewer that you did not miss the
comment, without having to review a second time. Of course, I often click on
Done and then forget to make the change before submitting :-)

[Ivan Posva, 2013-03-11 06:03:27]

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

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:224: ASSERT(Utils::IsUint(16, imm.value()));
I would expect that imm is checked against Utils::IsInt(16, imm.value()) due to:
The 16-bit signed immediate is added to the 32-bit value in GPR rs to produce a
32-bit result.

And then do the static cast to uint16_t below.

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:226: EmitIType(ADDI, rs, rt, imm_value);
Also I am not sure we even use this instruction due to its trap causing
implications.

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:230: ASSERT(Utils::IsUint(16, imm.value()));
ditto (sign extension)

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

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips_test.cc:32: __ addi(V0, V0, Immediate(1));  // V0 not
set on overflow
See comment below.

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips_test.cc:39:
EXPECT_EQ(static_cast<int32_t>(0x7fffffff),
Shouldn't this execute a trap?

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips_test.cc:58: __ addiu(V0, V0, Immediate(1));  // V0 is
set on overflow
V0 is set to which value?
Probably it would be clearer to say:
V0 is modified on overflow.

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips_test.cc:127: __ addi(R1, ZR, Immediate(0xffff));
It would be much clearer if you passed a -1 here.

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

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:259: Format(instr, "addi 'rt, 'rs, 'immu");
'imms as this immediate is sign-extended.

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:263: Format(instr, "addiu 'rt, 'rs, 'immu");
ditto.

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

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/simulator_mips...
runtime/vm/simulator_mips.cc:289: set_register(instr->RtField(), res);
Shouldn't this also dispatch to a trap handler?

[zra, 2013-03-11 15:46:03]

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

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:224: ASSERT(Utils::IsUint(16, imm.value()));
On 2013/03/11 06:03:27, Ivan Posva wrote:
> I would expect that imm is checked against Utils::IsInt(16, imm.value()) due
to:
> The 16-bit signed immediate is added to the 32-bit value in GPR rs to produce
a
> 32-bit result.
> 
> And then do the static cast to uint16_t below.

Yah, you're right. I've already fixed this in my next CL.

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:226: EmitIType(ADDI, rs, rt, imm_value);
On 2013/03/11 06:03:27, Ivan Posva wrote:
> Also I am not sure we even use this instruction due to its trap causing
> implications.

Should I remove it?

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:230: ASSERT(Utils::IsUint(16, imm.value()));
On 2013/03/11 06:03:27, Ivan Posva wrote:
> ditto (sign extension)

In next CL

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

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips_test.cc:39:
EXPECT_EQ(static_cast<int32_t>(0x7fffffff),
On 2013/03/11 06:03:27, Ivan Posva wrote:
> Shouldn't this execute a trap?

It should execute a trap, yes. Is there any way to test that behavior, or should
I just remove this test?

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips_test.cc:58: __ addiu(V0, V0, Immediate(1));  // V0 is
set on overflow
On 2013/03/11 06:03:27, Ivan Posva wrote:
> V0 is set to which value?
> Probably it would be clearer to say:
> V0 is modified on overflow.

Done.

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/assembler_mips...
runtime/vm/assembler_mips_test.cc:127: __ addi(R1, ZR, Immediate(0xffff));
On 2013/03/11 06:03:27, Ivan Posva wrote:
> It would be much clearer if you passed a -1 here.

Done.

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

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:259: Format(instr, "addi 'rt, 'rs, 'immu");
On 2013/03/11 06:03:27, Ivan Posva wrote:
> 'imms as this immediate is sign-extended.

Done.

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/disassembler_m...
runtime/vm/disassembler_mips.cc:263: Format(instr, "addiu 'rt, 'rs, 'immu");
On 2013/03/11 06:03:27, Ivan Posva wrote:
> ditto.

Done.

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

https://codereview.chromium.org/12545024/diff/20002/runtime/vm/simulator_mips...
runtime/vm/simulator_mips.cc:289: set_register(instr->RtField(), res);
On 2013/03/11 06:03:27, Ivan Posva wrote:
> Shouldn't this also dispatch to a trap handler?

Yes. I've added a TODO for myself that I'll fill in once I bring the debugger
over from ARM.