Yet another round of comments. When they have been addressed there should
not be
much more to do before this can be committed.
http://codereview.chromium.org/543161/diff/9055/9072
File src/mips/assembler-mips-inl.h (right):
http://codereview.chromium.org/543161/diff/9055/9072#newcode76
src/mips/assembler-mips-inl.h:76: Operand::Operand(Object** opp) {
I don't think this one is needed. It is there for ARM, but is not used
and should probably be removed.
http://codereview.chromium.org/543161/diff/9055/9072#newcode82
src/mips/assembler-mips-inl.h:82: Operand::Operand(Context** cpp) {
I don't think this one is needed. It is there for ARM, but is not used
and should probably be removed.
http://codereview.chromium.org/543161/diff/9055/9089
File src/mips/assembler-mips.cc (right):
http://codereview.chromium.org/543161/diff/9055/9089#newcode1138
src/mips/assembler-mips.cc:1138: ((instr1 == 0) && ((instr2 &
kOpcodeMask) == ADDI ||
Please add a constant for no operation.
http://codereview.chromium.org/543161/diff/9055/9089#newcode1142
src/mips/assembler-mips.cc:1142: if (instr1 == 0) {
Ditto.
http://codereview.chromium.org/543161/diff/9055/9089#newcode1144
src/mips/assembler-mips.cc:1144: return (Address)(((instr2 & kImm16Mask)
<< 16)>>16);
Change to C++ style cast. 4 concurrences below.
http://codereview.chromium.org/543161/diff/9055/9089#newcode1166
src/mips/assembler-mips.cc:1166: // instr1 would be unused.
Using macros here is a bit obscure. We have the USE macro in globals.h
to help here. Alternatively use a
Instr instr2 = ...
#ifdef DEBUG
Instr instr1 = ...
CHECK(...)
#endif
We normally change ASSERT to CHECK when inside an #ifdef DEBUG/#endif,
but that is not that important.
http://codereview.chromium.org/543161/diff/9055/9089#newcode1179
src/mips/assembler-mips.cc:1179: uint32_t itarget = (uint32_t)(target);
C style case.
http://codereview.chromium.org/543161/diff/9055/9089#newcode1184
src/mips/assembler-mips.cc:1184: *p = 0x0;
Please add a constant for no operation.
http://codereview.chromium.org/543161/diff/9055/9089#newcode1189
src/mips/assembler-mips.cc:1189: *p = 0x0;
Ditto.
http://codereview.chromium.org/543161/diff/9055/9093
File src/mips/assembler-mips.h (right):
http://codereview.chromium.org/543161/diff/9055/9093#newcode367
src/mips/assembler-mips.h:367: #define DEFINE_INSTRUCTION_R_(instr)
\
Looks as if this macro is not used any more.
http://codereview.chromium.org/543161/diff/9055/9093#newcode374
src/mips/assembler-mips.h:374: #define DEFINE_INSTRUCTION_I_(instr)
\
Ditto.
http://codereview.chromium.org/543161/diff/9055/9093#newcode676
src/mips/assembler-mips.h:676: #undef DEFINE_INSTRUCTION_R_
Looks as if this macro is not used any more.
http://codereview.chromium.org/543161/diff/9055/9093#newcode677
src/mips/assembler-mips.h:677: #undef DEFINE_INSTRUCTION_I_
Ditto.
http://codereview.chromium.org/543161/diff/9055/9075
File src/mips/codegen-mips.cc (right):
http://codereview.chromium.org/543161/diff/9055/9075#newcode188
src/mips/codegen-mips.cc:188: UNIMPLEMENTED();
UNIMPLEMENTED_MIPS()
http://codereview.chromium.org/543161/diff/9055/9075#newcode224
src/mips/codegen-mips.cc:224: UNIMPLEMENTED();
UNIMPLEMENTED_MIPS()
http://codereview.chromium.org/543161/diff/9055/9075#newcode274
src/mips/codegen-mips.cc:274: UNIMPLEMENTED();
UNIMPLEMENTED_MIPS()
http://codereview.chromium.org/543161/diff/9055/9075#newcode289
src/mips/codegen-mips.cc:289: UNIMPLEMENTED();
UNIMPLEMENTED_MIPS()
http://codereview.chromium.org/543161/diff/9055/9091
File src/mips/constants-mips.h (right):
http://codereview.chromium.org/543161/diff/9055/9091#newcode34
src/mips/constants-mips.h:34: #ifdef DEBUG
Maybe define these macros to print in release mode as well.
http://codereview.chromium.org/543161/diff/9055/9091#newcode123
src/mips/constants-mips.h:123: typedef unsigned char byte_;
Why the trailing underscore? Is there a namespace problem? I can see in
constants-arm.h we have the same without the trailing underscore. Will
it be possible to reuse the byte typedef in globals.h?
http://codereview.chromium.org/543161/diff/9055/9091#newcode383
src/mips/constants-mips.h:383: enum IType {
Why not just Type? Is that not allowed?
http://codereview.chromium.org/543161/diff/9055/9091#newcode391
src/mips/constants-mips.h:391: IType instrType() const;
instrType -> InstructionType or just Type (which might conflict with the
enum above).
http://codereview.chromium.org/543161/diff/9055/9091#newcode462
src/mips/constants-mips.h:462: ASSERT(instrType() == kRegisterType ||
instrType() == kImmediateType);
On 2010/01/30 02:41:05, alexandre.rames wrote:
This ASSERT causes an infinite loop in debug mode as it calls
instrType() and
FunctionFieldRaw is also called by instrType().
I remove it for now.
Thats OK.
http://codereview.chromium.org/543161/diff/9055/9081
File src/mips/disasm-mips.cc (right):
http://codereview.chromium.org/543161/diff/9055/9081#newcode256
src/mips/disasm-mips.cc:256: v8i::OS::SNPrintF(out_buffer_ +
out_buffer_pos_, "0x%05x", code);
4 space indent.
http://codereview.chromium.org/543161/diff/9055/9081#newcode267
src/mips/disasm-mips.cc:267: v8i::OS::SNPrintF(out_buffer_ +
out_buffer_pos_, "0x%03x", code);
Ditto.
http://codereview.chromium.org/543161/diff/9055/9081#newcode543
src/mips/disasm-mips.cc:543: break;
You don't need 'break' in default. Also some places below.
http://codereview.chromium.org/543161/diff/9055/9080
File src/mips/frames-mips.h (right):
http://codereview.chromium.org/543161/diff/9055/9080#newcode41
src/mips/frames-mips.h:41: static const RegList kJSCallerSaved =
Is it possible to use the Register constants here, e.g. a0.bit() instead
of 4.
http://codereview.chromium.org/543161/diff/9055/9080#newcode57
src/mips/frames-mips.h:57: // Saved temporaries
Ditto.
http://codereview.chromium.org/543161/diff/9055/9088
File src/mips/macro-assembler-mips.cc (right):
http://codereview.chromium.org/543161/diff/9055/9088#newcode122
src/mips/macro-assembler-mips.cc:122: // Will clobber 4 registers:
object, offset, scratch, ip. The
You probably want to remove this comment as it will be difficult to know
how this will be implemented.
http://codereview.chromium.org/543161/diff/9055/9079
File src/mips/macro-assembler-mips.h (right):
http://codereview.chromium.org/543161/diff/9055/9079#newcode208
src/mips/macro-assembler-mips.h:208: void MultiPush_reversed(RegList
regs);
MultiPush_reversed -> MultiPushReversed
http://codereview.chromium.org/543161/diff/9055/9079#newcode226
src/mips/macro-assembler-mips.h:226: void MultiPop_reversed(RegList
regs);
MultiPop_reversed -> MultiPopReversed
http://codereview.chromium.org/543161/diff/9055/9079#newcode253
src/mips/macro-assembler-mips.h:253: Register scratch = at) {
Add ASSERT_EQ(0, kSmiTag).
http://codereview.chromium.org/543161/diff/9055/9079#newcode260
src/mips/macro-assembler-mips.h:260: Register scratch = at) {
Ditto.
http://codereview.chromium.org/543161/diff/9055/9092
File src/mips/simulator-mips.cc (right):
http://codereview.chromium.org/543161/diff/9055/9092#newcode53
src/mips/simulator-mips.cc:53: bool haveSameSign(int32_t a, int32_t b) {
Uppercase H.
http://codereview.chromium.org/543161/diff/9055/9092#newcode244
src/mips/simulator-mips.cc:244:
Please use static const int kXxxYyy for these constants.
http://codereview.chromium.org/543161/diff/9055/9092#newcode601
src/mips/simulator-mips.cc:601: ASSERT((fpureg >= 0) && (fpureg <
kNumFPURegisters) && (fpureg%2 == 0));
Is fpureg%2 == 0 in the ASSERT correct when just setting an 32-bit
value?
http://codereview.chromium.org/543161/diff/9055/9092#newcode606
src/mips/simulator-mips.cc:606: ASSERT((fpureg >= 0) && (fpureg <
kNumFPURegisters) && (fpureg%2 == 0));
Please format (fpureg%2 == 0) as ((fpureg % 2) == 0)
http://codereview.chromium.org/543161/diff/9055/9092#newcode607
src/mips/simulator-mips.cc:607:
*reinterpret_cast<double*>(&FPUregisters_[fpureg]) = value;
Would a bit_cast be better here?
http://codereview.chromium.org/543161/diff/9055/9092#newcode653
src/mips/simulator-mips.cc:653: if ((addr & 3) == 0) {
Please consider using kPointerAlignmentMask from globals.h instead of
the constant 3 (three times more below).
http://codereview.chromium.org/543161/diff/9055/9092#newcode675
src/mips/simulator-mips.cc:675: if ((addr & 3) == 0) {
Should this be 7 instead of 3? If so please add constants
kDoubleAlignmentBits, kDoubleAlignment and kDoubleAlignmentMask.
http://codereview.chromium.org/543161/diff/9055/9092#newcode686
src/mips/simulator-mips.cc:686: if ((addr & 3) == 0) {
Ditto.
http://codereview.chromium.org/543161/diff/9055/9092#newcode863
src/mips/simulator-mips.cc:863: // Instruction fields
Please fix the alignment of the following code - or get rid of it
altogether.
http://codereview.chromium.org/543161/diff/9055/9092#newcode912
src/mips/simulator-mips.cc:912: UNIMPLEMENTED_MIPS();
No break on the default case (more below).
http://codereview.chromium.org/543161/diff/9055/9092#newcode1122
src/mips/simulator-mips.cc:1122: set_register(LO, rs/rt);
Spacing: rs / rt (three more just below).
http://codereview.chromium.org/543161/diff/9055/9092#newcode1299
src/mips/simulator-mips.cc:1299: alu_out = (oe_imm16<<16);
Spaces around << (several other places).
http://codereview.chromium.org/543161/diff/9055/9092#newcode1616
src/mips/simulator-mips.cc:1616:
Please add another blank line.
http://codereview.chromium.org/543161/diff/9055/9095
File src/mips/simulator-mips.h (right):
http://codereview.chromium.org/543161/diff/9055/9095#newcode205
src/mips/simulator-mips.h:205:
Does these halfword read/writes make any sense on a MIPS platform? If
not please remove them.
http://codereview.chromium.org/543161/diff/9055/9095#newcode220
src/mips/simulator-mips.h:220: inline int32_t getDoubleHIW(double*
addr);
get -> Get
http://codereview.chromium.org/543161/diff/9055/9095#newcode221
src/mips/simulator-mips.h:221: inline int32_t getDoubleLOW(double*
addr);
Ditto.
http://codereview.chromium.org/543161/diff/9055/9095#newcode223
src/mips/simulator-mips.h:223: inline int32_t setDoubleHIW(double*
addr);
set -> Set
http://codereview.chromium.org/543161/diff/9055/9095#newcode224
src/mips/simulator-mips.h:224: inline int32_t setDoubleLOW(double*
addr);
Ditto.
http://codereview.chromium.org/543161/diff/9055/9095#newcode247
src/mips/simulator-mips.h:247: enum Exception {
Please use either kXxxYxx or XXX_YYY naming, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumerator_Names#Enumerator_Names.
http://codereview.chromium.org/543161/diff/9055/9090
File src/mips/stub-cache-mips.cc (right):
http://codereview.chromium.org/543161/diff/9055/9090#newcode1
src/mips/stub-cache-mips.cc:1: // Copyright 2006-2010 the V8 project
authors. All rights reserved.
Don't you want all the UNIMPLEMENTED in this file to be
UNIMPLEMENTED_MIPS?
http://codereview.chromium.org/543161/diff/9055/9078
File src/mips/virtual-frame-mips.cc (right):
http://codereview.chromium.org/543161/diff/9055/9078#newcode1
src/mips/virtual-frame-mips.cc:1: // Copyright 2006-2010 the V8 project
authors. All rights reserved.
Don't you want all the UNIMPLEMENTED in this file to be
UNIMPLEMENTED_MIPS?
http://codereview.chromium.org/543161/diff/9055/9068
File src/objects-inl.h (right):
http://codereview.chromium.org/543161/diff/9055/9068#newcode1032
src/objects-inl.h:1032: // WRITE_FIELD does not update the remembered
set, but there is no need here.
Please revert this.
http://codereview.chromium.org/543161/diff/9055/9062
File src/objects.h (right):
http://codereview.chromium.org/543161/diff/9055/9062#newcode1105
src/objects.h:1105:
Please revert this.
http://codereview.chromium.org/543161/diff/9055/9066
File src/platform-linux.cc (right):
http://codereview.chromium.org/543161/diff/9055/9066#newcode750
src/platform-linux.cc:750: // Implement this on MIPS.
Please UNIMPLELEMTED() here.
http://codereview.chromium.org/543161/diff/9055/9056
File test/cctest/test-assembler-mips.cc (right):
http://codereview.chromium.org/543161/diff/9055/9056#newcode1
test/cctest/test-assembler-mips.cc:1: // Copyright 2006-2008 the V8
project authors. All rights reserved.
2006-2008 -> 2010.
http://codereview.chromium.org/543161/diff/9055/9056#newcode79
test/cctest/test-assembler-mips.cc:79: __ sllv(zero_reg, zero_reg,
zero_reg); // nop
Perhaps add Nop to the macro assembler?
http://codereview.chromium.org/543161/diff/9055/9056#newcode183
test/cctest/test-assembler-mips.cc:183: __ nop();
Code in comments.
http://codereview.chromium.org/543161
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
