Done with all files.
When the comments has been addressed this should be committable.
Regarding the preprocessor symbol mips32r2 I have a few comments. Should
this be
a CPU feature like FPU with CPU feature check and CPU feature scope (on ARM
ARMv7 is a CPU feature)? If not why not have it as a global boolean instead
of
0/1? You can save that to a separate change.
http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (left):
http://codereview.chromium.org/6709022/diff/5001/src/mips/assembler-mips.cc#oldcode478
src/mips/assembler-mips.cc:478: ASSERT(fd.is_valid() && fs.is_valid() &&
ft.is_valid());
Shouldn't this and all other FPU instructions start with:
ASSERT(CpuFeatures::IsEnabled(FPU));
?
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.cc
File src/mips/macro-assembler-mips.cc (right):
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.cc#newcode2162
src/mips/macro-assembler-mips.cc:2162: void
MacroAssembler::AllocateHeapNumber(Register result,
Indentation off.
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.cc#newcode2171
src/mips/macro-assembler-mips.cc:2171:
AllocateInNewSpace(HeapNumber::kSize,
Indentation off.
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.cc#newcode2183
src/mips/macro-assembler-mips.cc:2183: #ifdef DEBUG
Code in comments.
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.cc#newcode2472
src/mips/macro-assembler-mips.cc:2472: lw(result,
Indentation.
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.cc#newcode2573
src/mips/macro-assembler-mips.cc:2573: lw(scratch1,
FieldMemOperand(object, HeapNumber::kExponentOffset));
What is the purpose of this block?
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.cc#newcode2955
src/mips/macro-assembler-mips.cc:2955: #endif
The comment about vstrm is ARM specific.
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.cc#newcode3040
src/mips/macro-assembler-mips.cc:3040: return 2 * kPointerSize; //
TODO(REBASE): use above flog & remove this line.
REBASE -> mips, flog -> flag
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h
File src/mips/macro-assembler-mips.h (right):
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h#newcode45
src/mips/macro-assembler-mips.h:45: // The programmer should know that
the MacroAssembler may clobber these two,
two -> three
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h#newcode105
src/mips/macro-assembler-mips.h:105: void Name(bool
ProtectBranchDelaySlot = true); \
ProtectBranchDelaySlot -> protect_branch_delay_slot (quite a few places)
How about using an enum instead of a bool.
enum BranchDelaySlot {
PROTECT_BRANCH_DELAY_SLOT,
EXPOSE_BRANCH_DELAY_SLOT,
}
Seeing EXPOSE_BRANCH_DELAY_SLOT as an argument instead of just false
seems more clear.
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h#newcode227
src/mips/macro-assembler-mips.h:227: Condition cc, // eq for new space,
ne otherwise
End comment with full stop.
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h#newcode273
src/mips/macro-assembler-mips.h:273: // ie. check if is is a sll
zero_reg, zero_reg, <type> (referenced as
is is -> it is
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h#newcode282
src/mips/macro-assembler-mips.h:282: static inline int
GetCodeMarker(Instr instr) {
Isn't there functions in assembler to extract opcode, rt, rs and sa from
an instructions?
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h#newcode293
src/mips/macro-assembler-mips.h:293: int type = (sllzz &&
Doesn't this fit one line? If not please format like this
int type =
<the rest>
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h#newcode448
src/mips/macro-assembler-mips.h:448: Addu(sp, sp, Operand(2 *
-kPointerSize));
I would prefer -(2 * kPointerSize) instead of 2 * -kPointerSize. What is
the reason for not having Subu?
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h#newcode472
src/mips/macro-assembler-mips.h:472:
Why do we have both push and Push? (same for pop)
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h#newcode570
src/mips/macro-assembler-mips.h:570: void AlignStack(int offset); //
TODO(REBASE) : remove this function.
REBASE -> mips
http://codereview.chromium.org/6709022/diff/5001/src/mips/macro-assembler-mips.h#newcode759
src/mips/macro-assembler-mips.h:759: // sw(t0, MemOperand(sp,
MacroAssembler::kCFuncArg_5));
How about adding CFunctionArgumentOperand (like ContextOperand) and use
sw(t0, CFunctionArgumentOperand(5))
http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc
File src/mips/simulator-mips.cc (right):
http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#newcode309
src/mips/simulator-mips.cc:309: PrintF("\n\n");
Only 2 space indent.
http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#newcode1368
src/mips/simulator-mips.cc:1368: // RS field is equal to 00001
Is there any reason to check for R2 in the simulator? It should be the
code generation that should only generate R1 instructions if R2 is not
enabled.
http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#newcode2103
src/mips/simulator-mips.cc:2103: alu_out |= rt & mask;
Format
}
break;
as
break;
}
More below.
http://codereview.chromium.org/6709022/diff/5001/src/mips/simulator-mips.cc#newcode2303
src/mips/simulator-mips.cc:2303: PrintF(" 0x%08x %s\n",
reinterpret_cast<intptr_t>(instr),
Indentation.
http://codereview.chromium.org/6709022/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
