LGTM. My comments are minor, looks like frames-ia32.cc and frames-arm.cc are very close to each other, you can refactor them in another CL.
Looking forward to your submission. http://codereview.chromium.org/1930/diff/1/9 File src/builtins-arm.cc (right): http://codereview.chromium.org/1930/diff/1/9#newcode67 Line 67: __ push(r1); // constructor function The constructor function pushed on the stack seems not used. Double check! http://codereview.chromium.org/1930/diff/1/9#newcode100 Line 100: // r3: number of arguments (smi-tagged) How about adding one more line comment here: // r2: caller sp http://codereview.chromium.org/1930/diff/1/18 File src/codegen-arm.cc (right): http://codereview.chromium.org/1930/diff/1/18#newcode3876 Line 3876: ASSERT(args->length() == 1); Please update comment above function declaration. http://codereview.chromium.org/1930/diff/1/8 File src/frames-arm.cc (right): http://codereview.chromium.org/1930/diff/1/8#newcode48 Line 48: return static_cast<StackFrame::Type>(Smi::cast(marker)->value()); Now this function looks exactly like the on in frame-ia32.cc. Can we move it to frames.cc? http://codereview.chromium.org/1930/diff/1/8#newcode99 Line 99: return fp() + offset + (arguments * kPointerSize); The same as the one in frames-ia32.cc http://codereview.chromium.org/1930/diff/1/20 File src/macro-assembler-arm.cc (right): http://codereview.chromium.org/1930/diff/1/20#newcode299 Line 299: This part is a bit complicated. I can see unnecessary mov instructions in different paths. For example, when both actual and expected are immediate but not equal, there are two instructions that move actual.immediate() into r0. http://codereview.chromium.org/1930/diff/1/20#newcode309 Line 309: mov(r0, Operand(actual.immediate())); This mov(r0, actual...) seems redundant. http://codereview.chromium.org/1930/diff/1/20#newcode316 Line 316: mov(r0, Operand(actual.immediate())); This mov(r0, actual...) seems redundant. http://codereview.chromium.org/1930/diff/1/20#newcode665 Line 665: Builtins::JavaScript id, bool* resolved) { Can this function be made that same as in IA32? (non-static). Part of code looks identical as the same function in IA32. You may want to refactor the common code out. http://codereview.chromium.org/1930 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
