MIPS: Add rounding support in simulator and RINT instruction.

MIPS: Add rounding support in simulator and RINT instruction.

Added rounding according to fcsr, CVT_W_D and RINT.D instruction in assembler, dissasembler and simulator and wrote appropiate tests.

BUG=

Committed: https://crrev.com/9da34c56a153a0da7dbb16a24c9d0dfed1925336
Cr-Commit-Position: refs/heads/master@{#28143}

[balazs.kilvady, 2015-04-24 15:38:15]

balazs.kilvady@imgtec.com changed reviewers:
+ balazs.kilvady@imgtec.com

[balazs.kilvady, 2015-04-24 15:38:16]

Looks good. I see SecondaryField param is used in min, max too, but it is a nit
for me:

https://codereview.chromium.org/1108583003/diff/1/src/mips/assembler-mips.h
File src/mips/assembler-mips.h (right):

https://codereview.chromium.org/1108583003/diff/1/src/mips/assembler-mips.h#n...
src/mips/assembler-mips.h:915: void rint(SecondaryField fmt, FPURegister fd,
FPURegister fs);
I think rint_s() and rint_d() should be implemented instead of using
SecondaryField param to match to all the other instructions.

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

https://codereview.chromium.org/1108583003/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64.h:945: void rint(SecondaryField fmt, FPURegister fd,
FPURegister fs);
I think rint_s() and rint_d() should be implemented instead of using
SecondaryField param to match to all the other instructions.

[paul.l..., 2015-04-27 05:12:49]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[paul.l..., 2015-04-27 05:12:50]

This looks very nice Djordje. I really like the thorough tests you've created!
Do these pass on qemu?

https://codereview.chromium.org/1108583003/diff/1/src/mips/assembler-mips.h
File src/mips/assembler-mips.h (right):

https://codereview.chromium.org/1108583003/diff/1/src/mips/assembler-mips.h#n...
src/mips/assembler-mips.h:915: void rint(SecondaryField fmt, FPURegister fd,
FPURegister fs);
On 2015/04/24 15:38:16, balazs.kilvady wrote:
> I think rint_s() and rint_d() should be implemented instead of using
> SecondaryField param to match to all the other instructions.

I agree with Balázs here, however this is then different than how Dusan
implemented the new min/mina/max/maxa in 4b5af7b3. Dusan what is your reasoning
for the change in format there?

We should reach consensus on this before you make a bunch of changes. If we
agree to separate _D, _S  instructions, then we should do the same for min/max
in a follow-on CL.

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

https://codereview.chromium.org/1108583003/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1275: void
Simulator::set_fcsr_rounding_mode(FPURoundingMode mode) { FCSR_ |= mode; }
Maybe you should ensure this call does not trash other bits, by doing FCSR_ |=
mode & 3;
In that case, you should probably make a constant kFPURoundingModeMask and use
it here, and just below.

https://codereview.chromium.org/1108583003/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1317: // two representable values, round to zero.
Behave like round_w_d
nit: period at end of this sentence, and 3 more below.

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

https://codereview.chromium.org/1108583003/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:1255: // for cvt instructions only
Style nit: Comments should start with capital letter, and end with period. (Here
and next f()).

Maybe the comment is unnecessary: I believe all you are saying is that CVT_W_D
use this, and the RINT instructions do not. it is clear in the calling
functions, and you don't repeat this in mips32 sim, so maybe just drop it.

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

https://codereview.chromium.org/1108583003/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:1567: for (int j = 0;j < 4;j++) {
Style nit: space after ';' in these 4 loops.

[dusmil.imgtec, 2015-04-27 10:02:52]

dusan.milosavljevic@imgtec.com changed reviewers:
+ dusan.milosavljevic@imgtec.com

[dusmil.imgtec, 2015-04-27 10:02:52]

https://codereview.chromium.org/1108583003/diff/1/src/mips/assembler-mips.h
File src/mips/assembler-mips.h (right):

https://codereview.chromium.org/1108583003/diff/1/src/mips/assembler-mips.h#n...
src/mips/assembler-mips.h:915: void rint(SecondaryField fmt, FPURegister fd,
FPURegister fs);
Yes, We need to have those additional wrappers suffixed with _D and _S.
It is easier to use them in code then setting secondary field or RS field every
time.

[akos.palfi.imgtec, 2015-04-27 10:38:24]

akos.palfi@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com

[Djordje.Pesic, 2015-04-27 12:46:59]

