MIPS: Implement MADD.S, MSUB, MADDF and MSUBF.

MIPS: Implement MADD.S, MSUB, MADDF and MSUBF.

Implementation MADD.S. MSUB.fmt, MADDF.fmt, MSUBF.fmt and corresponding
tests for assembler and disassembler.

TEST=cctest/test-assembler-mips[64], cctest/test-disasm-mips[64]
BUG=

Committed: https://crrev.com/65fd5e1165104e387edf7348661c5f43ac954a52
Cr-Commit-Position: refs/heads/master@{#39415}

[Ilija.Pavlovic1, 2016-09-05 07:27:51]

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

[Ilija.Pavlovic1, 2016-09-05 07:27:52]

PTAL.

[balazs.kilvady, 2016-09-05 12:54:01]

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

https://codereview.chromium.org/2313623002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:3393: switch (get_instr()->FunctionFieldRaw()) {
Since fr, fs and ft is used in all switch cases, please declare and initialize
them at van place before the switch.

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

https://codereview.chromium.org/2313623002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:3322: void Simulator::DecodeTypeRegisterCOP1X() {
Since fr, fs and ft is used in all switch cases, please declare and initialize
them at van place before the switch.

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

https://codereview.chromium.org/2313623002/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:5388: 
I think you should create common TestCases but different test functions for r2
and r6 instructions (also take care about r1, longsoon test boards with
IsMipsArchVariant() checking). Please add the short input values to the test
cases to show different results for maddf.fmt and madd.fmt with different
expected results. Tests exist to discover problems not to hide them :) Also
tricky as simulator will produce the same result for the same input while r2 and
r6 boards can differ. So I think you need a #if USE_SIMULATOR #else #endif
solution to make the tests passable on sims and all the supported boars (r1, r2,
r6).

[Ilija.Pavlovic1, 2016-09-07 08:44:30]

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

https://codereview.chromium.org/2313623002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:3393: switch (get_instr()->FunctionFieldRaw()) {
On 2016/09/05 12:54:00, balazs.kilvady wrote:
> Since fr, fs and ft is used in all switch cases, please declare and initialize
> them at van place before the switch.

Note in MADD_D and MSUB_D that the same variables are declared as "double".
Therefore variable declarations must be inside of the "case" bodies.

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

https://codereview.chromium.org/2313623002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:3322: void Simulator::DecodeTypeRegisterCOP1X() {
On 2016/09/05 12:54:00, balazs.kilvady wrote:
> Since fr, fs and ft is used in all switch cases, please declare and initialize
> them at van place before the switch.

These variables are declared as "float" and in MADD_D and MSUB_D are declared as
"double". Therefore these declarations must be on this places.

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

https://codereview.chromium.org/2313623002/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:5388: 
On 2016/09/05 12:54:01, balazs.kilvady wrote:
> I think you should create common TestCases but different test functions for r2
> and r6 instructions (also take care about r1, longsoon test boards with
> IsMipsArchVariant() checking). Please add the short input values to the test
> cases to show different results for maddf.fmt and madd.fmt with different
> expected results. Tests exist to discover problems not to hide them :) Also
> tricky as simulator will produce the same result for the same input while r2
and
> r6 boards can differ. So I think you need a #if USE_SIMULATOR #else #endif
> solution to make the tests passable on sims and all the supported boars (r1,
r2,
> r6).

The test will be broken in two tests: for r2 will be TEST(madd_s_madd_s) and for
r6 will be TEST(maddf_s_msubf_s). On similar way will be done separation for
TEST(madd_d_msub_d_maddf_d_msubf_d). Short input values will be added.

[balazs.kilvady, 2016-09-07 09:46:52]

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

https://codereview.chromium.org/2313623002/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:3393: switch (get_instr()->FunctionFieldRaw()) {
On 2016/09/07 08:44:29, Ilija.Pavlovic1 wrote:
> On 2016/09/05 12:54:00, balazs.kilvady wrote:
> > Since fr, fs and ft is used in all switch cases, please declare and
initialize
> > them at van place before the switch.
> 
> Note in MADD_D and MSUB_D that the same variables are declared as "double".
> Therefore variable declarations must be inside of the "case" bodies.

Acknowledged.

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

https://codereview.chromium.org/2313623002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:3322: void Simulator::DecodeTypeRegisterCOP1X() {
On 2016/09/07 08:44:29, Ilija.Pavlovic1 wrote:
> On 2016/09/05 12:54:00, balazs.kilvady wrote:
> > Since fr, fs and ft is used in all switch cases, please declare and
initialize
> > them at van place before the switch.
> 
> These variables are declared as "float" and in MADD_D and MSUB_D are declared
as
> "double". Therefore these declarations must be on this places.

Acknowledged.

[balazs.kilvady, 2016-09-07 18:32:48]

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

https://codereview.chromium.org/2313623002/diff/20001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5390: if (IsMipsArchVariant(kMips32r2)) {
In this kind of cases I prefer if (!cond) return; to save one block-nesting
level, but it is matter of taste.

