Incremental mode now works for x64. The only difference

Incremental mode now works for x64.  The only difference
between IA32 and x64 is now that in IA32 we can mark
pointer-free objects black without going into runtime
code.
Also fixed a bug where spaces with heightened alignment
requirements could get messed up by the free-list chopping
that was implemented to get timely marking increments.
Committed: http://code.google.com/p/v8/source/detail?r=8270

[Lasse Reichstein, 2011-06-10 13:55:44]

LGTM

http://codereview.chromium.org/7104107/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7104107/diff/1/src/arm/full-codegen-arm.cc#new...
src/arm/full-codegen-arm.cc:3279: // so we don't call the stub that handles
this.
More explanation (e.g. that the incremental marker is never in the middle of a
FixedArray).

http://codereview.chromium.org/7104107/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/7104107/diff/1/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:3169: void MacroAssembler::IsDataObject(Register
value,
JumpIfNotDataObject

http://codereview.chromium.org/7104107/diff/1/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:3171: Label* not_data_object) {
Should it have a distance too?

http://codereview.chromium.org/7104107/diff/1/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:3182: // Jump if we need to mark it grey and push
it.
Comment not related to function name or description.

http://codereview.chromium.org/7104107/diff/1/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:3184: bind(&is_data_object);
This detects only HeapNumber and non-cons String.
It seems there are other data objects (Oddballs), but I guess they are
irrelevant because they are always in old-space?
I.e., add comment saying why.

http://codereview.chromium.org/7104107/diff/1/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/7104107/diff/1/src/spaces.h#newcode170
src/spaces.h:170: static const uint32_t kBitsPerCell = 32;
kBitsPerInt ?

http://codereview.chromium.org/7104107/diff/1/src/spaces.h#newcode173
src/spaces.h:173: static const uint32_t kBytesPerCell = kBitsPerCell / 8;
8 -> kBitsPerByte

http://codereview.chromium.org/7104107/diff/1/src/spaces.h#newcode174
src/spaces.h:174: static const uint32_t kBytesPerCellLog2 = kBitsPerCellLog2 -
3;
3 -> kBitesPerByteLog2

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:5235: if (save_address) __ push(regs_.address());
Put braces around then-block.
Don't trust that __ is a single expression macro (it could be doing coverage
analysis at some point again).

