Dear Erik,
Thank you for reviewing our patch. We addressed a few of your comments, and
have
questions for the remaining.
Thanks,
Gergely
http://codereview.chromium.org/8491008/diff/13001/src/mips/assembler-mips-inl.h
File src/mips/assembler-mips-inl.h (right):
http://codereview.chromium.org/8491008/diff/13001/src/mips/assembler-mips-inl.h#newcode111
src/mips/assembler-mips-inl.h:111: || rmode_ == EXTERNAL_REFERENCE);
On 2011/12/21 09:33:55, Erik Corry wrote:
The funky indentation here doesn't really add anything. Let's just
stick with
one subexpression per line with a || at the end of each line like we
do in the
rest of the VM.
Done.
http://codereview.chromium.org/8491008/diff/13001/src/mips/assembler-mips-inl.h#newcode117
src/mips/assembler-mips-inl.h:117: // For an instructions like LUI/ORI
where the target bits are mixed into the
On 2011/12/21 09:33:55, Erik Corry wrote:
s/an instructions/an instruction/
Done.
http://codereview.chromium.org/8491008/diff/13001/src/mips/assembler-mips-inl.h#newcode123
src/mips/assembler-mips-inl.h:123: // place, ready to be patched with
the target. In our case, after jump
On 2011/12/21 09:33:55, Erik Corry wrote:
s/In our case//
Done.
http://codereview.chromium.org/8491008/diff/13001/src/serialize.cc
File src/serialize.cc (right):
http://codereview.chromium.org/8491008/diff/13001/src/serialize.cc#newcode764
src/serialize.cc:764: static const int kInstructionsForSplitImmediate =
3*kPointerSize;
On 2011/12/21 09:33:55, Erik Corry wrote:
Inconsistency: This constant is premultiplied by kPOinterSize, whereas
kInstructionsFor32BitConstant is measured in instructions and has to
be
multiplied by kInstructionSize when used.
We will change the naming.
This constant is only used in MIPS specific code (see below), and
kInstrSize is not defined on all architectures. Should we put it simply
inside #ifdef V8_TARGET_ARCH_MIPS?
http://codereview.chromium.org/8491008/diff/13001/src/serialize.cc#newcode838
src/serialize.cc:838: if (Assembler::kCallTargetSize == 0) {
\
On 2011/12/21 09:33:55, Erik Corry wrote:
The size of a call target is only relevant if we are dealing with a
call
instruction. In that case 'within == kFirstInstruction' so we will
get into the
'if' just below, which already sets current_was_incremented. In other
words I
think this can be simplified.
Actually, this is a subtle way of saying #ifdef MIPS, because only MIPS
has call target size set to 0. We need to change the patch site because
we use the LUI / ORI instruction pair. So the other option would be to
use #if V8_TARGET_ARCH_MIPS instead of the regular if. This is always
needed, not just when within == kFirstInstruction.
Another possibility would be to add a comment explaining, that this is
required by MIPS for handling the LUI / ORI pair.
Which way would you prefer?
http://codereview.chromium.org/8491008/diff/13001/src/serialize.cc#newcode978
src/serialize.cc:978: ONE_PER_SPACE(kNewObject, kFromCode,
kStartOfObject)
On 2011/12/21 09:33:55, Erik Corry wrote:
I'm a bit worried about the code size here. The deserialization is
performance
critical for startup and context creation, and this macro expands a
lot. I'd
like this to be in an ifdef MIPS. The others in this file don't
expand as much.
Done.
http://codereview.chromium.org/8491008/diff/13001/src/serialize.cc#newcode1014
src/serialize.cc:1014: CASE_STATEMENT(kRootArray, kFromCode,
kStartOfObject, 0)
On 2011/12/21 09:33:55, Erik Corry wrote:
Can we replace these with LoadRoot which does the load relative to a
root array
array and needs no relocation info?
Could you please elaborate this a bit in more detail? We are not
completely sure what you would like to see here.
http://codereview.chromium.org/8491008/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
