Thanks for the code review! I have addressed the comments, and rebased the
code
on bleeding_edge r7312.
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
On 2011/03/21 16:05:19, Søren Gjesse wrote:
I don't like these platform dependent members here - if this is
currently
unavoidable they should at least be inside #ifdef/#endif.
I have removed this for now, since relocation is not needed for the
assembler/sim tests. I have commented out the access to these members
from assembler-mips-inl.h.
Going forward, I will submit a single commit with our approach to
relocation, and maybe you can suggest a cleaner way than this to handle
it.
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());
On 2011/03/21 16:05:19, Søren Gjesse wrote:
Code in comments.
Removed.
http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newcode624
src/mips/assembler-mips.cc:624: if (rmode == RelocInfo::NONE) {
On 2011/03/21 16:05:19, Søren Gjesse wrote:
Use
return rmode != RelocInfo::NONE
instead of the if statement.
Done.
http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newcode988
src/mips/assembler-mips.cc:988:
On 2011/03/21 16:05:19, Søren Gjesse wrote:
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.
I've filed an issue to remove this. I see that it is still present in
Arm, and disabled for crankshaft. We will follow this for now, and
remove it later.
http://codereview.chromium.org/6709022/diff/1/src/mips/assembler-mips.cc#newcode1767
src/mips/assembler-mips.cc:1767: } else {
On 2011/03/21 16:05:19, Søren Gjesse wrote:
Instead of UNIMPLEMENTED_MIPS() this should probable be UNREACHABLE().
Or even
better just put in ASSERT(mips32r2).
More of these below.
Done.
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) ||
On 2011/03/21 16:05:19, Søren Gjesse wrote:
Code in comments.
Done.
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.
On 2011/03/21 16:05:19, Søren Gjesse wrote:
Please start comment with uppercase letter.
Done.
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()) {
On 2011/03/21 16:05:19, Søren Gjesse wrote:
Please remove the true here. Disabling crankshaft for MIPS should be
handled by
an #ifdef/#endif in flag-definitions.h.
Done.
http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode70
src/mips/cpu-mips.cc:70: #else // simulator mode
On 2011/03/21 16:05:19, Søren Gjesse wrote:
#else comment should match the #if
Done.
http://codereview.chromium.org/6709022/diff/1/src/mips/cpu-mips.cc#newcode77
src/mips/cpu-mips.cc:77: #endif // #ifdef __mips
On 2011/03/21 16:05:19, Søren Gjesse wrote:
#endif comment should match the #if
Done.
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
On 2011/03/21 16:05:19, Søren Gjesse wrote:
Please consider moving this to constants-mips.h. See constants-arm.h
for how
CAN_USE_ARMV7_INSTRUCTIONS is defined.
Done.
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
On 2011/03/21 16:05:19, Søren Gjesse wrote:
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
The code has been moved to the READ_DOUBLE_FIELD macro as you suggest.
However, I saw no way to leave this as a pure macro, as the macro needs
to evaluate to an expression of type double that can be returned by
HeapNumber:: value(). All approaches to multiline macros that I know
(for instance, wrapping in do { ... } while(0)), end up as a compound
statement, not an expression.
Therefore I used a static inline function, and had the macro call that.
I am open to better suggestions.
http://codereview.chromium.org/6709022/diff/1/src/objects-inl.h#newcode1212
src/objects-inl.h:1212: #else // V8_TARGET_ARCH_MIPS
On 2011/03/21 16:05:19, Søren Gjesse wrote:
Ditto.
This one could have been implemented as pure macro, but I chose to
implement this with static inline, as was done above, for consistency.
Maybe the inline function names should me made more unique, possibly
mips_write_double_field()?
http://codereview.chromium.org/6709022/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
