Thanks Anton, Serya for your comments. Please review the latest patch.
http://codereview.chromium.org/6462029/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):
http://codereview.chromium.org/6462029/diff/1/src/arm/simulator-arm.cc#newcode1534
src/arm/simulator-arm.cc:1534: typedef v8::Handle<v8::Value>
(*SimulatorRuntimeDirectApiCall)(int32_t arg0);
On 2011/02/17 16:13:44, antonm wrote:
Do we need word Direct here? It looks like simulator doesn't have to
know it's
anything direct. But if you feel it conveys important semantic thing,
of course
we should keep it.
I think direct differentiates from the builtin call and i would like to
keep it for now.
http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):
http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newcode1242
src/arm/stub-cache-arm.cc:1242: Register holder_reg =
On 2011/02/17 16:13:44, antonm wrote:
if you change this, please, change other platforms files as well:
we're trying
to keep the things in parallel as much as we can,
Do not wish to touch x86, reverted it back to reg.
http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newcode1245
src/arm/stub-cache-arm.cc:1245: ASSERT(holder_reg.is(receiver) ||
holder_reg.is(scratch1));
On 2011/02/17 16:13:44, antonm wrote:
do you waht to assert this or rather the fact that
!holder_reg.is(scratch{2,3})?
The holder_reg.is(scratch{2,3}) check already happens in
checkprototypes.
http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newcode1249
src/arm/stub-cache-arm.cc:1249: __ push(receiver);
On 2011/02/17 16:13:44, antonm wrote:
may I ask you to reformat and comment you code slightly to better
communicate
what you're doing here?
I'd suggest:
Comment at ln. 1247 becomes:
// Build AccessorGetter arguments on the stack below exit frame to
make GC aware
of them and store pointers to them.
[Empty line]
// Build AccessorInfo on the stack.
__ push(receiver);
__ mov(scratch2, sp); // scratch2 = &AccessorInfo.
__ push(holder_reg);
....
__ push(sratch3);
[Here comes blank line]
// Push name to the stack.
__ push(name_reg);
__ mov(r0, sp);
with the merge of pushes, its difficult to separate the blocks. i have
modified the comments.
http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newcode1260
src/arm/stub-cache-arm.cc:1260: __ push(name_reg);
On 2011/02/20 17:28:39, SeRya wrote:
__ Push(receiver, scratch3, name_reg) (or at least __ Push(scratch3,
name_reg))
apparently should work and save one instruction.
Done. Merged the last three pushes. cant merge the receiver since it can
be same as holder reg.
http://codereview.chromium.org/6462029/diff/1/src/arm/stub-cache-arm.cc#newcode1268
src/arm/stub-cache-arm.cc:1268: __ str(scratch2, MemOperand(sp, 1 *
kPointerSize));
On 2011/02/17 16:13:44, antonm wrote:
Please, explain briefly why we need additional wrapping here.
This to create internal::Object** (AccessorInfo only member)
BTW, is this ABI universal across ARM compilers?
I cant see any ABI/compiler specific code here. May be i miss your
point.
http://codereview.chromium.org/6462029/diff/1/src/assembler.h
File src/assembler.h (right):
http://codereview.chromium.org/6462029/diff/1/src/assembler.h#newcode481
src/assembler.h:481: DIRECT_API_CALL,
On 2011/02/17 16:13:44, antonm wrote:
again question about DIRECT, maybe just drop it, but again no
insisting, feel
free to keep it if you like
keeping it for now to differentiate from builtin.
http://codereview.chromium.org/6462029/diff/1/src/assembler.h#newcode485
src/assembler.h:485: DIRECT_LOAD_CALL
On 2011/02/17 16:13:44, antonm wrote:
Please, do not use load and use getter or callback instead (or even
accessor
getter): we already have this callback/getter distinction and I think
we should
introduce another word.
Done.
http://codereview.chromium.org/6462029/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):
http://codereview.chromium.org/6462029/diff/1/test/cctest/test-api.cc#newcode7587
test/cctest/test-api.cc:7587: v8::V8::LowMemoryNotification();
On 2011/02/17 16:13:44, antonm wrote:
you can probably directly force GC as v8::internal::Heap is exposed
here. And
yes, we do it :)
Done.
http://codereview.chromium.org/6462029/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
