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