MIPS: port ARM: Fix context save/restore for VFP registers.

MIPS: port ARM: Fix context save/restore for VFP registers.

This commit was missed/skipped earlier for some reason.

Ported r8357 (d78dae4)

BUG=
TEST=

Committed: http://code.google.com/p/v8/source/detail?r=9088

[Yang, 2011-08-31 12:38:44]

Hi Paul,

I noticed a few things. Please take another look.

Cheers,

-Yang

http://codereview.chromium.org/7809014/diff/1/src/mips/code-stubs-mips.cc
File src/mips/code-stubs-mips.cc (right):

http://codereview.chromium.org/7809014/diff/1/src/mips/code-stubs-mips.cc#new...
src/mips/code-stubs-mips.cc:3843: __ MultiPopFPU(kCalleeSavedFPU);
Shouldn't this be surrounded by the conditional "if
(CpuFeatures::IsSupported(FPU))"?

At least that's what's been done in r8357 and seems to make sense here as well.

http://codereview.chromium.org/7809014/diff/1/src/mips/frames-mips.h
File src/mips/frames-mips.h (right):

http://codereview.chromium.org/7809014/diff/1/src/mips/frames-mips.h#newcode67
src/mips/frames-mips.h:67: 1 << 20 | 1 << 22 | 1 << 24 | 1 << 26 | 1 << 28 | 1
<< 30;
Comments similar to those in frames-arm.h would be nice.

http://codereview.chromium.org/7809014/diff/1/src/mips/macro-assembler-mips.cc
File src/mips/macro-assembler-mips.cc (right):

http://codereview.chromium.org/7809014/diff/1/src/mips/macro-assembler-mips.c...
src/mips/macro-assembler-mips.cc:766: }
Consider - for readability - introducing a stack_offset variable to keep track
where to store the next register. For example:

int16_t NumToPush = NumberOfBitsSet(regs);
int16_t stack_offset = NumToPush * 8;

addiu(sp, sp, -8 * NumToPush);
for (int16_t i = kNumRegisters; i > 0; i--) {
  if ((regs & (1 << i)) != 0) {
    stack_offset -= 8;
    sdc1(FPURegister::from_code(i),
         MemOperand(sp, stack_offset));
  }
}

Also consider using kDoubleSize instead of the constant 8.

Maybe those non-FPU MultiPush and MultiPop commands can be refactored as well?

[Paul Lind, 2011-09-01 06:50:27]

http://codereview.chromium.org/7809014/diff/1/src/mips/code-stubs-mips.cc
File src/mips/code-stubs-mips.cc (right):

http://codereview.chromium.org/7809014/diff/1/src/mips/code-stubs-mips.cc#new...
src/mips/code-stubs-mips.cc:3843: __ MultiPopFPU(kCalleeSavedFPU);
On 2011/08/31 12:38:44, Yang wrote:
> Shouldn't this be surrounded by the conditional "if
> (CpuFeatures::IsSupported(FPU))"?
> 
> At least that's what's been done in r8357 and seems to make sense here as
well.

Done.

http://codereview.chromium.org/7809014/diff/1/src/mips/frames-mips.h
File src/mips/frames-mips.h (right):

http://codereview.chromium.org/7809014/diff/1/src/mips/frames-mips.h#newcode67
src/mips/frames-mips.h:67: 1 << 20 | 1 << 22 | 1 << 24 | 1 << 26 | 1 << 28 | 1
<< 30;
On 2011/08/31 12:38:44, Yang wrote:
> Comments similar to those in frames-arm.h would be nice.

Done. Refactored the other RegLists too.

http://codereview.chromium.org/7809014/diff/1/src/mips/macro-assembler-mips.cc
File src/mips/macro-assembler-mips.cc (right):

