MIPS: Optimize simulator.

MIPS: Optimize simulator.

The patch decreases the calls of huge switch instructions making the DecodeType*() functions to work in one phase and optimizing Instruction::InstructionType(). Speed gain in release full check is about 33% (6:13 s -> 4:09 s) and in optdebug full test is about 50% (12:29 -> 6:17)

BUG=

Committed: https://crrev.com/09f41681ef83fe9e9d79748e2a60f16b70d5934c
Cr-Commit-Position: refs/heads/master@{#30596}

[paul.l..., 2015-09-02 17:30:12]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[paul.l..., 2015-09-02 17:30:13]

This looks great. Of course, it needs a real issue title. I suggest you do some
basic perf measurements, so you can say improves perf by XX% in release mode
check and YYY% in optdebug or debug check or whatever. Some rough metics would
be really nice to see in the commit message.

I agree with your comment elsewhere that we don't need mips64 in this CL. In
fact, I think it's better to keep them separate, and get this one landed.

(There are further small changes that I could suggest, but they are orthogonal
to this refactoring, and there is no reason to delay this excellent
improvement!)

(I did avoid cc of v8-dev, since the issue title is kind of meaningless now :)

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips.cc
File src/mips/simulator-mips.cc (right):

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:995: fprintf(stderr, "executed instructions: %"
PRIu64 "\n", icount_);
See my other comment (simulator-mips.h) about these perf counters, I this we
should either remove them, or enable them via a compile-time definition only
when we wish to use them.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3769: void Simulator::ArithmeticCommon(const int32_t
rt_reg, const int32_t alu_out) {
I think this could be better named SetResult() or SetOutputRegister() or
SetResultRegister(). (I kind of like the first one the best.)

See below comments, there seem to be lots of places where you _could_ use this
function but just do 'set_register(rt_reg, alu_out);' instread. Is it
intentional? It seems that the 'TrWriteReg()' is the only difference. If you are
avoiding the tracing, maybe make 'tracing' a parameter to this function, enabled
by default so you can use this function consistently.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3775: // Branch instructions common part.
I am ok with this as a macro -- it let's you avoid the function return value to
set execute_branch_delay_instruction, and avoid passing in 'instr'. Is there any
other reason for this as macro rather than inline function?

BranchHelper() might be better than than BranchCommon.

Also, I'd suggest maybe BranchAndLinkHelper() for the one that deals with
(optional) linking, and BranchHelper() for the one that has no linking support.
(That name is more like other MIPS instruction names than BranchNoLinking...)

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3894: default:
This is not necessary, but it might read a bit more clearly to separate the two
...AL() instructions and call BranchAndLinkHelper() explicitly. And call
BranchHelper() for the other two.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3955: ArithmeticCommon(rt_reg, alu_out);
Here (using above naming suggestion) you can just write:
SetResultRegister(rt_reg, rs + se_imm16); and avoid the temp use of alu_out
altogether. Many similar places below. (I think 'alu_out' was part of 2-phase
execution before, but we don't need that anymore.)

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3967: ArithmeticCommon(rt_reg, alu_out);
Here may be a case where use of 'alu_out' does enhance readability, as the
expression is quite long, and would be split across lines if you put it in the
function call directly.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3989: set_register(rt_reg, alu_out);
Any reason this is not ArithmeticCommon() (a.k.a SetResultRegister()) ? 

Oops, now I get it!! The ReadB will print address and data read. So we don't
want to print value again.

Maybe consider my suggestion above to have a NO_TRACE optional param to
SetResultRegister() for this case.

The only advantage is consistency of always calling that same function for
setting the result. Maybe this enhances readability. Your call, I could go
either way.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips.h
File src/mips/simulator-mips.h (right):

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.h:296: // Functions called from DeocodeTypeRegister
nits: Spelling: DecodeTypeRegister and end with period.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.h:424: uint64_t type_immediate_count_;
I tend to think these counts of the various types were a debugging aid for doing
this perf improvement, but we don't need to keep them.

If you do want to keep them, I would suggest adding them under some #ifdef
PERF_COUNTERS or similar, and creating macros to increment them that would also
'disappear' in normal release & debug builds.

[balazs.kilvady, 2015-09-04 12:57:15]

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips.cc
File src/mips/simulator-mips.cc (right):

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:995: fprintf(stderr, "executed instructions: %"
PRIu64 "\n", icount_);
On 2015/09/02 17:30:13, paul.l... wrote:
> See my other comment (simulator-mips.h) about these perf counters, I this we
> should either remove them, or enable them via a compile-time definition only
> when we wish to use them.

Done.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3769: void Simulator::ArithmeticCommon(const int32_t
rt_reg, const int32_t alu_out) {
On 2015/09/02 17:30:13, paul.l... wrote:
> I think this could be better named SetResult() or SetOutputRegister() or
> SetResultRegister(). (I kind of like the first one the best.)
> 
> See below comments, there seem to be lots of places where you _could_ use this
> function but just do 'set_register(rt_reg, alu_out);' instread. Is it
> intentional? It seems that the 'TrWriteReg()' is the only difference. If you
are
> avoiding the tracing, maybe make 'tracing' a parameter to this function,
enabled
> by default so you can use this function consistently.

Done.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3775: // Branch instructions common part.
On 2015/09/02 17:30:13, paul.l... wrote:
> I am ok with this as a macro -- it let's you avoid the function return value
to
> set execute_branch_delay_instruction, and avoid passing in 'instr'. Is there
any
> other reason for this as macro rather than inline function?
The only reason for using macros to avoid using inline functions with 5
parameters or repeating instr->getters().

> 
> BranchHelper() might be better than than BranchCommon.
> 
> Also, I'd suggest maybe BranchAndLinkHelper() for the one that deals with
> (optional) linking, and BranchHelper() for the one that has no linking
support.
> (That name is more like other MIPS instruction names than BranchNoLinking...)

Done.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3894: default:
On 2015/09/02 17:30:13, paul.l... wrote:
> This is not necessary, but it might read a bit more clearly to separate the
two
> ...AL() instructions and call BranchAndLinkHelper() explicitly. And call
> BranchHelper() for the other two.

Done.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3955: ArithmeticCommon(rt_reg, alu_out);
On 2015/09/02 17:30:12, paul.l... wrote:
> Here (using above naming suggestion) you can just write:
> SetResultRegister(rt_reg, rs + se_imm16); and avoid the temp use of alu_out
> altogether. Many similar places below. (I think 'alu_out' was part of 2-phase
> execution before, but we don't need that anymore.)
> 

Done.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3967: ArithmeticCommon(rt_reg, alu_out);
On 2015/09/02 17:30:12, paul.l... wrote:
> Here may be a case where use of 'alu_out' does enhance readability, as the
> expression is quite long, and would be split across lines if you put it in the
> function call directly.

Done.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:3989: set_register(rt_reg, alu_out);
On 2015/09/02 17:30:12, paul.l... wrote:
> Any reason this is not ArithmeticCommon() (a.k.a SetResultRegister()) ? 
> 
> Oops, now I get it!! The ReadB will print address and data read. So we don't
> want to print value again.
> 
> Maybe consider my suggestion above to have a NO_TRACE optional param to
> SetResultRegister() for this case.
> 
> The only advantage is consistency of always calling that same function for
> setting the result. Maybe this enhances readability. Your call, I could go
> either way.

Done.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips.h
File src/mips/simulator-mips.h (right):

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.h:296: // Functions called from DeocodeTypeRegister
On 2015/09/02 17:30:13, paul.l... wrote:
> nits: Spelling: DecodeTypeRegister and end with period.

Done.

https://codereview.chromium.org/1310883005/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.h:424: uint64_t type_immediate_count_;
On 2015/09/02 17:30:13, paul.l... wrote:
> I tend to think these counts of the various types were a debugging aid for
doing
> this perf improvement, but we don't need to keep them.
> 
> If you do want to keep them, I would suggest adding them under some #ifdef
> PERF_COUNTERS or similar, and creating macros to increment them that would
also
> 'disappear' in normal release & debug builds.

Done.

[paul.l..., 2015-09-04 16:35:45]

paul.lind@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com

[paul.l..., 2015-09-04 16:35:45]

LGTM. Thanks!

[balazs.kilvady, 2015-09-04 17:13:44]

The CQ bit was checked by balazs.kilvady@imgtec.com

[commit-bot: I haz the power, 2015-09-04 17:13:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310883005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310883005/40001

[commit-bot: I haz the power, 2015-09-04 17:39:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-04 17:40:19]

Patchset 3 (id:??) landed as
https://crrev.com/09f41681ef83fe9e9d79748e2a60f16b70d5934c
Cr-Commit-Position: refs/heads/master@{#30596}