First round of comments, mainly for the platform independent files.
As you have probably seen the isolates branch was merged with bleeding_edge
on
Friday (r7268 reverted in r7269 and finally landed in r7271. This means that
this patch will have to be updated to current bleeding_edge with isolates
to be
committed. Looking at the changes in the ARM specific files in r7271 this
should
be straight forward. You can run test.py with option --isolates to test with
several active isolates.
http://codereview.chromium.org/6709022/diff/1/src/assembler.h
File src/assembler.h (right):
http://codereview.chromium.org/6709022/diff/1/src/assembler.h#newcode361
src/assembler.h:361: // Code and Embedded Object pointers in mips are
stored split across
I don't like these platform dependent members here - if this is
currently unavoidable they should at least be inside #ifdef/#endif.
http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):
http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newcode607
src/mips/assembler-mips.cc:607: // ASSERT(L->pos() == kEndOfChain ||
L->is_linked());
Code in comments.
http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newcode624
src/mips/assembler-mips.cc:624: if (rmode == RelocInfo::NONE) {
Use
return rmode != RelocInfo::NONE
instead of the if statement.
http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newcode988
src/mips/assembler-mips.cc:988:
You should probably consider getting rid of ppep-hole optimizations
going forward. For crankshaft we have disbled it as it was to difficult
to keep control of the code generated when jumping from optimized code
to un-optimized code.
http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newcode1767
src/mips/assembler-mips.cc:1767: } else {
Instead of UNIMPLEMENTED_MIPS() this should probable be UNREACHABLE().
Or even better just put in ASSERT(mips32r2).
More of these below.
http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newcode2072
src/mips/assembler-mips.cc:2072: // if ( ! (((instr1 & kOpcodeMask) ==
LUI && (instr2 & kOpcodeMask) == ORI) ||
Code in comments.
http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc
File src/mips/cpu-mips.cc (right):
http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode44
src/mips/cpu-mips.cc:44: #include "simulator.h" // for cache flushing.
Please start comment with uppercase letter.
http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode53
src/mips/cpu-mips.cc:53: if (true || !CpuFeatures::IsSupported(FPU) ||
Serializer::enabled()) {
Please remove the true here. Disabling crankshaft for MIPS should be
handled by an #ifdef/#endif in flag-definitions.h.
http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode70
src/mips/cpu-mips.cc:70: #else // simulator mode
#else comment should match the #if
http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode77
src/mips/cpu-mips.cc:77: #endif // #ifdef __mips
#endif comment should match the #if
http://codereview.chromium.org/6709022/diff/1/src/mips/disasm-mips.cc
File src/mips/disasm-mips.cc (right):
http://codereview.chromium.org/6709022/diff/1/src/mips/disasm-mips.cc#newcode66
src/mips/disasm-mips.cc:66: #ifdef _MIPS_ARCH_MIPS32R2
Please consider moving this to constants-mips.h. See constants-arm.h for
how CAN_USE_ARMV7_INSTRUCTIONS is defined.
http://codereview.chromium.org/6709022/diff/1/src/objects-inl.h
File src/objects-inl.h (right):
http://codereview.chromium.org/6709022/diff/1/src/objects-inl.h#newcode1195
src/objects-inl.h:1195: #else // V8_TARGET_ARCH_MIPS
I think this should be put into the macro READ_DOUBLE_FIELD, and then
there should be two different macro definitions
#ifndef V8_TARGET_ARCH_MIPS
#define READ_DOUBLE_FIELD(p, offset) \
<current definition>
#else
#define READ_DOUBLE_FIELD(p, offset) \
<MIPS specific definition>
#endif
http://codereview.chromium.org/6709022/diff/1/src/objects-inl.h#newcode1212
src/objects-inl.h:1212: #else // V8_TARGET_ARCH_MIPS
Ditto.
http://codereview.chromium.org/6709022/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
