Next round of comments. I still have to review macro-assembler-mips and simulator-mips.
http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl.h File src/mips/assembler-mips-inl.h (right): http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl.h#newcode147 src/mips/assembler-mips-inl.h:147: // Commenting out, to simplify arch-independednt changes. I suggest adding a TODO(mips) to the beginning of this comment for reference. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl.h#newcode164 src/mips/assembler-mips-inl.h:164: // Commenting out, to simplify arch-independednt changes. Ditto. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl.h#newcode199 src/mips/assembler-mips-inl.h:199: // The pc_ offset of 0 assumes mips patched return sequence per patched return sequence -> patched return sequence or patched debug break slot. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips-inl.h#newcode208 src/mips/assembler-mips-inl.h:208: // The pc_ offset of 0 assumes mips patched return sequence per Ditto. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc File src/mips/assembler-mips.cc (right): http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#newcode235 src/mips/assembler-mips.cc:235: static const int kMinimalBufferSize = 4*KB; Spaces around binary operations. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#newcode409 src/mips/assembler-mips.cc:409: bool ret = (opcode == SLL && Indention off - indent to parentesis. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#newcode438 src/mips/assembler-mips.cc:438: // We actually create a new lw instruction based on the original one. Format this Instr temp_instr = LW | (instr & kRsFieldMask) | (instr & kRtFieldMask) | (offset & kImm16Mask); as Instr temp_instr = LW | (instr & kRsFieldMask) | (instr & kRtFieldMask) | (offset & kImm16Mask); if it fits in the 80 chars or Instr temp_instr = LW | (instr & kRsFieldMask) | (instr & kRtFieldMask) | (offset & kImm16Mask); if not. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#newcode544 src/mips/assembler-mips.cc:544: ASSERT(0 <= pos && pos <= pc_offset()); // must have a valid binding position Case + full stop in comment. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#newcode884 src/mips/assembler-mips.cc:884: BlockTrampolinePoolFor(1); This additional blocking for one instruction is that for the delay slot? If a BlockTrampolinePoolScope always blocks for one instruction after the scope maybe that should be part of the BlockTrampolinePoolScope. I see this same pattern below. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#newcode1242 src/mips/assembler-mips.cc:1242: ASSERT(rd.is_valid() && rt.is_valid() && is_uint5(sa)); Please change pattern if (mips32r2) { ... something ... } else { ASSERT(mips32r2) } to ASSERT(mips32r2) ... something ... It is used a number of times below. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#newcode1272 src/mips/assembler-mips.cc:1272: } else { // Offset > 16 bits, use multiple instructions to load. Consider refactoring ASSERT(!rs.rm().is(at)); lui(at, rs.offset_ >> 16); ori(at, at, rs.offset_ & 0xffff); addu(at, at, rs.rm()); into a helper function. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#newcode1274 src/mips/assembler-mips.cc:1274: lui(at, rs.offset_ >> 16); Constants for 16 and 0xffff? http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#newcode1567 src/mips/assembler-mips.cc:1567: rt.code_ = (cc & 0x0003)<<2 | 1; Spaces around << (more below). http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#newcode1588 src/mips/assembler-mips.cc:1588: // Ins instr has 'rt' field as dest, and two uint5: msb, lsb End comment with full stop. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h File src/mips/assembler-mips.h (right): http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#newcode172 src/mips/assembler-mips.h:172: // Currently f0 is used as a scratch register. This comment about f2 does not seems to be what the code below actually does. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#newcode237 src/mips/assembler-mips.h:237: const FPURegister f0 = { 0 }; // return value in hard float mode Start comment with uppercase and end with full stop. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#newcode249 src/mips/assembler-mips.h:249: const FPURegister f12 = { 12 }; // arg in hard float mode arg -> arg 0? Start comment with uppercase and end with full stop. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#newcode251 src/mips/assembler-mips.h:251: const FPURegister f14 = { 14 }; // arg in hard float mode arg -> arg 1? Start comment with uppercase and end with full stop. http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#newcode486 src/mips/assembler-mips.h:486: // These values are used in serialization process and must be zero for in -> in the http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#newcode998 src/mips/assembler-mips.h:998: next_label_ = start + slot_count*2*kInstrSize; Spaces between binary operations (more below). http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#newcode1002 src/mips/assembler-mips.h:1002: int bound() { If this function is just an accessor for next_slot_ shouldn't it be called next_slot instead of bound? http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.h#newcode1039 src/mips/assembler-mips.h:1039: static const int kTrampolineInst = Only 4 space indent of line starting with "2 *". http://codereview.chromium.org/6709022/diff/5001/src/mips/disasm-mips.cc File src/mips/disasm-mips.cc (right): http://codereview.chromium.org/6709022/diff/5001/src/mips/disasm-mips.cc#newcode211 src/mips/disasm-mips.cc:211: out_buffer_pos_ += OS::SNPrintF(out_buffer_ + out_buffer_pos_, Indentation. More below. Some of these fit one line. http://codereview.chromium.org/6709022/diff/5001/src/mips/disasm-mips.cc#newcode513 src/mips/disasm-mips.cc:513: } End case with {}'s like this: break; } instead of } break; http://codereview.chromium.org/6709022/diff/5001/src/mips/frames-mips.h File src/mips/frames-mips.h (right): http://codereview.chromium.org/6709022/diff/5001/src/mips/frames-mips.h#newcode109 src/mips/frames-mips.h:109: // TODO(regis): Use a patched sp value on the stack instead. regis -> mips http://codereview.chromium.org/6709022/diff/5001/test/cctest/test-assembler-mips.cc File test/cctest/test-assembler-mips.cc (right): http://codereview.chromium.org/6709022/diff/5001/test/cctest/test-assembler-mips.cc#newcode331 test/cctest/test-assembler-mips.cc:331: #ifdef DEBUG Do you want to have this printing? http://codereview.chromium.org/6709022/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
