Looks good so far, my comments are mostly nits. As discussed, you'll have to take care of the other architectures before you can land this.
http://codereview.chromium.org/8241003/diff/1008/src/code-stubs.h File src/code-stubs.h (right): http://codereview.chromium.org/8241003/diff/1008/src/code-stubs.h#newcode1041 src/code-stubs.h:1041: private: nit: classes, then methods, then data members. (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declaration_Order#Declaration_Order) http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc File src/ia32/code-stubs-ia32.cc (right): http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7047 src/ia32/code-stubs-ia32.cc:7047: // edi: source FixedArray Please move this comment to some place where the statement is true. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7065 src/ia32/code-stubs-ia32.cc:7065: // edi: source FixedArray You can omit all comments for registers whose contents have not changed since the last comment. Up to you. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7115 src/ia32/code-stubs-ia32.cc:7115: // Restore necessary values do store. I don't understand this comment. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7132 src/ia32/code-stubs-ia32.cc:7132: // Restore esp and esi before jumping into runtime. Slightly outdated. s/esp and esi/registers/? http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7141 src/ia32/code-stubs-ia32.cc:7141: void FastElementsConversionStub::GenerateDoubleToObject( TODO(jkummerow): continue reviewing here. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7159 src/ia32/code-stubs-ia32.cc:7159: // edx: receiver Again, you can omit comments about registers that haven't changed. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7164 src/ia32/code-stubs-ia32.cc:7164: // eax: destination FixedDoubleArray s/FixedDoubleArray/FixedArray/ http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7167 src/ia32/code-stubs-ia32.cc:7167: // edi: source FixedArray Please move this to where it's true. Also, s/FixedArray/FixedDoubleArray/. Or just delete this comment here since you have the correct version four lines below. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7176 src/ia32/code-stubs-ia32.cc:7176: // ebx: index of current element (smi-tagged) Move this comment. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7187 src/ia32/code-stubs-ia32.cc:7187: __ mov(esi, FieldOperand(edi, ebx, times_4, FixedDoubleArray::kHeaderSize)); You could check if SSE2 is available and if yes, do a single 64bit move using an xmm register, falling back to two 32bit moves via esi otherwise. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7223 src/ia32/code-stubs-ia32.cc:7223: __ mov(ebx, eax); This move is unnecessary. EAX is popped next anyway, so it's OK for it to be clobbered. http://codereview.chromium.org/8241003/diff/1008/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/8241003/diff/1008/src/ic.cc#newcode1784 src/ic.cc:1784: nit: remove this empty line http://codereview.chromium.org/8241003/diff/1008/test/mjsunit/elements-kind.js File test/mjsunit/elements-kind.js (right): http://codereview.chromium.org/8241003/diff/1008/test/mjsunit/elements-kind.js#newcode103 test/mjsunit/elements-kind.js:103: assertEquals(expected, getKind(obj), name_opt); Nice! I like this refactoring. http://codereview.chromium.org/8241003/diff/1008/test/mjsunit/elements-kind.js#newcode186 test/mjsunit/elements-kind.js:186: //%OptimizeFunctionOnNextCall(polymorphic); Please add a comment stating why this section is disabled. On a side note, I'd prefer /*...*/, but leave that up to you. http://codereview.chromium.org/8241003/diff/1008/test/mjsunit/elements-transition.js File test/mjsunit/elements-transition.js (right): http://codereview.chromium.org/8241003/diff/1008/test/mjsunit/elements-transition.js#newcode33 test/mjsunit/elements-transition.js:33: var me = new Array(length); As discussed, please add a comment explaining why every operation here needs to be performed for two identical arrays. http://codereview.chromium.org/8241003/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
