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#newcode3843
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.cc#newcode766
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?
http://codereview.chromium.org/7809014/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
