http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right):
http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc#newcode6017 src/arm/code-stubs-arm.cc:6017: // Must be null (deleted entry). On 2011/03/15 09:07:01, Lasse Reichstein wrote:
How about adding some debug code to test that it is null?
Done. http://codereview.chromium.org/6682026/diff/1014/src/arm/code-stubs-arm.cc#newcode6022 src/arm/code-stubs-arm.cc:6022: // Check that the candidate is a non-external ascii string. The instance On 2011/03/15 09:07:01, Lasse Reichstein wrote:
ASCII is an acronym.
Done. http://codereview.chromium.org/6682026/diff/1014/src/arm/codegen-arm.cc File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/6682026/diff/1014/src/arm/codegen-arm.cc#newcode7209 src/arm/codegen-arm.cc:7209: On 2011/03/15 09:07:01, Lasse Reichstein wrote:
This code now clobbers scratch1 which is assumed to be the elements
fixed-array
below.
Nice catch. The tests missed this because this whole file is dead code in the Cranshaft VM. http://codereview.chromium.org/6682026/diff/1014/src/arm/codegen-arm.cc#newcode7209 src/arm/codegen-arm.cc:7209: On 2011/03/15 09:07:01, Lasse Reichstein wrote:
Add to the comment that we know the length is a smi because we tested
the array
to have fast elements above.
Done. http://codereview.chromium.org/6682026/diff/1014/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (right): http://codereview.chromium.org/6682026/diff/1014/src/ia32/full-codegen-ia32.cc#newcode3052 src/ia32/full-codegen-ia32.cc:3052: __ CmpObjectType(object, JS_OBJECT_TYPE, temp); On 2011/03/15 09:07:01, Lasse Reichstein wrote:
JS_ARRAY_TYPE
Done. http://codereview.chromium.org/6682026/diff/1014/src/x64/assembler-x64.h File src/x64/assembler-x64.h (right): http://codereview.chromium.org/6682026/diff/1014/src/x64/assembler-x64.h#newcode659 src/x64/assembler-x64.h:659: // Instructions to load a 64-bit immediate into a register. On 2011/03/15 09:07:01, Lasse Reichstein wrote:
Good change. Never name something "new X". It gets old fast :)
yup. http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc File src/x64/macro-assembler-x64.cc (right): http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode127 src/x64/macro-assembler-x64.cc:127: // catch stores of smis and stores into young gen. On 2011/03/15 09:07:01, Lasse Reichstein wrote:
gen -> generation.
Done. http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode156 src/x64/macro-assembler-x64.cc:156: // catch stores of smis and stores into young gen. On 2011/03/15 09:07:01, Lasse Reichstein wrote:
ditto.
Done. http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode843 src/x64/macro-assembler-x64.cc:843: AbortIfNotSmi(smi2); On 2011/03/15 09:07:01, Lasse Reichstein wrote:
Good stuff!
yup http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode855 src/x64/macro-assembler-x64.cc:855: Cmp(dst, src); On 2011/03/15 09:07:01, Lasse Reichstein wrote:
... apart from the optimizations that Cmp also does.
Comment deleted. http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode897 src/x64/macro-assembler-x64.cc:897: // The Operand cannot use the smi register, since we may use the scratch On 2011/03/15 09:07:01, Lasse Reichstein wrote:
Notice that smi_reg is either kScratchRegister or kSmiConstantRegister
in this
case.
Comment updated. http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode1797 src/x64/macro-assembler-x64.cc:1797: Condition is_smi = CheckSmi(object); On 2011/03/15 09:07:01, Lasse Reichstein wrote:
Negate condition?
Done. http://codereview.chromium.org/6682026/diff/1014/src/x64/macro-assembler-x64.cc#newcode1797 src/x64/macro-assembler-x64.cc:1797: Condition is_smi = CheckSmi(object); On 2011/03/15 09:07:01, Lasse Reichstein wrote:
Negate condition?
Not done. http://codereview.chromium.org/6682026/diff/1014/test/cctest/test-macro-assembler-x64.cc File test/cctest/test-macro-assembler-x64.cc (right): http://codereview.chromium.org/6682026/diff/1014/test/cctest/test-macro-assembler-x64.cc#newcode223 test/cctest/test-macro-assembler-x64.cc:223: __ cmpq(rcx, rcx); On 2011/03/15 09:07:01, Lasse Reichstein wrote:
Really should be SmiCompare in this function.
Since we are only testing for equality cmpq seems fine. http://codereview.chromium.org/6682026/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