http://codereview.chromium.org/7809014/diff/1/src/mips/macro-assembler-mips.c...
src/mips/macro-assembler-mips.cc:766: }
On 2011/08/31 12:38:44, Yang wrote:
> Consider - for readability - introducing a stack_offset variable to keep track
> where to store the next register. For example:
> 
> int16_t NumToPush = NumberOfBitsSet(regs);
> int16_t stack_offset = NumToPush * 8;
> 
> addiu(sp, sp, -8 * NumToPush);
> for (int16_t i = kNumRegisters; i > 0; i--) {
>   if ((regs & (1 << i)) != 0) {
>     stack_offset -= 8;
>     sdc1(FPURegister::from_code(i),
>          MemOperand(sp, stack_offset));
>   }
> }
> 
> Also consider using kDoubleSize instead of the constant 8.
> 
> Maybe those non-FPU MultiPush and MultiPop commands can be refactored as well?

Nice suggestion, the code looks much cleaner now. I updated all the Multi's ...

BTW, mips does not have a subiu instruction. The addiu takes a signed 16-bit
immediate, so it can be used for subtract. I changed to use the Subu()
macro-instruction, which emits addiu with negative immediate value, to make it
more obvious in the code we are subtracting (more obvious than - sign.)

[Yang, 2011-09-01 07:37:41]

On 2011/09/01 06:50:27, Paul Lind wrote:
> http://codereview.chromium.org/7809014/diff/1/src/mips/code-stubs-mips.cc
> File src/mips/code-stubs-mips.cc (right):
> 
>
http://codereview.chromium.org/7809014/diff/1/src/mips/code-stubs-mips.cc#new...
> src/mips/code-stubs-mips.cc:3843: __ MultiPopFPU(kCalleeSavedFPU);
> On 2011/08/31 12:38:44, Yang wrote:
> > Shouldn't this be surrounded by the conditional "if
> > (CpuFeatures::IsSupported(FPU))"?
> > 
> > At least that's what's been done in r8357 and seems to make sense here as
> well.
> 
> Done.
> 
> http://codereview.chromium.org/7809014/diff/1/src/mips/frames-mips.h
> File src/mips/frames-mips.h (right):
> 
> http://codereview.chromium.org/7809014/diff/1/src/mips/frames-mips.h#newcode67
> src/mips/frames-mips.h:67: 1 << 20 | 1 << 22 | 1 << 24 | 1 << 26 | 1 << 28 | 1
> << 30;
> On 2011/08/31 12:38:44, Yang wrote:
> > Comments similar to those in frames-arm.h would be nice.
> 
> Done. Refactored the other RegLists too.
> 
> http://codereview.chromium.org/7809014/diff/1/src/mips/macro-assembler-mips.cc
> File src/mips/macro-assembler-mips.cc (right):
> 
>
http://codereview.chromium.org/7809014/diff/1/src/mips/macro-assembler-mips.c...
> src/mips/macro-assembler-mips.cc:766: }
> On 2011/08/31 12:38:44, Yang wrote:
> > Consider - for readability - introducing a stack_offset variable to keep
track
> > where to store the next register. For example:
> > 
> > int16_t NumToPush = NumberOfBitsSet(regs);
> > int16_t stack_offset = NumToPush * 8;
> > 
> > addiu(sp, sp, -8 * NumToPush);
> > for (int16_t i = kNumRegisters; i > 0; i--) {
> >   if ((regs & (1 << i)) != 0) {
> >     stack_offset -= 8;
> >     sdc1(FPURegister::from_code(i),
> >          MemOperand(sp, stack_offset));
> >   }
> > }
> > 
> > Also consider using kDoubleSize instead of the constant 8.
> > 
> > Maybe those non-FPU MultiPush and MultiPop commands can be refactored as
well?
> 
> Nice suggestion, the code looks much cleaner now. I updated all the Multi's
...
> 
> BTW, mips does not have a subiu instruction. The addiu takes a signed 16-bit
> immediate, so it can be used for subtract. I changed to use the Subu()
> macro-instruction, which emits addiu with negative immediate value, to make it
> more obvious in the code we are subtracting (more obvious than - sign.)

LGTM and landing.

Though it worries me a little bit that there does not seem to be any test
coverage for CPU features. Is it possible to emulate those in the simulator?