MIPS: Add float instructions and test coverage, part one

MIPS: Add float instructions and test coverage, part one

Implement assembler, disassembler tests for all instructions for mips32 and mips64. Additionally, add missing single precision float instructions for r2 and r6 architecture variants in assembler, simulator and disassembler with corresponding tests.

Committed: https://crrev.com/2a6a87d71a1ae280c9a47ec65c75b0cfcf898377
Cr-Commit-Position: refs/heads/master@{#28402}

[Djordje.Pesic, 2015-05-04 11:24:54]

djordje.pesic@imgtec.com changed reviewers:
+ balazs.kilvady@imgtec.com, dusan.milosavljevic@imgtec.com,
paul.lind@imgtec.com

[paul.l..., 2015-05-05 02:56:09]

Thanks for this CL, and for adding all these tests, which have been missing for
so long!

This is only a very preliminary first review, and I only looked at
test-assembler-mips.cc in any detail. More review to come.

There are many arch-variants and modes covered by these tests. It would be most
helpful for reviewers to know what platforms you have tested on. We really
should ensure that sim, and hardware (or qemu) have been tested in mips32r1, r2,
r6, mips64r2, r6, and on hardware with/without FPU. That is a _lot_ of testing
combinations.

https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-...
File test/cctest/test-assembler-mips.cc (right):

https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:1332: TEST(MIPS16) {
I've not been a fan of these numeric test names. Initially this was taken from
the early test-assembler-arm.cc. But they have gone to naming the tests for
specific functionality. 

This one might be TEST(sel) or TEST(selxxx) for examples. I'm open to your
suggestion for better names, but I would like to see some names here. Earlier
tests in this file also had a comment summary right below the test name to show
what was being tested. If a single test name is not sufficient to convey the
summary, please add comment as needed.

https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:1333: if (IsMipsArchVariant(kMips32r6)) {
I see 3 basic categories of tests, generic for all mips32 arch variants,
mips32r2 specific, and mips32r6 specific. I think you should Order the tests to
keep all tests of a category (e.g., mips32r6) together in the file. Maybe with
some sort of "banner comment":

// ----- mips32r6 specific tests ----------------------------------------

TEST(xxxx) {
...

Same for test-assembler-mips64.

https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:1370: __ seleqz(D, f4, f0, f2);
The seleqz functions should have assembler aliases seleqz_s and seleqz_d (can be
implemented all in assembler-mips.h, I think), and those aliases should be used
here.

https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:2821: __ floor_l_s(f10, f6);
As we just learned in gh issue 115 we cannot use the 'long' variant of these
instructions when running on mips32 in fp32 mode. They are only 'legal' when run
in fp64 mode. I believe that fp64 is selected by default when using mips32r6,
but it is definitely not default in mips32r2.

https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:2937: __ ceil_l_s(f10, f6);
Same as above floor, re: long variants. Maybe in other places as well.

[Djordje.Pesic, 2015-05-06 08:10:39]

https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-...
File test/cctest/test-assembler-mips.cc (right):

https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:1332: TEST(MIPS16) {
On 2015/05/05 02:56:08, paul.l... wrote:
> I've not been a fan of these numeric test names. Initially this was taken from
> the early test-assembler-arm.cc. But they have gone to naming the tests for
> specific functionality. 
> 
> This one might be TEST(sel) or TEST(selxxx) for examples. I'm open to your
> suggestion for better names, but I would like to see some names here. Earlier
> tests in this file also had a comment summary right below the test name to
show
> what was being tested. If a single test name is not sufficient to convey the
> summary, please add comment as needed.

Done.

https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:1333: if (IsMipsArchVariant(kMips32r6)) {
On 2015/05/05 02:56:09, paul.l... wrote:
> I see 3 basic categories of tests, generic for all mips32 arch variants,
> mips32r2 specific, and mips32r6 specific. I think you should Order the tests
to
> keep all tests of a category (e.g., mips32r6) together in the file. Maybe with
> some sort of "banner comment":
> 
> // ----- mips32r6 specific tests ----------------------------------------
> 
> TEST(xxxx) {
> ...
> 
> Same for test-assembler-mips64.

Done.

https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:1370: __ seleqz(D, f4, f0, f2);
On 2015/05/05 02:56:08, paul.l... wrote:
> The seleqz functions should have assembler aliases seleqz_s and seleqz_d (can
be
> implemented all in assembler-mips.h, I think), and those aliases should be
used
> here.

Done.

[paul.l..., 2015-05-08 05:23:18]

One nit comment. I need to continue this review tomorrow.

https://codereview.chromium.org/1119203003/diff/60001/test/cctest/test-assemb...
File test/cctest/test-assembler-mips.cc (right):

https://codereview.chromium.org/1119203003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:811: // Test conversions between doubles and
long integers.
Somewhat misleading comments regarding 'long' (from original code.) While
technically, a 'long' is a 32-bit int in gcc, when discussing these MIPS
conversion instructions, we use 'long' to refer to 64-bit conversions, and 'w'
to convert to 32-bit integers. These are all test32-bit ints. Please change the
wording to not use 'long'.

[paul.l..., 2015-05-09 01:07:48]

Many of the comments I made in the mips32 code apply equally to mips64.

https://codereview.chromium.org/1119203003/diff/60001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):

https://codereview.chromium.org/1119203003/diff/60001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:2291: void Assembler::rsqrt_d(FPURegister fd,
FPURegister fs) {
From the manual: "The result of RSQRT.D is UNPREDICTABLE if the processor is
executing in 16 FP registers mode."
So we either need an assertion about FP64 mode, or we need to consider dropping
these altogether, as I suggested elsewhere due to their accuracy issues.

https://codereview.chromium.org/1119203003/diff/60001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:2296: void Assembler::recip_d(FPURegister fd,
FPURegister fs) {
From the manual: "The result of RECIP.D is UNPREDICTABLE if the processor is
executing in 16 FP registers mode."
So we either need an assertion about FP64 mode, or we need to consider dropping
this instr.

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips.h
File src/mips/constants-mips.h (right):

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips...
src/mips/constants-mips.h:480: NEG_D = ((0 << 3) + 7),
We have inconsistency in naming with many of these FP ops, which apparently were
named when we only intended to support doubles. These are defined as MOV.fmt &
NEG.fmt in the manual, and can be used for single or double, by specifying the
format field.

We do not have to clean up the entire world in this CL, but we should note
inconsistency, and make a decision if these should be cleaned in a future CL.

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips...
src/mips/constants-mips.h:490: RSQRT_D = ((2 << 3) + 6),
These two should be named consistently. Since they are both *.fmt, you should
remove the _D from RSQRT.

Should we even implement instructions for these? The manual says both are
approximations: "The numeric accuracy of this operation is implementation
dependent; it does not meet the accuracy specified by the IEEE 754 Floating
Point standard. The computed result differs from both the exact result and the
IEEE-mandated representation of the exact result by no more than two units in
the least-significant place (ULP)."

If we have a need now, put them in. If we do not have a need that can tolerate
the inaccuracy, we might want to skip these. If we do not skip, we should add
comments to the assembler files where they are defined, so someone does not use
them in a way that requires accuracy.

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips...
src/mips/constants-mips.h:550: MOVN_C = ((2 << 3) + 3),
Other FP instructions that have _C suffix (to differentiate from integer
version), have an explicit comment:
 // COP1 on FPR registers. 
Please add that here.

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

https://codereview.chromium.org/1119203003/diff/60001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:2562: case TRUNC_L_D: {  // Mips32r2 instruction.
Since you are touching this code, consider adding an assert fail to all this
mips32r2 (and above) instructions if the arch-variant is mips32r1 or earlier. I
suppose easiest is DCHECK(IsMipsArchVariant(kMips32r2) ||
IsMipsArchVariant(kMips32r6));

https://codereview.chromium.org/1119203003/diff/60001/src/mips64/simulator-mi...
File src/mips64/simulator-mips64.cc (right):

https://codereview.chromium.org/1119203003/diff/60001/src/mips64/simulator-mi...
src/mips64/simulator-mips64.cc:1191: void
Simulator::set_fcsr_rounding_mode(FPURoundingMode mode) { FCSR_ |= mode; }
As you did in previous CL for mips32, please use the RoundingModeMask here and
below:
FCSR_ |= mode & kFPURoundingModeMask

https://codereview.chromium.org/1119203003/diff/60001/src/mips64/simulator-mi...
src/mips64/simulator-mips64.cc:1260: 
I like the comments that you have on the set_fcsr_round64_error() below, please
add them here as well.

https://codereview.chromium.org/1119203003/diff/60001/src/mips64/simulator-mi...
src/mips64/simulator-mips64.cc:2564: case ROUND_L_S: {  // Mips32r2 instruction.
mips64r2.

We don't support earlier than mips64r2, so you don't need asserts here, but
please correct the comment.

[dusmil.imgtec, 2015-05-11 13:13:24]

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips.h
File src/mips/constants-mips.h (right):

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips...
src/mips/constants-mips.h:490: RSQRT_D = ((2 << 3) + 6),
For the sake of simulator completeness, let's leave them in code.
Their names already mean approximation, so if programmers used them they used to
know their behavior.

[Djordje.Pesic, 2015-05-14 13:22:31]

https://codereview.chromium.org/1119203003/diff/60001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):

https://codereview.chromium.org/1119203003/diff/60001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:2291: void Assembler::rsqrt_d(FPURegister fd,
FPURegister fs) {
On 2015/05/09 01:07:47, paul.l... wrote:
> From the manual: "The result of RSQRT.D is UNPREDICTABLE if the processor is
> executing in 16 FP registers mode."
> So we either need an assertion about FP64 mode, or we need to consider
dropping
> these altogether, as I suggested elsewhere due to their accuracy issues.

Done.

https://codereview.chromium.org/1119203003/diff/60001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:2296: void Assembler::recip_d(FPURegister fd,
FPURegister fs) {
On 2015/05/09 01:07:47, paul.l... wrote:
> From the manual: "The result of RECIP.D is UNPREDICTABLE if the processor is
> executing in 16 FP registers mode."
> So we either need an assertion about FP64 mode, or we need to consider
dropping
> this instr.

Done.

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips.h
File src/mips/constants-mips.h (right):

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips...
src/mips/constants-mips.h:480: NEG_D = ((0 << 3) + 7),
On 2015/05/09 01:07:47, paul.l... wrote:
> We have inconsistency in naming with many of these FP ops, which apparently
were
> named when we only intended to support doubles. These are defined as MOV.fmt &
> NEG.fmt in the manual, and can be used for single or double, by specifying the
> format field.
> 
> We do not have to clean up the entire world in this CL, but we should note
> inconsistency, and make a decision if these should be cleaned in a future CL.

Done.

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips...
src/mips/constants-mips.h:490: RSQRT_D = ((2 << 3) + 6),
On 2015/05/09 01:07:47, paul.l... wrote:
> These two should be named consistently. Since they are both *.fmt, you should
> remove the _D from RSQRT.
> 
> Should we even implement instructions for these? The manual says both are
> approximations: "The numeric accuracy of this operation is implementation
> dependent; it does not meet the accuracy specified by the IEEE 754 Floating
> Point standard. The computed result differs from both the exact result and the
> IEEE-mandated representation of the exact result by no more than two units in
> the least-significant place (ULP)."
> 
> If we have a need now, put them in. If we do not have a need that can tolerate
> the inaccuracy, we might want to skip these. If we do not skip, we should add
> comments to the assembler files where they are defined, so someone does not
use
> them in a way that requires accuracy.

Done.

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips...
src/mips/constants-mips.h:490: RSQRT_D = ((2 << 3) + 6),
On 2015/05/11 13:13:24, dusmil.imgtec wrote:
> For the sake of simulator completeness, let's leave them in code.
> Their names already mean approximation, so if programmers used them they used
to
> know their behavior.
> 
> 

Done.

https://codereview.chromium.org/1119203003/diff/60001/src/mips/constants-mips...
src/mips/constants-mips.h:550: MOVN_C = ((2 << 3) + 3),
On 2015/05/09 01:07:47, paul.l... wrote:
> Other FP instructions that have _C suffix (to differentiate from integer
> version), have an explicit comment:
>  // COP1 on FPR registers. 
> Please add that here.

Done.

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

https://codereview.chromium.org/1119203003/diff/60001/src/mips/simulator-mips...
src/mips/simulator-mips.cc:2562: case TRUNC_L_D: {  // Mips32r2 instruction.
On 2015/05/09 01:07:48, paul.l... wrote:
> Since you are touching this code, consider adding an assert fail to all this
> mips32r2 (and above) instructions if the arch-variant is mips32r1 or earlier.
I
> suppose easiest is DCHECK(IsMipsArchVariant(kMips32r2) ||
> IsMipsArchVariant(kMips32r6));

Done.

https://codereview.chromium.org/1119203003/diff/60001/src/mips64/simulator-mi...
File src/mips64/simulator-mips64.cc (right):

https://codereview.chromium.org/1119203003/diff/60001/src/mips64/simulator-mi...
src/mips64/simulator-mips64.cc:1191: void
Simulator::set_fcsr_rounding_mode(FPURoundingMode mode) { FCSR_ |= mode; }
On 2015/05/09 01:07:48, paul.l... wrote:
> As you did in previous CL for mips32, please use the RoundingModeMask here and
> below:
> FCSR_ |= mode & kFPURoundingModeMask

Done.

https://codereview.chromium.org/1119203003/diff/60001/src/mips64/simulator-mi...
src/mips64/simulator-mips64.cc:1260: 
On 2015/05/09 01:07:48, paul.l... wrote:
> I like the comments that you have on the set_fcsr_round64_error() below,
please
> add them here as well.

Done.

https://codereview.chromium.org/1119203003/diff/60001/src/mips64/simulator-mi...
src/mips64/simulator-mips64.cc:2564: case ROUND_L_S: {  // Mips32r2 instruction.
On 2015/05/09 01:07:48, paul.l... wrote:
> mips64r2.
> 
> We don't support earlier than mips64r2, so you don't need asserts here, but
> please correct the comment.

Done.

https://codereview.chromium.org/1119203003/diff/60001/test/cctest/test-assemb...
File test/cctest/test-assembler-mips.cc (right):

https://codereview.chromium.org/1119203003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:811: // Test conversions between doubles and
long integers.
On 2015/05/08 05:23:18, paul.l... wrote:
> Somewhat misleading comments regarding 'long' (from original code.) While
> technically, a 'long' is a 32-bit int in gcc, when discussing these MIPS
> conversion instructions, we use 'long' to refer to 64-bit conversions, and 'w'
> to convert to 32-bit integers. These are all test32-bit ints. Please change
the
> wording to not use 'long'.

Done.

[dusmil.imgtec, 2015-05-14 13:23:16]

LGTM.

[Djordje.Pesic, 2015-05-14 13:36:37]

The CQ bit was checked by djordje.pesic@imgtec.com

[commit-bot: I haz the power, 2015-05-14 13:36:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119203003/80001

[commit-bot: I haz the power, 2015-05-14 14:02:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-14 14:03:02]

Patchset 5 (id:??) landed as
https://crrev.com/2a6a87d71a1ae280c9a47ec65c75b0cfcf898377
Cr-Commit-Position: refs/heads/master@{#28402}