Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(25)

Issue 2375673002: MIPS64: Improve performance of simulator in debug mode. (Closed)

Created:
4 years, 2 months ago by balazs.kilvady
Modified:
4 years, 2 months ago
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

MIPS64: Improve performance of simulator in debug mode. BUG= Committed: https://crrev.com/96cb6d5a5894e5a44612a09c4f49ceb76fddaf80 Cr-Commit-Position: refs/heads/master@{#39860}

Patch Set 1 #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+351 lines, -302 lines) Patch
M src/mips64/constants-mips64.h View 9 chunks +203 lines, -69 lines 3 comments Download
M src/mips64/constants-mips64.cc View 1 chunk +0 lines, -112 lines 0 comments Download
M src/mips64/simulator-mips64.h View 3 chunks +48 lines, -17 lines 0 comments Download
M src/mips64/simulator-mips64.cc View 39 chunks +100 lines, -104 lines 6 comments Download

Depends on Patchset:

Messages

Total messages: 15 (7 generated)
balazs.kilvady
4 years, 2 months ago (2016-09-27 15:21:41 UTC) #2
miran.karic
lgtm
4 years, 2 months ago (2016-09-29 10:02:32 UTC) #4
Ilija.Pavlovic1
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.h#newcode1335 src/mips64/constants-mips64.h:1335: Why this ...
4 years, 2 months ago (2016-09-29 10:13:40 UTC) #6
balazs.kilvady
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.h#newcode1335 src/mips64/constants-mips64.h:1335: On 2016/09/29 10:13:39, Ilija.Pavlovic1 wrote: > Why this code ...
4 years, 2 months ago (2016-09-29 10:57:41 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2375673002/1
4 years, 2 months ago (2016-09-29 10:58:03 UTC) #10
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 2 months ago (2016-09-29 11:25:07 UTC) #11
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/96cb6d5a5894e5a44612a09c4f49ceb76fddaf80 Cr-Commit-Position: refs/heads/master@{#39860}
4 years, 2 months ago (2016-09-29 11:25:25 UTC) #13
Ilija.Pavlovic1
4 years, 2 months ago (2016-09-29 13:15:33 UTC) #15
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698