Another round of comments, mainly on the MIPS assembler.
http://codereview.chromium.org/543161/diff/4001/5005 File src/bootstrapper.cc (right): http://codereview.chromium.org/543161/diff/4001/5005#newcode901 src/bootstrapper.cc:901: // Call function using either the runtime object or the global Please revert this to remove bootstrapper.cc from this change. I will fix this on bleeding_edge. http://codereview.chromium.org/543161/diff/4001/5052 File src/execution.cc (right): http://codereview.chromium.org/543161/diff/4001/5052#newcode41 src/execution.cc:41: #elif V8_TARGET_ARCH_MIPS In bleeding_edge this #ifdef block have been moved to "simulator.h" http://codereview.chromium.org/543161/diff/4001/5044 File src/mips/assembler-mips.cc (right): http://codereview.chromium.org/543161/diff/4001/5044#newcode1049 src/mips/assembler-mips.cc:1049: // Arithmetic These arithmetic instructions should be split into assembler and macro-assembler versions. E.g. in assembler the addi instruction should ASSERT that the immediate is 16-bit and just emmit a ADDI instruction. The macro-assembler should then have an Add macro taking a 32-bit immediate. http://codereview.chromium.org/543161/diff/4001/5044#newcode1303 src/mips/assembler-mips.cc:1303: Instr instr = SPECIAL | TGE | rs.code()<<21 | rt.code()<<16 | code<<6; Why not use the kXxxShift constants from constants-mips.h here and below. http://codereview.chromium.org/543161/diff/4001/5044#newcode1408 src/mips/assembler-mips.cc:1408: void Assembler::ldc1(CRegister fd, const MemOperand& src) { Another macro-assembler macro. http://codereview.chromium.org/543161/diff/4001/5044#newcode1443 src/mips/assembler-mips.cc:1443: void Assembler::sdc1(CRegister fd, const MemOperand& src) { Ditto. http://codereview.chromium.org/543161/diff/4001/5044#newcode1568 src/mips/assembler-mips.cc:1568: //------------Pseudo-instructions------------- Pseudo-instructions are macros for the macro-assembler. http://codereview.chromium.org/543161/diff/4001/5044#newcode1861 src/mips/assembler-mips.cc:1861: // nop(); Code in comments. http://codereview.chromium.org/543161/diff/4001/5048 File src/mips/assembler-mips.h (right): http://codereview.chromium.org/543161/diff/4001/5048#newcode132 src/mips/assembler-mips.h:132: struct CRegister { Rename CRegister to FPURegister like in simulator. http://codereview.chromium.org/543161/diff/4001/5048#newcode402 src/mips/assembler-mips.h:402: // instruction. I think these blcond methods should be moved to macro-assembler-mips.* as it is not MIPS instructions. I suggest the name Branch. http://codereview.chromium.org/543161/diff/4001/5048#newcode562 src/mips/assembler-mips.h:562: void push(Register src) { Move to macro-assembler as Push. http://codereview.chromium.org/543161/diff/4001/5048#newcode567 src/mips/assembler-mips.h:567: void push(Register src, Condition cond, Register tst1, Register tst2) { Ditto. http://codereview.chromium.org/543161/diff/4001/5048#newcode579 src/mips/assembler-mips.h:579: void pop(Register dst) { Ditto. http://codereview.chromium.org/543161/diff/4001/5048#newcode583 src/mips/assembler-mips.h:583: void pop() { Ditto. http://codereview.chromium.org/543161/diff/4001/5048#newcode598 src/mips/assembler-mips.h:598: void jmp(Label* L) { Ditto. http://codereview.chromium.org/543161/diff/4001/5048#newcode701 src/mips/assembler-mips.h:701: // Anyway we could surely implement this differently. Consider using the names Immediate, Jump and Register instead of 1, 2 and 3. http://codereview.chromium.org/543161/diff/4001/5046 File src/mips/constants-mips.h (right): http://codereview.chromium.org/543161/diff/4001/5046#newcode94 src/mips/constants-mips.h:94: class CRegisters { Please rename CRegisters to FPURegisters http://codereview.chromium.org/543161/diff/4001/5046#newcode130 src/mips/constants-mips.h:130: static const int opcode_o = 26; Please use the kXxxYyy naming of constants, e.g. opcode_o -> kOpcodeShift opcode_l -> kOpcodeBits and change the type from int to uint32_t. static const uint32_t kOpcodeBits = 6; static const uint32_t kOpcodeShift = 21; Also move enum constants below here. e.g. static const uint32_t kOpcodeMask = ((1 << kOpcodeBits) - 1) << kOpcodeShift; Also you might calculate each shift value by adding the bits of the previous field. http://codereview.chromium.org/543161/diff/4001/5046#newcode277 src/mips/constants-mips.h:277: enum { Why are these constants in an enum? How about moving them up to the constants above? http://codereview.chromium.org/543161/diff/4001/5046#newcode296 src/mips/constants-mips.h:296: // the 'U' prefix is used to specify unsigned comparisons. I think the U prefix should be unsigned_ instead. As an alternative you could consider he Intel naming, where less and greater are unsigned and above and below are signed - even though that can also cause confusion. http://codereview.chromium.org/543161/diff/4001/5046#newcode334 src/mips/constants-mips.h:334: enum C_Condition { I suggest renaming C_Condition to FPUCondition. http://codereview.chromium.org/543161/diff/4001/5046#newcode355 src/mips/constants-mips.h:355: // bool InstructionSetsConditionCodes(byte_* ptr) { Please update this code sample - the HasS method is not part of this class. Maybe just remove it. http://codereview.chromium.org/543161/diff/4001/5046#newcode400 src/mips/constants-mips.h:400: inline int rsField() const { Rename rsField -> RsField - and for the other methods below. Maybe ASSERT the instruction type is what is expected. e.g. for this method ASSERT instruction is immediate or register. http://codereview.chromium.org/543161/diff/4001/5046#newcode469 src/mips/constants-mips.h:469: inline int32_t Imm16Field() const { Maybe rename Imm16Field -> ImmediateField? Maybe ASSERT type is ImmediateInstructionType. http://codereview.chromium.org/543161/diff/4001/5046#newcode473 src/mips/constants-mips.h:473: inline int32_t Imm26Field() const { Maybe rename Imm26Field to InstructionIndexField. Maybe ASSERT type is JumpInstructionType. http://codereview.chromium.org/543161/diff/4001/5046#newcode484 src/mips/constants-mips.h:484: // Get the encoding type of the instruction. How about changing the type into an enum with the values kImmediateType kJumpType kRegisterType or some other names which provide more info than 1,2 and 3? http://codereview.chromium.org/543161/diff/4001/5046#newcode506 src/mips/constants-mips.h:506: Name constants like kXxxYyy. http://codereview.chromium.org/543161 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