https://codereview.chromium.org/2313623002/diff/20001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5400: 
If you use the same test values for all the 4 variants then you could create a
helper function - a template which works for floats and doubles. (BTW in pure C
there are sqrt() for doubles and sqrtf() for floats but in c++ you can use
std::sqrt() of <cmath> which is always good:
http://en.cppreference.com/w/cpp/numeric/math/sqrt).

https://codereview.chromium.org/2313623002/diff/20001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5529: float res_sub = tc.fr - (tc.fs *
tc.ft);
Looks good, but could you check the compiled code of the above two lines? If the
generated code also uses maddf.fmt and subf.mft then the test is quite useless
and alway passed. I am thinking about an expected result table but then we
should create two versions: one for boards and one for simulators. As simulator
uses the same code to calculate maddf.fmt's result I don't know if it worst too
much to run these tests on sims. But anyway, syntax, and call parameters will be
checked.

[Ilija.Pavlovic1, 2016-09-09 12:35:18]

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

https://codereview.chromium.org/2313623002/diff/20001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5390: if (IsMipsArchVariant(kMips32r2)) {
On 2016/09/07 18:32:47, balazs.kilvady wrote:
> In this kind of cases I prefer if (!cond) return; to save one block-nesting
> level, but it is matter of taste.

The test is made as template so the logic (and checking) is now different.

Done.

https://codereview.chromium.org/2313623002/diff/20001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5400: 
On 2016/09/07 18:32:47, balazs.kilvady wrote:
> If you use the same test values for all the 4 variants then you could create a
> helper function - a template which works for floats and doubles. (BTW in pure
C
> there are sqrt() for doubles and sqrtf() for floats but in c++ you can use
> std::sqrt() of <cmath> which is always good:
> http://en.cppreference.com/w/cpp/numeric/math/sqrt).

Used std::sqrt().

Done.

https://codereview.chromium.org/2313623002/diff/20001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5529: float res_sub = tc.fr - (tc.fs *
tc.ft);
On 2016/09/07 18:32:47, balazs.kilvady wrote:
> Looks good, but could you check the compiled code of the above two lines? If
the
> generated code also uses maddf.fmt and subf.mft then the test is quite useless
> and alway passed. I am thinking about an expected result table but then we
> should create two versions: one for boards and one for simulators. As
simulator
> uses the same code to calculate maddf.fmt's result I don't know if it worst
too
> much to run these tests on sims. But anyway, syntax, and call parameters will
be
> checked. 

The test is now template.
The formula is used for creating expected results. On this way, control value is
calculated and rounded on the way which is used on the test system (simulator or
board). Using hard-coded values would not be practical.

Done.

[ivica.bogosavljevic, 2016-09-12 12:39:52]

https://codereview.chromium.org/2313623002/diff/40001/src/mips64/assembler-mi...
File src/mips64/assembler-mips64.cc (right):

https://codereview.chromium.org/2313623002/diff/40001/src/mips64/assembler-mi...
src/mips64/assembler-mips64.cc:2785: DCHECK(kArchVariant != kMips64r6);
I would prefer you set here: 
kArchVairan == kMips64r2 because, in the future, if kMips64r7 appears, this
condition will yield true.
The same applies for other instructions as well

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

https://codereview.chromium.org/2313623002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5424: 
I think we should avoid this std::is_same because it nullifies the purpose of
template functions. Instead, we should forward the assembler code to be executed
using lambda expressions. For example, please see: TEST(Ulwc1) in
test-macro-assembler-mips.cc.

https://codereview.chromium.org/2313623002/diff/40001/test/cctest/test-assemb...
File test/cctest/test-assembler-mips64.cc (right):

https://codereview.chromium.org/2313623002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5975: __ lwc1(f6, MemOperand(a0,
offsetof(TestCase, fs)));
Same comment as in test-assembler-mips.cc

[Ilija.Pavlovic1, 2016-09-12 13:41:35]

https://codereview.chromium.org/2313623002/diff/40001/src/mips64/assembler-mi...
File src/mips64/assembler-mips64.cc (right):

https://codereview.chromium.org/2313623002/diff/40001/src/mips64/assembler-mi...
src/mips64/assembler-mips64.cc:2785: DCHECK(kArchVariant != kMips64r6);
On 2016/09/12 12:39:52, ivica.bogosavljevic wrote:
> I would prefer you set here: 
> kArchVairan == kMips64r2 because, in the future, if kMips64r7 appears, this
> condition will yield true.
> The same applies for other instructions as well

Done.

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

https://codereview.chromium.org/2313623002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5424: 
On 2016/09/12 12:39:52, ivica.bogosavljevic wrote:
> I think we should avoid this std::is_same because it nullifies the purpose of
> template functions. Instead, we should forward the assembler code to be
executed
> using lambda expressions. For example, please see: TEST(Ulwc1) in
> test-macro-assembler-mips.cc.

With using lambda functions we will get repeating the same code lines (lwc1 or
ldc1) in the test itself. These lines will remain unchanged.

Ack.

