And here are the comments.
http://codereview.chromium.org/1320006/diff/35001/36005 File src/mips/assembler-mips.cc (right): http://codereview.chromium.org/1320006/diff/35001/36005#newcode56 src/mips/assembler-mips.cc:56: supported_ |= 1u << FPU; Only 2 space indent. http://codereview.chromium.org/1320006/diff/35001/36005#newcode975 src/mips/assembler-mips.cc:975: // load to two 32-bit loads. This really should be done in macro-assembler, I think you should just remove "This really should be done in macro-assembler, but this should be temporary...." http://codereview.chromium.org/1320006/diff/35001/36005#newcode981 src/mips/assembler-mips.cc:981: // GenInstrImmediate(LDC1, src.rm(), fd, src.offset_); Please remove code in comments. http://codereview.chromium.org/1320006/diff/35001/36005#newcode992 src/mips/assembler-mips.cc:992: // store to two 32-bit stores. This really should be done in macro-assembler, Ditto. http://codereview.chromium.org/1320006/diff/35001/36005#newcode998 src/mips/assembler-mips.cc:998: // GenInstrImmediate(SDC1, src.rm(), fd, src.offset_); Ditto. http://codereview.chromium.org/1320006/diff/35001/36008 File src/mips/assembler-mips.h (right): http://codereview.chromium.org/1320006/diff/35001/36008#newcode142 src/mips/assembler-mips.h:142: void setcode(int f) { setcode -> set_code http://codereview.chromium.org/1320006/diff/35001/36003 File src/mips/codegen-mips.cc (right): http://codereview.chromium.org/1320006/diff/35001/36003#newcode1285 src/mips/codegen-mips.cc:1285: __ addiu(sp, sp, -StandardFrameConstants::kCArgsSlotsSize); Please add a comment on the usage of the branch delay slot and on the instruction where execution continues after return. http://codereview.chromium.org/1320006/diff/35001/36006 File src/mips/constants-mips.h (right): http://codereview.chromium.org/1320006/diff/35001/36006#newcode597 src/mips/constants-mips.h:597: static const int kBArgsSlotsSize = 0 * Instruction::kInstructionSize; DO you need a separate constant for builtins? Aren't they always called like JavaScript? http://codereview.chromium.org/1320006/diff/35001/36022 File src/mips/frames-mips.h (right): http://codereview.chromium.org/1320006/diff/35001/36022#newcode125 src/mips/frames-mips.h:125: // C/C++ argument slots size. Aren't these constants duplicated here and in constants-mips.h? http://codereview.chromium.org/1320006/diff/35001/36004 File src/mips/macro-assembler-mips.cc (right): http://codereview.chromium.org/1320006/diff/35001/36004#newcode55 src/mips/macro-assembler-mips.cc:55: bool ProtectBranchDelaySlot) { \ ProtectBranchDelaySlot -> protect_branch_delay_slot http://codereview.chromium.org/1320006/diff/35001/36004#newcode142 src/mips/macro-assembler-mips.cc:142: Branch(2, NegateCondition(cond), src1, src2); Assert that lw generates 2 instructions? Or use a label? http://codereview.chromium.org/1320006/diff/35001/36004#newcode525 src/mips/macro-assembler-mips.cc:525: if (ProtectBranchDelaySlot) Either use {}'s or place the nop() on the same line as the if. http://codereview.chromium.org/1320006/diff/35001/36004#newcode559 src/mips/macro-assembler-mips.cc:559: // instruction, as the location will be remember for patching the target. remember -> remembered http://codereview.chromium.org/1320006/diff/35001/36004#newcode860 src/mips/macro-assembler-mips.cc:860: Branch(2, NegateCondition(cond), rs, rt); Please use use either labels to jump or assert the number of instructions generated in some way. Several places below. http://codereview.chromium.org/1320006/diff/35001/36004#newcode1214 src/mips/macro-assembler-mips.cc:1214: addiu(sp, sp, -StandardFrameConstants::kBArgsSlotsSize); Please add a comment that the second addiu will be executed after return. http://codereview.chromium.org/1320006/diff/35001/36004#newcode1224 src/mips/macro-assembler-mips.cc:1224: addiu(sp, sp, -StandardFrameConstants::kBArgsSlotsSize); Ditto. http://codereview.chromium.org/1320006/diff/35001/36020 File src/mips/macro-assembler-mips.h (right): http://codereview.chromium.org/1320006/diff/35001/36020#newcode46 src/mips/macro-assembler-mips.h:46: // Unless we know exactly what we do. Therefore we create another scratch reg. Remove "Unless we know exactly what we do."? http://codereview.chromium.org/1320006/diff/35001/36020#newcode65 src/mips/macro-assembler-mips.h:65: // Arguments macros Are you sure that using these macros here are beneficial? http://codereview.chromium.org/1320006/diff/35001/36020#newcode245 src/mips/macro-assembler-mips.h:245: Branch(3, cond, tst1, Operand(tst2)); The constant 3 relies on Addu only generates one instruction. Could that be asserted? http://codereview.chromium.org/1320006/diff/35001/36020#newcode329 src/mips/macro-assembler-mips.h:329: // Clobber t0, t1, t2. Clobber -> Clobbers http://codereview.chromium.org/1320006/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
