Thanks for the helpful comments. Should be much better now, please take
another
look.
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"
On 2011/07/12 12:03:26, Mads Ager wrote:
Why do you need an extra include here when the rest of the file has
not changed?
Done.
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));
On 2011/07/12 12:03:26, Mads Ager wrote:
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.
Done.
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;
On 2011/07/12 12:03:26, Mads Ager wrote:
This doesn't really matter, but I like sizeof(kHoleNanLower32) which
you are
using in the 32-bit version better. :-)
Done.
http://codereview.chromium.org/7307030/diff/12005/src/arm/stub-cache-arm.cc#newcode4208
src/arm/stub-cache-arm.cc:4208: __ add(r5, r4, Operand(1), SetCC);
On 2011/07/12 14:00:30, Rodolph Perfetta wrote:
0x7FFFFFFF + 1 will not set the overflow flag, but the negative flag.
Some
negative values are valid for the upper part of a double so maybe a
cmp could be
used instead. The V8 assembler will replace cmp with 0x7FFFFFFF by cmn
with
0x80000001 and 0x80000001 is a valid ARM immediate (no need for extra
instructions).
Done.
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.
On 2011/07/12 12:03:26, Mads Ager wrote:
NaN
Done.
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"
On 2011/07/12 12:03:26, Mads Ager wrote:
Is isolate.h needed here? There are no other changes to this file?
Done.
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();
On 2011/07/12 12:03:26, Mads Ager wrote:
Do you only need this change in ia32 lithium codegen and not in any of
the
others?
No, only the ia32 implementation uses this constant from assembler.cc in
this way. I was surprised, too.
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));
On 2011/07/12 12:03:26, Mads Ager wrote:
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.
Done.
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);
On 2011/07/12 12:03:26, Mads Ager wrote:
Move the identical branch instructions out of the if?
Done.
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"
On 2011/07/12 12:03:26, Mads Ager wrote:
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?
Done.
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"
On 2011/07/12 12:03:26, Mads Ager wrote:
Is this addition needed?
Done.
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,
On 2011/07/12 12:03:26, Mads Ager wrote:
Same as on ia32 this can lead to unbalanced push/pop on the FPU
register stack.
Done.
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);
On 2011/07/12 12:03:26, Mads Ager wrote:
Move the branch out of the conditional?
Done.
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.
On 2011/07/12 12:03:26, Mads Ager wrote:
Long line. :-)
Done.
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));
On 2011/07/12 12:03:26, Mads Ager wrote:
Long line.
Done.
http://codereview.chromium.org/7307030/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
