Thanks for comments. I'll remove my over-optimization, see if I can remove
the
api-stuff and reupload.
If I don't make it by today, have a good week-end.
http://codereview.chromium.org/1689010/diff/1/2
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/1689010/diff/1/2#newcode1331
src/ia32/stub-cache-ia32.cc:1331: __ j(above, &call_builtin);
Because it is conceivable (although not likely) that our heap can cross
the 0x80000000 address boundary, and that would make "greater" give the
wrong result (since the larger address is considered negative).
Well, actually, it can't happen the way we currently allocate our
NewSpace, but using unsigned comparison for addresses is still "more
correct".
http://codereview.chromium.org/1689010/diff/1/2#newcode1429
src/ia32/stub-cache-ia32.cc:1429: __ j(equal, &return_undefined);
Good point. Too optimistic an assumption here.
http://codereview.chromium.org/1689010/diff/1/3
File src/x64/codegen-x64.cc (right):
http://codereview.chromium.org/1689010/diff/1/3#newcode7779
src/x64/codegen-x64.cc:7779: masm->RecordWriteHelper(object_, addr_,
scratch_);
Heh, funny story. It's this way here because I copied it from IA32 :).
I.e., I have no idea, and it probably should just be __ for consistency.
http://codereview.chromium.org/1689010/diff/1/5
File src/x64/macro-assembler-x64.cc (right):
http://codereview.chromium.org/1689010/diff/1/5#newcode198
src/x64/macro-assembler-x64.cc:198: shr(scratch,
Immediate(kPointerSizeLog2));
We are finding the index of a pointer position in the page. We have one
bit in the RSet per pointer (per kPointerSize bytes), so the correct
thing to do here is to divide by kPointerSize (which we can do by a
shift since it's a power of 2).
Using kObjectAlignmentBits is only working by coincidence. We could have
an object alignment that is larger than pointer-size (although it better
be a multiple of it :).
http://codereview.chromium.org/1689010/diff/1/6
File src/x64/macro-assembler-x64.h (right):
http://codereview.chromium.org/1689010/diff/1/6#newcode543
src/x64/macro-assembler-x64.h:543: Register scratch, int save_at_depth,
I don't know. It seems to be used, although I'm not sure exactly what it
means.
http://codereview.chromium.org/1689010/diff/1/7
File src/x64/stub-cache-x64.cc (right):
http://codereview.chromium.org/1689010/diff/1/7#newcode558
src/x64/stub-cache-x64.cc:558: // Holds information about possible
function call optimizations.
Great. Let me see if I can remove it and still run the rest.
http://codereview.chromium.org/1689010/diff/1/7#newcode1199
src/x64/stub-cache-x64.cc:1199: __ j(greater,
&attempt_to_grow_elements);
I thought about it and decided aginst it.
Smi values are signed, so to use unsigned comparison should only be to
combine a greater-than-or-negative test in one comparison. I see no
reason to believe that either register should hold a negative value
(both are read from fields that must only hold positive values), so
testing for negativity would actually be confusing at this point (the
reader would start thinking about how the value could possibly be
positive).
http://codereview.chromium.org/1689010/diff/1/7#newcode1209
src/x64/stub-cache-x64.cc:1209: index.reg, index.scale,
Fixed.
http://codereview.chromium.org/1689010/diff/1/7#newcode1348
src/x64/stub-cache-x64.cc:1348: __ j(equal, &return_undefined);
Over-optimized. Will fix.
http://codereview.chromium.org/1689010/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