Is there no spare register to use instead of the stack?

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.h
File src/x64/code-stubs-x64.h (left):

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.h#oldcod...
src/x64/code-stubs-x64.h:650: void SaveCallerSaveRegisters(MacroAssembler* masm,
SaveFPRegsMode mode) {
Is this the C+-calling convention saved registers, or just internal ones.
If the former, the XMM registers differ beetween Win64 ABI and X64 ABI (but
pushing them all is obviously safe).

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.h
File src/x64/code-stubs-x64.h (right):

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.h#newcod...
src/x64/code-stubs-x64.h:694: kRememberedSetOnNoNeedToInformIncrementalMarker
Do what to remembered set on no need to inform incremental marker?

http://codereview.chromium.org/7104107/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/7104107/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:3156: // (or them and test against Smi mask.)
Use JumpIf[Not]BothSmi in X64.

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:3864: // Note that we are using a 4-byte aligned
8-byte load.
Badness. If it also overlaps a cache boundary, it can be pretty expensive.

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.h#n...
src/x64/macro-assembler-x64.h:195: // Check if an object has the black
incremental marking color.  Also uses ecx!
ecx->rcx (if still true - I assume it's for shifting).

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.h#n...
src/x64/macro-assembler-x64.h:196: void IsBlack(Register object,
IsBlack -> JumpIfBlack

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.h#n...
src/x64/macro-assembler-x64.h:199: Label* is_black,
is_black -> on_black

[Erik Corry, 2011-06-10 21:57:29]

http://codereview.chromium.org/7104107/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7104107/diff/1/src/arm/full-codegen-arm.cc#new...
src/arm/full-codegen-arm.cc:3279: // so we don't call the stub that handles
this.
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> More explanation (e.g. that the incremental marker is never in the middle of a
> FixedArray).

Done.

http://codereview.chromium.org/7104107/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/7104107/diff/1/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:3169: void MacroAssembler::IsDataObject(Register
value,
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> JumpIfNotDataObject

Done.

http://codereview.chromium.org/7104107/diff/1/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:3171: Label* not_data_object) {
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> Should it have a distance too?

No, ARM has fixed width instructions.

http://codereview.chromium.org/7104107/diff/1/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:3182: // Jump if we need to mark it grey and push
it.
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> Comment not related to function name or description.

Deleted.

http://codereview.chromium.org/7104107/diff/1/src/arm/macro-assembler-arm.cc#...
src/arm/macro-assembler-arm.cc:3184: bind(&is_data_object);
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> This detects only HeapNumber and non-cons String.
> It seems there are other data objects (Oddballs), but I guess they are
> irrelevant because they are always in old-space?
> I.e., add comment saying why.

Done.

http://codereview.chromium.org/7104107/diff/1/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/7104107/diff/1/src/spaces.h#newcode170
src/spaces.h:170: static const uint32_t kBitsPerCell = 32;
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> kBitsPerInt ?

Not really.

http://codereview.chromium.org/7104107/diff/1/src/spaces.h#newcode173
src/spaces.h:173: static const uint32_t kBytesPerCell = kBitsPerCell / 8;
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> 8 -> kBitsPerByte

Done.

http://codereview.chromium.org/7104107/diff/1/src/spaces.h#newcode174
src/spaces.h:174: static const uint32_t kBytesPerCellLog2 = kBitsPerCellLog2 -
3;
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> 3 -> kBitesPerByteLog2

Done.

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:5235: if (save_address) __ push(regs_.address());
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> Put braces around then-block.
> Don't trust that __ is a single expression macro (it could be doing coverage
> analysis at some point again).

Done.

> Is there no spare register to use instead of the stack?

Changed to use arg3.

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.h
File src/x64/code-stubs-x64.h (left):

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.h#oldcod...
src/x64/code-stubs-x64.h:650: void SaveCallerSaveRegisters(MacroAssembler* masm,
SaveFPRegsMode mode) {
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> Is this the C+-calling convention saved registers, or just internal ones.
> If the former, the XMM registers differ beetween Win64 ABI and X64 ABI (but
> pushing them all is obviously safe).

It's a conservative estimate of the intersection of the caller-save registers in
V8 and the caller save registers in C++.

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.h
File src/x64/code-stubs-x64.h (right):

http://codereview.chromium.org/7104107/diff/1/src/x64/code-stubs-x64.h#newcod...
src/x64/code-stubs-x64.h:694: kRememberedSetOnNoNeedToInformIncrementalMarker
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> Do what to remembered set on no need to inform incremental marker?

Update.

http://codereview.chromium.org/7104107/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/7104107/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:3156: // (or them and test against Smi mask.)
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> Use JumpIf[Not]BothSmi in X64.

JumpIfBothSmi doesn't exist.  I'll do this if I see evidence that it makes any
difference.

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.cc#...
src/x64/macro-assembler-x64.cc:3864: // Note that we are using a 4-byte aligned
8-byte load.
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> Badness. If it also overlaps a cache boundary, it can be pretty expensive.

 Lets see.  The alternative, on IA32 is a mispredicted branch.

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.h#n...
src/x64/macro-assembler-x64.h:195: // Check if an object has the black
incremental marking color.  Also uses ecx!
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> ecx->rcx (if still true - I assume it's for shifting).

Done.

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.h#n...
src/x64/macro-assembler-x64.h:196: void IsBlack(Register object,
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> IsBlack -> JumpIfBlack

Done.

http://codereview.chromium.org/7104107/diff/1/src/x64/macro-assembler-x64.h#n...
src/x64/macro-assembler-x64.h:199: Label* is_black,
On 2011/06/10 13:55:44, Lasse Reichstein wrote:
> is_black -> on_black

Done.