Zaheer, that's almost lgtm, but I have a couple of questions.
Plus I would really appreciate if frame handling can be refactored to be as close as possible to ia32 implementation. Thanks a lot for porting this, it's most appreciated. 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)); 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? 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! nit: [P]atch and [c]all. 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! once again, can we factor out frame handling? 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)); do not we have a named constant for -2? 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)); Shouldn't it be LoadRoot(r4, Heap::kTheHoleValueRootIndex)? http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#newcode1499 src/arm/macro-assembler-arm.cc:1499: LeaveExitFrame(0); LeaveExitFrame apparently takes bool, not int 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 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. 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, nit: indentation is slightly off http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newcode647 src/arm/stub-cache-arm.cc:647: Failure** failure) { may you add a stack map like in ia32 version? http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newcode662 src/arm/stub-cache-arm.cc:662: __ stm(ib, sp, r5.bit() | r6.bit()); please, add a comment what are you doing here. 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)); 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. 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 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, ...) http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newcode2319 src/arm/stub-cache-arm.cc:2319: #ifdef USE_SIMULATOR do we need a special case for simulator? http://codereview.chromium.org/6170001/diff/1/src/v8utils.h File src/v8utils.h (right): http://codereview.chromium.org/6170001/diff/1/src/v8utils.h#newcode31 src/v8utils.h:31: #include <stdarg.h> why this new include? http://codereview.chromium.org/6170001/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
