MIPS64: Improve performance of simulator in debug mode.

MIPS64: Improve performance of simulator in debug mode.

BUG=

Committed: https://crrev.com/96cb6d5a5894e5a44612a09c4f49ceb76fddaf80
Cr-Commit-Position: refs/heads/master@{#39860}

[balazs.kilvady, 2016-09-27 15:21:40]

balazs.kilvady@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com, gergely.kis@imgtec.com,
ilija.pavlovic.imgtec@gmail.com, ivica.bogosavljevic@imgtec.com,
marija.antic@imgtec.com, miran.karic@imgtec.com

[miran.karic, 2016-09-29 10:02:31]

miran.karic@imgtec.com changed reviewers:
+ Miran.Karic@imgtec.com

[miran.karic, 2016-09-29 10:02:32]

lgtm

[Ilija.Pavlovic1, 2016-09-29 10:13:39]

ilija.pavlovic@imgtec.com changed reviewers:
+ Ilija.Pavlovic@imgtec.com
- Miran.Karic@imgtec.com

[Ilija.Pavlovic1, 2016-09-29 10:13:40]

Generally this seems to me ok (lgtm).

https://codereview.chromium.org/2375673002/diff/1/src/mips64/constants-mips64.h
File src/mips64/constants-mips64.h (right):

https://codereview.chromium.org/2375673002/diff/1/src/mips64/constants-mips64...
src/mips64/constants-mips64.h:1335: 
Why this code is transferred here from constants-mips64.cc?

https://codereview.chromium.org/2375673002/diff/1/src/mips64/simulator-mips64.cc
File src/mips64/simulator-mips64.cc (right):

https://codereview.chromium.org/2375673002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:4789: SimInstruction simInstr = instr_;
Why this variable is defined? instr_ is the same type and defined inside class
Simulator.

https://codereview.chromium.org/2375673002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:4831: instr_ = instr;
Was it possible to do the same thing as for DecodeTypeJump(): to have
Simulator::InstructionDecode() without "Instructio* instr"?

[balazs.kilvady, 2016-09-29 10:57:40]

balazs.kilvady@imgtec.com changed reviewers:
+ ilija.pavlovic@imgtec.com
- Ilija.Pavlovic@imgtec.com

[balazs.kilvady, 2016-09-29 10:57:41]

https://codereview.chromium.org/2375673002/diff/1/src/mips64/constants-mips64.h
File src/mips64/constants-mips64.h (right):

https://codereview.chromium.org/2375673002/diff/1/src/mips64/constants-mips64...
src/mips64/constants-mips64.h:1335: 
On 2016/09/29 10:13:39, Ilija.Pavlovic1 wrote:
> Why this code is transferred here from constants-mips64.cc?

Because of the template stuff. Template implementation should be in the header
to make it includable to all the instantiation modules.

https://codereview.chromium.org/2375673002/diff/1/src/mips64/simulator-mips64.cc
File src/mips64/simulator-mips64.cc (right):

https://codereview.chromium.org/2375673002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:4789: SimInstruction simInstr = instr_;
On 2016/09/29 10:13:40, Ilija.Pavlovic1 wrote:
> Why this variable is defined? instr_ is the same type and defined inside class
> Simulator.

A copy of the current instruction is needed here as the
BranchDelayInstructionDecode() will override the this->instr_ member.

https://codereview.chromium.org/2375673002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:4831: instr_ = instr;
On 2016/09/29 10:13:39, Ilija.Pavlovic1 wrote:
> Was it possible to do the same thing as for DecodeTypeJump(): to have
> Simulator::InstructionDecode() without "Instructio* instr"?

No, this function is the main entry point for processing the current
instruction. So the this->instr_ must be inited at this point from the input
Instruction* instr.

This assignment instr_ = instr; also does the the one-and-only deep
InstructionType check for the current instruction and SimInstruction stores the
type. So the following InstructionType calls will be inlined and quick. So
without using the previous EXTRA (not deep/ quick/ not-so-reliable) type
checking we can be quicker while we can use deep/detailed/reliable instruction
type checking.

[balazs.kilvady, 2016-09-29 10:58:01]

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

[commit-bot: I haz the power, 2016-09-29 10:58:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 11:25:07]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-29 11:25:24]

Description was changed from

==========
MIPS64: Improve performance of simulator in debug mode.

BUG=
==========

to

==========
MIPS64: Improve performance of simulator in debug mode.

BUG=

Committed: https://crrev.com/96cb6d5a5894e5a44612a09c4f49ceb76fddaf80
Cr-Commit-Position: refs/heads/master@{#39860}
==========

[commit-bot: I haz the power, 2016-09-29 11:25:25]

Patchset 1 (id:??) landed as
https://crrev.com/96cb6d5a5894e5a44612a09c4f49ceb76fddaf80
Cr-Commit-Position: refs/heads/master@{#39860}

[Ilija.Pavlovic1, 2016-09-29 13:15:33]

ilija.pavlovic@imgtec.com changed reviewers:
+ Ilija.Pavlovic@imgtec.com

[Ilija.Pavlovic1, 2016-09-29 13:15:33]

https://codereview.chromium.org/2375673002/diff/1/src/mips64/constants-mips64.h
File src/mips64/constants-mips64.h (right):

https://codereview.chromium.org/2375673002/diff/1/src/mips64/constants-mips64...
src/mips64/constants-mips64.h:1335: 
On 2016/09/29 10:57:41, balazs.kilvady wrote:
> On 2016/09/29 10:13:39, Ilija.Pavlovic1 wrote:
> > Why this code is transferred here from constants-mips64.cc?
> 
> Because of the template stuff. Template implementation should be in the header
> to make it includable to all the instantiation modules.

Acknowledged.

https://codereview.chromium.org/2375673002/diff/1/src/mips64/simulator-mips64.cc
File src/mips64/simulator-mips64.cc (right):

https://codereview.chromium.org/2375673002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:4789: SimInstruction simInstr = instr_;
On 2016/09/29 10:57:41, balazs.kilvady wrote:
> On 2016/09/29 10:13:40, Ilija.Pavlovic1 wrote:
> > Why this variable is defined? instr_ is the same type and defined inside
class
> > Simulator.
> 
> A copy of the current instruction is needed here as the
> BranchDelayInstructionDecode() will override the this->instr_ member.

Acknowledged.

https://codereview.chromium.org/2375673002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:4831: instr_ = instr;
On 2016/09/29 10:57:41, balazs.kilvady wrote:
> On 2016/09/29 10:13:39, Ilija.Pavlovic1 wrote:
> > Was it possible to do the same thing as for DecodeTypeJump(): to have
> > Simulator::InstructionDecode() without "Instructio* instr"?
> 
> No, this function is the main entry point for processing the current
> instruction. So the this->instr_ must be inited at this point from the input
> Instruction* instr.
> 
> This assignment instr_ = instr; also does the the one-and-only deep
> InstructionType check for the current instruction and SimInstruction stores
the
> type. So the following InstructionType calls will be inlined and quick. So
> without using the previous EXTRA (not deep/ quick/ not-so-reliable) type
> checking we can be quicker while we can use deep/detailed/reliable instruction
> type checking.

Acknowledged.