MIPS: Enable serialization for MIPS architecture.

MIPS: Enable serialization for MIPS architecture.


BUG=
TEST=

[kisg, 2011-11-07 09:55:13]

This patch depends on the following patch to be landed first: 
http://codereview.chromium.org/8467010/

[kisg, 2011-11-13 21:42:26]

Rebased on r9981

[kisg, 2011-11-21 21:54:13]

Rebased on r10041

[kisg, 2011-12-04 15:05:35]

Rebased on r10145.

[Erik Corry, 2011-12-21 09:33:55]

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...
src/mips/assembler-mips-inl.h:111: || rmode_ == EXTERNAL_REFERENCE);
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.

http://codereview.chromium.org/8491008/diff/13001/src/mips/assembler-mips-inl...
src/mips/assembler-mips-inl.h:117: // For an instructions like LUI/ORI where the
target bits are mixed into the
s/an instructions/an instruction/

http://codereview.chromium.org/8491008/diff/13001/src/mips/assembler-mips-inl...
src/mips/assembler-mips-inl.h:123: // place, ready to be patched with the
target. In our case, after jump
s/In our case//

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;
Inconsistency: This constant is premultiplied by kPOinterSize, whereas
kInstructionsFor32BitConstant is measured in instructions and has to be
multiplied by kInstructionSize when used.

http://codereview.chromium.org/8491008/diff/13001/src/serialize.cc#newcode838
src/serialize.cc:838: if (Assembler::kCallTargetSize == 0) {                    
        \
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.

http://codereview.chromium.org/8491008/diff/13001/src/serialize.cc#newcode978
src/serialize.cc:978: ONE_PER_SPACE(kNewObject, kFromCode, kStartOfObject)
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.

http://codereview.chromium.org/8491008/diff/13001/src/serialize.cc#newcode1014
src/serialize.cc:1014: CASE_STATEMENT(kRootArray, kFromCode, kStartOfObject, 0)
Can we replace these with LoadRoot which does the load relative to a root array
array and needs no relocation info?

[kisg, 2011-12-22 15:30:10]

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...
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...
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...
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.