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#newcode3279 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#newcode3169 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#newcode3171 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#newcode3182 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#newcode3184 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#newcode5235 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#oldcode650 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#newcode694 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#newcode3156 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#newcode3864 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#newcode195 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#newcode196 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#newcode199 src/x64/macro-assembler-x64.h:199: Label* is_black, is_black -> on_black http://codereview.chromium.org/7104107/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