https://codereview.chromium.org/2313623002/diff/40001/test/cctest/test-assemb...
File test/cctest/test-assembler-mips64.cc (right):

https://codereview.chromium.org/2313623002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5975: __ lwc1(f6, MemOperand(a0,
offsetof(TestCase, fs)));
On 2016/09/12 12:39:52, ivica.bogosavljevic wrote:
> Same comment as in test-assembler-mips.cc

These lines will remain unchanged.

Ack.

[ivica.bogosavljevic, 2016-09-13 07:44:21]

Please make sure that tests are passing for BOTH MIPSR6 simulator and qemu-user
with the same results.

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

https://codereview.chromium.org/2313623002/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5426: if (std::is_same<T, float>::value) {
Could you use if (sizeof(T) == 4) else if (sizeof(T) == 8) instead of this type
check?

https://codereview.chromium.org/2313623002/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5460: if (IsMipsArchVariant(kMips32r2))
You need to add braces {} here

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

https://codereview.chromium.org/2313623002/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5974: if (std::is_same<T, float>::value) {
Same as in test-assembler-mips.cc

https://codereview.chromium.org/2313623002/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:6008: if (kArchVariant != kMips64r6)
Same as test-assembler-mips.cc

[balazs.kilvady, 2016-09-13 10:20:32]

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

https://codereview.chromium.org/2313623002/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5426: if (std::is_same<T, float>::value) {
On 2016/09/13 07:44:20, ivica.bogosavljevic wrote:
> Could you use if (sizeof(T) == 4) else if (sizeof(T) == 8) instead of this
type
> check?

Why would that be better? T can be float or double not any 4 or 8 bytes long
type. This way we have an implicit typecheck. In the else branch instead of 'if'
you can use DCHECK(...double...) so the function/test fails if T is not float or
double. Otherwise you could use static_assert() for ensuring T's type. Or some
template constraints?

Or even you could use one more template function argument for lwc1/ldc1 and one
more lambda parameter so the tests could pass a loader function/lambda to this
helper function.

But I think it's OK.

https://codereview.chromium.org/2313623002/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5460: if (IsMipsArchVariant(kMips32r2))
On 2016/09/13 07:44:20, ivica.bogosavljevic wrote:
> You need to add braces {} here

I agree. And bellow and for all the multi line `if`s

[Ilija.Pavlovic1, 2016-09-13 12:57:57]

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

https://codereview.chromium.org/2313623002/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5426: if (std::is_same<T, float>::value) {
On 2016/09/13 07:44:20, ivica.bogosavljevic wrote:
> Could you use if (sizeof(T) == 4) else if (sizeof(T) == 8) instead of this
type
> check?

These kind of checkings will remain in the code.

Ack.

https://codereview.chromium.org/2313623002/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5460: if (IsMipsArchVariant(kMips32r2))
On 2016/09/13 10:20:32, balazs.kilvady wrote:
> On 2016/09/13 07:44:20, ivica.bogosavljevic wrote:
> > You need to add braces {} here
> 
> I agree. And bellow and for all the multi line `if`s

Done.

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

https://codereview.chromium.org/2313623002/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:6008: if (kArchVariant != kMips64r6)
On 2016/09/13 07:44:20, ivica.bogosavljevic wrote:
> Same as test-assembler-mips.cc

Done.

[balazs.kilvady, 2016-09-14 09:12:01]

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

https://codereview.chromium.org/2313623002/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5426: if (std::is_same<T, float>::value) {
On 2016/09/13 12:57:57, Ilija.Pavlovic1 wrote:
> On 2016/09/13 07:44:20, ivica.bogosavljevic wrote:
> > Could you use if (sizeof(T) == 4) else if (sizeof(T) == 8) instead of this
> type
> > check?
> 
> These kind of checkings will remain in the code.
> 
> Ack.
You could add else { UNREACHABLE(); } if you don't use (d)CHECK();

Done.

[balazs.kilvady, 2016-09-14 10:43:43]

LGTM

[Ilija.Pavlovic1, 2016-09-14 11:09:04]

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

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

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-14 11:36:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-14 11:37:22]

Description was changed from

==========
MIPS: Implement MADD.S, MSUB, MADDF and MSUBF.

Implementation MADD.S. MSUB.fmt, MADDF.fmt, MSUBF.fmt and corresponding
tests for assembler and disassembler.

TEST=cctest/test-assembler-mips[64], cctest/test-disasm-mips[64]
BUG=
==========

to

==========
MIPS: Implement MADD.S, MSUB, MADDF and MSUBF.

Implementation MADD.S. MSUB.fmt, MADDF.fmt, MSUBF.fmt and corresponding
tests for assembler and disassembler.

TEST=cctest/test-assembler-mips[64], cctest/test-disasm-mips[64]
BUG=

Committed: https://crrev.com/65fd5e1165104e387edf7348661c5f43ac954a52
Cr-Commit-Position: refs/heads/master@{#39415}
==========

[commit-bot: I haz the power, 2016-09-14 11:37:23]

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