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#newcode3279 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#newcode3169 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#newcode3171 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#newcode3182 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#newcode3184 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#newcode5235 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#oldcode650 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#newcode694 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#newcode3156 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#newcode3864 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#newcode195 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#newcode196 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#newcode199 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. http://codereview.chromium.org/7104107/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
