Reviewers: Mads Ager, Erik Corry, SeRya, antonm, Message: Thanks Anton for the review!
I have answered major comments and expecting feedback. i will submit the updated
patch with the fixed comments asap. http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1414 src/arm/macro-assembler-arm.cc:1414: mov(scratch, Operand(unwind_space)); On 2011/01/11 14:11:33, antonm wrote:
it looks like a lot of code below is shared with
MacroAssembler::EnterExitFrame.
Is it possible to refactor the common code and make it more like ia32 implementation (which just invokes EnterApiExitFrame + some magic for
some
platforms?
The behavior is different from EnterExitFrame - v8::arguments has to be allocated above exit frame to miss gc - The alignment has to happen after allocating space for the args and it should handle aligned/unaligned case based on arg_stack_space (unlike EnterExitFrame which knows the number of pushes - 5) - exit frame pc patching has to be handled - argv and builtin function is n/a in this case http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1461 src/arm/macro-assembler-arm.cc:1461: // patch the exit frame pc and Call the api function! On 2011/01/11 14:11:33, antonm wrote:
nit: [P]atch and [c]all.
Done http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1461 src/arm/macro-assembler-arm.cc:1461: // patch the exit frame pc and Call the api function! On 2011/01/11 14:11:33, antonm wrote:
once again, can we factor out frame handling?
same issues as mentioned before for refactoring http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1463 src/arm/macro-assembler-arm.cc:1463: str(ip, MemOperand(fp, -2 * kPointerSize)); On 2011/01/11 14:11:33, antonm wrote:
do not we have a named constant for -2?
check ExitFrame::FillState(..) it seems to be hard coded (pc = sp - 1 = fp -2). This value is already used for kMarkerOffset so i wasnt sure to introduce a new constant. http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1494 src/arm/macro-assembler-arm.cc:1494: ldr(r4, MemOperand(ip)); On 2011/01/11 14:11:33, antonm wrote:
Shouldn't it be LoadRoot(r4, Heap::kTheHoleValueRootIndex)?
Done http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1499 src/arm/macro-assembler-arm.cc:1499: LeaveExitFrame(0); On 2011/01/11 14:11:33, antonm wrote:
LeaveExitFrame apparently takes bool, not int
done http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.h#newcode644 src/arm/macro-assembler-arm.h:644: // creates the Exit frame, pushes the arguments and aligns the stack On 2011/01/11 14:11:33, antonm wrote:
nit: [C]reates. And you probably do not need [E] in exit frame.
Overall, why doc is that different from ia32 version? And, please,
provide
documentation for arg_stack_space and unwind_space parameters.
Done http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newcode645 src/arm/stub-cache-arm.cc:645: const CallOptimization& optimization, On 2011/01/11 14:11:33, antonm wrote:
nit: indentation is slightly off
Done http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newcode647 src/arm/stub-cache-arm.cc:647: Failure** failure) { On 2011/01/11 14:11:33, antonm wrote:
may you add a stack map like in ia32 version?
Done http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newcode674 src/arm/stub-cache-arm.cc:674: __ str(r2, MemOperand(sp)); On 2011/01/11 14:11:33, antonm wrote:
I don't know if it'd be faster, but maybe you can use memoperand that
post
increment pointer to arrange this sequence of stores, something like:
__ mov(r4, sp); __ str(r2, MemOperand(r4, kPointerSize, PostIndex); __ add(ip, r2, ...); __ str(ip, MemOperand(r4, kPointerSize, PostIndex); // etc.
will check http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newcode679 src/arm/stub-cache-arm.cc:679: // v8::Arguments::length_ = argc On 2011/01/11 14:11:33, antonm wrote:
for me comments are one line below they should be: I'd prefer to have
//
v8::Arguments::length_ = argc followed by mov(ip, Operand(argc)) and
str(ip,
...)
Mistake! fixed http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newcode2319 src/arm/stub-cache-arm.cc:2319: #ifdef USE_SIMULATOR On 2011/01/11 14:11:33, antonm wrote:
do we need a special case for simulator?
currently the simulator interface to the native api expects a argc, argv and direct call breaks that (it has a v8::Arguments&). I wasnt sure how to fix this in the simulator, but it would be great if we can do it. Description: Direct call api functions (arm implementation). This gives a 10% on dromaeo benchmark. Please review this at http://codereview.chromium.org/6170001/ SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/ Affected files: M AUTHORS M src/arm/macro-assembler-arm.h M src/arm/macro-assembler-arm.cc M src/arm/stub-cache-arm.cc M src/v8utils.h -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
