http://codereview.chromium.org/7307030/diff/12005/src/apiutils.h File src/apiutils.h (right):
http://codereview.chromium.org/7307030/diff/12005/src/apiutils.h#newcode31 src/apiutils.h:31: #include "objects.h" Why do you need an extra include here when the rest of the file has not changed? http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#newcode4196 src/arm/stub-cache-arm.cc:4196: __ ldr(r3, FieldMemOperand(elements_reg, FixedArray::kLengthOffset)); In the store stub you have named the scratch registers as well. I think you should be consistent and either name them here as well or remove the naming in the store stub. http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#newcode4203 src/arm/stub-cache-arm.cc:4203: uint32_t upper_32_offset = FixedArray::kHeaderSize + Register::kSizeInBytes; This doesn't really matter, but I like sizeof(kHoleNanLower32) which you are using in the 32-bit version better. :-) http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#newcode4211 src/arm/stub-cache-arm.cc:4211: // Non Nan. Allocate a new heap number and copy the double value into it. NaN http://codereview.chromium.org/7307030/diff/12005/src/factory.h File src/factory.h (right): http://codereview.chromium.org/7307030/diff/12005/src/factory.h#newcode34 src/factory.h:34: #include "isolate.h" Is isolate.h needed here? There are no other changes to this file? http://codereview.chromium.org/7307030/diff/12005/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): http://codereview.chromium.org/7307030/diff/12005/src/ia32/lithium-codegen-ia32.cc#newcode2793 src/ia32/lithium-codegen-ia32.cc:2793: ExternalReference::address_of_canonical_non_hole_nan(); Do you only need this change in ia32 lithium codegen and not in any of the others? http://codereview.chromium.org/7307030/diff/12005/src/ia32/stub-cache-ia32.cc File src/ia32/stub-cache-ia32.cc (right): http://codereview.chromium.org/7307030/diff/12005/src/ia32/stub-cache-ia32.cc#newcode3822 src/ia32/stub-cache-ia32.cc:3822: __ fld_d(FieldOperand(ecx, eax, times_4, FixedDoubleArray::kHeaderSize)); This looks dangerous. Can't this lead to an unbalanced FPU stack? You are loading here and then the allocation can fail and you don't pop the value if it does. Either you should allocate earlier or you should pop from the FPU stack (ffree(); fincstp()) before going into the runtime system to allocate the heap number. http://codereview.chromium.org/7307030/diff/12005/src/ia32/stub-cache-ia32.cc#newcode3914 src/ia32/stub-cache-ia32.cc:3914: __ j(above_equal, &miss_force_generic); Move the identical branch instructions out of the if? http://codereview.chromium.org/7307030/diff/12005/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/7307030/diff/12005/src/objects.cc#newcode31 src/objects.cc:31: #include "assembler.h" objects.cc needs the assembler? Is this for some constants? Maybe we should move those constants to something like v8.h in that case? Seems a bit strange that the assembler is needed here? http://codereview.chromium.org/7307030/diff/12005/src/runtime.h File src/runtime.h (right): http://codereview.chromium.org/7307030/diff/12005/src/runtime.h#newcode32 src/runtime.h:32: #include "objects.h" Is this addition needed? http://codereview.chromium.org/7307030/diff/12005/src/x64/stub-cache-x64.cc File src/x64/stub-cache-x64.cc (right): http://codereview.chromium.org/7307030/diff/12005/src/x64/stub-cache-x64.cc#newcode3618 src/x64/stub-cache-x64.cc:3618: __ fld_d(FieldOperand(rcx, kScratchRegister, times_8, Same as on ia32 this can lead to unbalanced push/pop on the FPU register stack. http://codereview.chromium.org/7307030/diff/12005/src/x64/stub-cache-x64.cc#newcode3712 src/x64/stub-cache-x64.cc:3712: __ j(above_equal, &miss_force_generic); Move the branch out of the conditional? http://codereview.chromium.org/7307030/diff/12005/test/mjsunit/unbox-double-arrays.js File test/mjsunit/unbox-double-arrays.js (right): http://codereview.chromium.org/7307030/diff/12005/test/mjsunit/unbox-double-arrays.js#newcode81 test/mjsunit/unbox-double-arrays.js:81: // (premonomorphic and monomorphic) of KeyedLoad access works in various cases. Long line. :-) http://codereview.chromium.org/7307030/diff/12005/test/mjsunit/unbox-double-arrays.js#newcode117 test/mjsunit/unbox-double-arrays.js:117: test_various_stores(large_array, Infinity, -Infinity, expected_array_value(7)); Long line. http://codereview.chromium.org/7307030/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