https://codereview.chromium.org/1108583003/diff/1/src/mips/assembler-mips.h
File src/mips/assembler-mips.h (right):

https://codereview.chromium.org/1108583003/diff/1/src/mips/assembler-mips.h#n...
src/mips/assembler-mips.h:915: void rint(SecondaryField fmt, FPURegister fd,
FPURegister fs);
On 2015/04/27 10:02:52, dusmil.imgtec wrote:
> Yes, We need to have those additional wrappers suffixed with _D and _S.
> It is easier to use them in code then setting secondary field or RS field
every
> time.
> 

Done.

https://codereview.chromium.org/1108583003/diff/1/src/mips/assembler-mips.h#n...
src/mips/assembler-mips.h:915: void rint(SecondaryField fmt, FPURegister fd,
FPURegister fs);
On 2015/04/27 05:12:49, paul.l... wrote:
> On 2015/04/24 15:38:16, balazs.kilvady wrote:
> > I think rint_s() and rint_d() should be implemented instead of using
> > SecondaryField param to match to all the other instructions.
> 
> I agree with Balázs here, however this is then different than how Dusan
> implemented the new min/mina/max/maxa in 4b5af7b3. Dusan what is your
reasoning
> for the change in format there?
> 
> We should reach consensus on this before you make a bunch of changes. If we
> agree to separate _D, _S  instructions, then we should do the same for min/max
> in a follow-on CL.

Done.

https://codereview.chromium.org/1108583003/diff/1/src/mips/assembler-mips.h#n...
src/mips/assembler-mips.h:915: void rint(SecondaryField fmt, FPURegister fd,
FPURegister fs);
On 2015/04/24 15:38:16, balazs.kilvady wrote:
> I think rint_s() and rint_d() should be implemented instead of using
> SecondaryField param to match to all the other instructions.

Done.

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

https://codereview.chromium.org/1108583003/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1275: void
Simulator::set_fcsr_rounding_mode(FPURoundingMode mode) { FCSR_ |= mode; }
On 2015/04/27 05:12:50, paul.l... wrote:
> Maybe you should ensure this call does not trash other bits, by doing FCSR_ |=
> mode & 3;
> In that case, you should probably make a constant kFPURoundingModeMask and use
> it here, and just below.

Done.

https://codereview.chromium.org/1108583003/diff/1/src/mips/simulator-mips.cc#...
src/mips/simulator-mips.cc:1317: // two representable values, round to zero.
Behave like round_w_d
On 2015/04/27 05:12:49, paul.l... wrote:
> nit: period at end of this sentence, and 3 more below.

Done.

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

https://codereview.chromium.org/1108583003/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64.h:945: void rint(SecondaryField fmt, FPURegister fd,
FPURegister fs);
On 2015/04/24 15:38:16, balazs.kilvady wrote:
> I think rint_s() and rint_d() should be implemented instead of using
> SecondaryField param to match to all the other instructions.

Done.

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

https://codereview.chromium.org/1108583003/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:1255: // for cvt instructions only
On 2015/04/27 05:12:50, paul.l... wrote:
> Style nit: Comments should start with capital letter, and end with period.
(Here
> and next f()).
> 
> Maybe the comment is unnecessary: I believe all you are saying is that CVT_W_D
> use this, and the RINT instructions do not. it is clear in the calling
> functions, and you don't repeat this in mips32 sim, so maybe just drop it.
I've dropped the comment

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

https://codereview.chromium.org/1108583003/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:1567: for (int j = 0;j < 4;j++) {
On 2015/04/27 05:12:50, paul.l... wrote:
> Style nit: space after ';' in these 4 loops.

Done.

[Djordje.Pesic, 2015-04-28 10:06:10]

On 2015/04/27 05:12:50, paul.l... wrote:
> This looks very nice Djordje. I really like the thorough tests you've created!
> Do these pass on qemu?
> 
Yes, these tests pass on qemu.

[paul.l..., 2015-04-28 14:18:13]

l-g-t-m, just waiting on CLA.

[paul.l..., 2015-04-29 15:29:20]

paul.lind@imgtec.com changed reviewers:
+ danno@chromium.org, machenbach@chromium.org

[paul.l..., 2015-04-29 15:29:21]

LGTM. The CLA is in, go ahead and commit. Thanks!

[Djordje.Pesic, 2015-04-30 06:04:14]

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

[commit-bot: I haz the power, 2015-04-30 06:04:28]

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

[commit-bot: I haz the power, 2015-04-30 06:29:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-30 06:29:27]

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