MIPS: Fix testcases r6_beqzc and mov.

MIPS: Fix testcases r6_beqzc and mov.

Remove incorrect usage of callee-saved FPU regs (f20 and above).
Also remove unnecessary push/pop which were occasionally unpaired,
and caused crash.

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

Committed: https://crrev.com/863ff3e3dd1796072f48c2675aae0dddecbd2163
Cr-Commit-Position: refs/heads/master@{#30729}

[Ilija.Pavlovic1, 2015-09-11 13:01:19]

ilija.pavlovic@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com, balazs.kilvady@imgtec.com, paul.lind@imgtec.com

[paul.l..., 2015-09-11 13:58:19]

Can you please update the commit message to be more descriptive. This line
"Adapted testcases for instructions beqzc, mov_s and mov_d." doesn't give any
information that I can't see from the diff.

Maybe make that 2nd line: "Remove incorrect usage of callee-saved FPU regs (f20
and above). Also remove unnecessary push/pop which were occasionally unpaired,
and caused crash."

Basically we should convey the 'why' (without writing a novel :-)

[petarj, 2015-09-11 14:02:27]

petarj@mips.com changed reviewers:
+ petarj@mips.com

[petarj, 2015-09-11 14:02:28]

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

https://codereview.chromium.org/1338713004/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:2709: __ ldc1(f5, MemOperand(a0,
offsetof(TestFloat, a)) );
Is it possible for this test to be run in fp32 mode? In case it is, you can not
use odd registers.

https://codereview.chromium.org/1338713004/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:2712: __ mov_d(f17, f5);
Same here.

https://codereview.chromium.org/1338713004/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-mips.cc:2714: __ sdc1(f17, MemOperand(a0,
offsetof(TestFloat, b)) );
Ditto.

[petarj, 2015-09-11 14:22:00]

One more thing/nit...

I notice that in assembler-mips.cc and in assembler-mips64.cc
we have the following code:

void Assembler::mov_d(FPURegister fd, FPURegister fs) {
  GenInstrRegister(COP1, D, f0, fs, fd, MOV_S);
}

void Assembler::mov_s(FPURegister fd, FPURegister fs) {
  GenInstrRegister(COP1, S, f0, fs, fd, MOV_D);
}

Shouldn't enums MOV_S and MOV_D be swapped here? They have the same
values, so it does not create any problems, but it may be worth changing
it as it makes the code more understandable and you are already fixing
this.

[Ilija.Pavlovic1, 2015-09-14 13:09:56]

On 2015/09/11 13:58:19, paul.l... wrote:
> Can you please update the commit message to be more descriptive. This line
> "Adapted testcases for instructions beqzc, mov_s and mov_d." doesn't give any
> information that I can't see from the diff.
> 
> Maybe make that 2nd line: "Remove incorrect usage of callee-saved FPU regs
(f20
> and above). Also remove unnecessary push/pop which were occasionally unpaired,
> and caused crash."
> 
> Basically we should convey the 'why' (without writing a novel :-)

Done.

[Ilija.Pavlovic1, 2015-09-14 13:14:52]

On 2015/09/11 14:02:28, petarj wrote:
>
https://codereview.chromium.org/1338713004/diff/1/test/cctest/test-assembler-...
> File test/cctest/test-assembler-mips.cc (right):
> 
>
https://codereview.chromium.org/1338713004/diff/1/test/cctest/test-assembler-...
> test/cctest/test-assembler-mips.cc:2709: __ ldc1(f5, MemOperand(a0,
> offsetof(TestFloat, a)) );
> Is it possible for this test to be run in fp32 mode? In case it is, you can
not
> use odd registers.
> 
>
https://codereview.chromium.org/1338713004/diff/1/test/cctest/test-assembler-...
> test/cctest/test-assembler-mips.cc:2712: __ mov_d(f17, f5);
> Same here.
> 
>
https://codereview.chromium.org/1338713004/diff/1/test/cctest/test-assembler-...
> test/cctest/test-assembler-mips.cc:2714: __ sdc1(f17, MemOperand(a0,
> offsetof(TestFloat, b)) );
> Ditto.

Following adaptations are done: f5 substituted with f4, f17 substituted with
f10, f18 substituted with f8.

Done.

[Ilija.Pavlovic1, 2015-09-14 13:15:24]

On 2015/09/11 14:22:00, petarj wrote:
> One more thing/nit...
> 
> I notice that in assembler-mips.cc and in assembler-mips64.cc
> we have the following code:
> 
> void Assembler::mov_d(FPURegister fd, FPURegister fs) {
>   GenInstrRegister(COP1, D, f0, fs, fd, MOV_S);
> }
> 
> void Assembler::mov_s(FPURegister fd, FPURegister fs) {
>   GenInstrRegister(COP1, S, f0, fs, fd, MOV_D);
> }
> 
> Shouldn't enums MOV_S and MOV_D be swapped here? They have the same
> values, so it does not create any problems, but it may be worth changing
> it as it makes the code more understandable and you are already fixing
> this.

Done.

[paul.l..., 2015-09-14 14:58:50]

LGTM.

[Ilija.Pavlovic1, 2015-09-15 07:12:37]

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

[commit-bot: I haz the power, 2015-09-15 07:12:48]

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

[commit-bot: I haz the power, 2015-09-15 07:37:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-15 07:37:28]

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