MIPS[64]: Add support for FPR content in simulator trace.

MIPS[64]: Add support for FPR content in simulator trace.

Simulator trace will display content of target floating point registers.
Content of FP registers is displayed in hexadecimal form which is
followed with float or/and double interpretation.
Also, with this implementation will be displayed contents of general
purpose registers (GPRs). Hexadecimal form is followed with signed
and unsigned integer interpretation (32-bit or/and 64-bit).

TEST=
BUG=

Review-Url: https://codereview.chromium.org/2603083002
Cr-Commit-Position: refs/heads/master@{#42911}
Committed: https://chromium.googlesource.com/v8/v8/+/dac1327596c31e8348cd5163dae50860f763647f

[Ilija.Pavlovic1, 2016-12-30 10:58:24]

ilija.pavlovic@imgtec.com changed reviewers:
+ ivica.bogosavljevic@imgtec.com, marija.antic@imgtec.com,
miran.karic@imgtec.com

[Ilija.Pavlovic1, 2016-12-30 10:58:25]

PTAL

[ivica.bogosavljevic, 2017-01-23 13:22:38]

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

https://codereview.chromium.org/2603083002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1139: i64 |=
static_cast<uint64_t>(get_fpu_register_word(fpureg + 1)) << 32;
According to strict C/C++, bitwise operation should not be done on signed
integers. Use unsigned integers and do a static_cast to signed at the end

https://codereview.chromium.org/2603083002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1716: break;
this should be UNREACHABLE, right?

https://codereview.chromium.org/2603083002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1739: break;
UNREACHABLE!

https://codereview.chromium.org/2603083002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1765: break;
ibid here and all places bellow!

https://codereview.chromium.org/2603083002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1852: TraceMemRd(addr, static_cast<int32_t>(*ptr),
t);
What happens if t != WORD? Should it be unreachable, or some other values are
allowed? Please document!

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

https://codereview.chromium.org/2603083002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:1669: default:
Everything mentioned in simulator-mips.cc applies here as well

[Ilija.Pavlovic1, 2017-01-27 09:09:16]

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

https://codereview.chromium.org/2603083002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1139: i64 |=
static_cast<uint64_t>(get_fpu_register_word(fpureg + 1)) << 32;
On 2017/01/23 13:22:38, ivica.bogosavljevic wrote:
> According to strict C/C++, bitwise operation should not be done on signed
> integers. Use unsigned integers and do a static_cast to signed at the end

Done.

https://codereview.chromium.org/2603083002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1716: break;
On 2017/01/23 13:22:38, ivica.bogosavljevic wrote:
> this should be UNREACHABLE, right?

Yes, this should be UNREACHABLE(). Also, missing "break;" in "case FLOAT".

Done.

https://codereview.chromium.org/2603083002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1739: break;
On 2017/01/23 13:22:38, ivica.bogosavljevic wrote:
> UNREACHABLE!

Done.

https://codereview.chromium.org/2603083002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1765: break;
On 2017/01/23 13:22:38, ivica.bogosavljevic wrote:
> ibid here and all places bellow!

Done.

https://codereview.chromium.org/2603083002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1852: TraceMemRd(addr, static_cast<int32_t>(*ptr),
t);
On 2017/01/23 13:22:38, ivica.bogosavljevic wrote:
> What happens if t != WORD? Should it be unreachable, or some other values are
> allowed? Please document!

For WORD, the trace should be visible.
For FLOAT, the tracing will be omitted.
Other values will be UNREACHABLE.

Done.

[Ilija.Pavlovic1, 2017-01-27 09:10:12]

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

https://codereview.chromium.org/2603083002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:1669: default:
On 2017/01/23 13:22:38, ivica.bogosavljevic wrote:
> Everything mentioned in simulator-mips.cc applies here as well

Done.

[ivica.bogosavljevic, 2017-01-27 09:55:11]

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

https://codereview.chromium.org/2603083002/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:2614: TraceRegWr(get_fpu_register(fd_reg()), DOUBLE);
If rt() != 0, the trace will point that the value of the variable changed even
though it hasn't

https://codereview.chromium.org/2603083002/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:2644: TraceRegWr(get_fpu_register(fd_reg()), DOUBLE);
I think it would be a very good idea to implement floating point tracing in the
same way as implemented in regular tracing

Take a look at the SetResult function: it does setting and tracing.

I suggest you create SetFpuFloatResult and SetFpuDoubleResult similar to
SetResult and use that. In this way the code is cleaner, it is more similar to
the previous code and there are less bugs.

[Ilija.Pavlovic1, 2017-01-31 08:59:36]

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

https://codereview.chromium.org/2603083002/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:2614: TraceRegWr(get_fpu_register(fd_reg()), DOUBLE);
On 2017/01/27 09:55:11, ivica.bogosavljevic wrote:
> If rt() != 0, the trace will point that the value of the variable changed even
> though it hasn't

Done.

https://codereview.chromium.org/2603083002/diff/20001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:2644: TraceRegWr(get_fpu_register(fd_reg()), DOUBLE);
On 2017/01/27 09:55:11, ivica.bogosavljevic wrote:
> I think it would be a very good idea to implement floating point tracing in
the
> same way as implemented in regular tracing
> 
> Take a look at the SetResult function: it does setting and tracing.
> 
> I suggest you create SetFpuFloatResult and SetFpuDoubleResult similar to
> SetResult and use that. In this way the code is cleaner, it is more similar to
> the previous code and there are less bugs.

Done.

[ivica.bogosavljevic, 2017-02-02 16:22:29]

LGTM

[Ilija.Pavlovic1, 2017-02-03 07:09:39]

The CQ bit was checked by ilija.pavlovic@imgtec.com

[commit-bot: I haz the power, 2017-02-03 07:09:44]

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, 2017-02-03 07:31:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486105779092710,
"parent_rev": "9c7da663f93df1e6fa8a9e899f2bacbe2b7f8d8e", "commit_rev":
"dac1327596c31e8348cd5163dae50860f763647f"}

[commit-bot: I haz the power, 2017-02-03 07:31:05]

Description was changed from

==========
MIPS[64]: Add support for FPR content in simulator trace.

Simulator trace will display content of target floating point registers.
Content of FP registers is displayed in hexadecimal form which is
followed with float or/and double interpretation.
Also, with this implementation will be displayed contents of general
purpose registers (GPRs). Hexadecimal form is followed with signed
and unsigned integer interpretation (32-bit or/and 64-bit).

TEST=
BUG=
==========

to

==========
MIPS[64]: Add support for FPR content in simulator trace.

Simulator trace will display content of target floating point registers.
Content of FP registers is displayed in hexadecimal form which is
followed with float or/and double interpretation.
Also, with this implementation will be displayed contents of general
purpose registers (GPRs). Hexadecimal form is followed with signed
and unsigned integer interpretation (32-bit or/and 64-bit).

TEST=
BUG=

Review-Url: https://codereview.chromium.org/2603083002
Cr-Commit-Position: refs/heads/master@{#42911}
Committed:
https://chromium.googlesource.com/v8/v8/+/dac1327596c31e8348cd5163dae50860f76...
==========

[commit-bot: I haz the power, 2017-02-03 07:31:06]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/dac1327596c31e8348cd5163dae50860f76...