LGTM
http://codereview.chromium.org/2111011/diff/10001/11002 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/2111011/diff/10001/11002#newcode694 src/x64/codegen-x64.cc:694: } else if (receiver_.is(value_)) { I don't get this, can receiver_.is(value_) ever be true? http://codereview.chromium.org/2111011/diff/10001/11002#newcode699 src/x64/codegen-x64.cc:699: } else if (key_.is(value_)) { Ditto. http://codereview.chromium.org/2111011/diff/10001/11002#newcode704 src/x64/codegen-x64.cc:704: // Value is now in rax. Its original location is remembered in value_. Maybe extend comment to say that value_ is used later to store the result, and that receiver_ and key_ might not have their original values. Alternatively use two locals receiver and key to avoid overwriting receiver_ and key_ when swapping with rax. http://codereview.chromium.org/2111011/diff/10001/11002#newcode718 src/x64/codegen-x64.cc:718: } How about } else // receiver_ is neither rcx nor rdx. __ movq(rdx, receiver_); if (!key_.is(rcx)) { __ movq(rcx, key_); } } instead of } else if (key_.is(rcx)) { __ movq(rdx, receiver_); } else { __ movq(rcx, key_); __ movq(rdx, receiver_); } http://codereview.chromium.org/2111011/diff/10001/11005 File src/x64/ic-x64.cc (right): http://codereview.chromium.org/2111011/diff/10001/11005#newcode900 src/x64/ic-x64.cc:900: // rdx: JSArray JSArray -> receiver (a JSArray) Like in the comment below. http://codereview.chromium.org/2111011/diff/10001/11005#newcode1065 src/x64/ic-x64.cc:1065: __ pop(rdx); Please add the comment on rbx and rdi content here as well. http://codereview.chromium.org/2111011/diff/10001/11005#newcode1102 src/x64/ic-x64.cc:1102: __ bind(&is_nan); Please add comment on register content as above. http://codereview.chromium.org/2111011/diff/10001/11007 File src/x64/virtual-frame-x64.cc (right): http://codereview.chromium.org/2111011/diff/10001/11007#newcode1035 src/x64/virtual-frame-x64.cc:1035: // are a and b. Other results can be live, but must not be in a_reg or b_reg. Please assert these asumptions. http://codereview.chromium.org/2111011/diff/10001/11007#newcode1046 src/x64/virtual-frame-x64.cc:1046: // a must be in b_reg, b in a_reg. How about adding swapping of results to the virtual frame? Then you don´t need to rely on a and b being invalidated and the caller can decide when to invalidate. http://codereview.chromium.org/2111011/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
