Improve generated MIPS code for conditional expressions and branches by delaying

Improve generated MIPS code for conditional expressions and branches by delaying
emission of comparison code.
Improve instruction tracing in all simulators.

R=zra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42555

[regis, 2014-12-19 01:15:26]

regis@google.com changed reviewers:
+ zra@google.com

[zra, 2014-12-19 17:49:48]

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

https://codereview.chromium.org/817593002/diff/1/runtime/vm/assembler_mips.h#...
runtime/vm/assembler_mips.h:979: addiu(AT, ZR, Immediate(cond.imm()));
Why not LoadImmediate?

https://codereview.chromium.org/817593002/diff/1/runtime/vm/assembler_mips.h#...
runtime/vm/assembler_mips.h:1061: ASSERT((left != IMM) && (right != IMM));  //
No unsigned constants used.
What do you do if you want to branch on an unsigned comparison with an
immediate? Does that not come up?

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

https://codereview.chromium.org/817593002/diff/1/runtime/vm/constants_mips.h#...
runtime/vm/constants_mips.h:264: class Condition : public ValueObject {
Should this go in assembler_mips.h?

https://codereview.chromium.org/817593002/diff/1/runtime/vm/flow_graph_compil...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/817593002/diff/1/runtime/vm/flow_graph_compil...
runtime/vm/flow_graph_compiler_mips.cc:1428: if (obj.IsSmi()) {
Code like this (from here down to the LoadObject) is copied in a few places. Can
it be split out into a function? It's a little different each time, so I'm not
sure.

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

https://codereview.chromium.org/817593002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1505: if (static_cast<uintptr_t>(icount_) >
Maybe move this condition to a function with a comment about casting -1 to
unsigned for the compare in the no-trace case.

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

https://codereview.chromium.org/817593002/diff/1/runtime/vm/simulator_arm64.c...
runtime/vm/simulator_arm64.cc:1566: if (static_cast<uintptr_t>(icount_) >
Same comment.

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

https://codereview.chromium.org/817593002/diff/1/runtime/vm/simulator_mips.cc...
runtime/vm/simulator_mips.cc:1211: if (static_cast<uintptr_t>(icount_) >
Same comment.

[regis, 2014-12-22 20:17:34]

Thanks!

PTAL

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

https://codereview.chromium.org/817593002/diff/1/runtime/vm/assembler_mips.h#...
runtime/vm/assembler_mips.h:979: addiu(AT, ZR, Immediate(cond.imm()));
On 2014/12/19 17:49:47, zra wrote:
> Why not LoadImmediate?

I like to see exactly what instruction we generate. But more importantly, I do
not want to load a value that would not fit in a 16-bit immediate field.

https://codereview.chromium.org/817593002/diff/1/runtime/vm/assembler_mips.h#...
runtime/vm/assembler_mips.h:1061: ASSERT((left != IMM) && (right != IMM));  //
No unsigned constants used.
On 2014/12/19 17:49:47, zra wrote:
> What do you do if you want to branch on an unsigned comparison with an
> immediate? Does that not come up?

This does not come up. The compiler will always allocate a pair of registers for
an unsigned constant.

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

https://codereview.chromium.org/817593002/diff/1/runtime/vm/constants_mips.h#...
runtime/vm/constants_mips.h:264: class Condition : public ValueObject {
On 2014/12/19 17:49:47, zra wrote:
> Should this go in assembler_mips.h?

Done.

https://codereview.chromium.org/817593002/diff/1/runtime/vm/flow_graph_compil...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/817593002/diff/1/runtime/vm/flow_graph_compil...
runtime/vm/flow_graph_compiler_mips.cc:1428: if (obj.IsSmi()) {
On 2014/12/19 17:49:47, zra wrote:
> Code like this (from here down to the LoadObject) is copied in a few places.
Can
> it be split out into a function? It's a little different each time, so I'm not
> sure.

There are 3 places with similar code. I moved the code to a new assembler method
LoadConditionOperand.

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

https://codereview.chromium.org/817593002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1505: if (static_cast<uintptr_t>(icount_) >
On 2014/12/19 17:49:47, zra wrote:
> Maybe move this condition to a function with a comment about casting -1 to
> unsigned for the compare in the no-trace case.

Done.

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

https://codereview.chromium.org/817593002/diff/1/runtime/vm/simulator_arm64.c...
runtime/vm/simulator_arm64.cc:1566: if (static_cast<uintptr_t>(icount_) >
On 2014/12/19 17:49:47, zra wrote:
> Same comment.

Done.

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

https://codereview.chromium.org/817593002/diff/1/runtime/vm/simulator_mips.cc...
runtime/vm/simulator_mips.cc:1211: if (static_cast<uintptr_t>(icount_) >
On 2014/12/19 17:49:47, zra wrote:
> Same comment.

Done.

[zra, 2014-12-22 20:46:00]

lgtm

[regis, 2014-12-22 20:50:22]

Committed patchset #3 (id:40001) manually as r42555.