Hi Bill, I haven't considered how easy it would be to do, but it seems nicer to never (re)materialize the receiver and possibly key on the frame for non-compound assignment loads, rather than materializing them and then dropping them.
In the case of an inlined property load, we can end up (1) emitting code to move a non-live receiver or key value to a register on the slow path (meanwhile increasing register pressure and possibly introducing spills on the fast path) or else (2) materializing a non-live receiver or key value on the stack on the fast path only to drop it on all paths. That means moving the check for compound assignment into the different cases of inlined/non-inlined slot/property/keyed property loads. It might be nice to see how much of a difference it makes (or if my intuition is completely wrong). http://codereview.chromium.org/487017/diff/2003/2004 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/2003/2004#newcode4357 src/x64/codegen-x64.cc:4357: Expression* expression, Tabs used for indentation? http://codereview.chromium.org/487017/diff/2003/2004#newcode4359 src/x64/codegen-x64.cc:4359: : cgen_(cgen), expression_(expression), Our normal style is to put these all on the same line or else one per line. http://codereview.chromium.org/487017/diff/2003/2004#newcode4366 src/x64/codegen-x64.cc:4366: cgen_->UnloadReference(this); No, ASSERT(is_invalid()). Reference should have been unloaded or never loaded (for reference errors). (or is_unloaded() or whatever you call it). http://codereview.chromium.org/487017/diff/2003/2004#newcode4423 src/x64/codegen-x64.cc:4423: if (!ref->is_illegal()) { I'm not sure why this check. The size of the illegal reference is zero, so it seems unnecessary. Moreover I would expect that we would not want to ever call UnloadReference on one that is not loaded. Just ASSERT(!is_invalid()) or whatever you call it. http://codereview.chromium.org/487017/diff/2003/2004#newcode4425 src/x64/codegen-x64.cc:4425: } I expected this to set the type to INVALID or UNLOADED or whatever. You wouldn't want to call UnloadReference without changing the type, would you? http://codereview.chromium.org/487017/diff/2003/2004#newcode6043 src/x64/codegen-x64.cc:6043: type_ = ILLEGAL; No need to change the type if you let UnloadReference do it. http://codereview.chromium.org/487017/diff/2003/2004#newcode6082 src/x64/codegen-x64.cc:6082: if (!is_compound_assignment_) { Why not ASSERT(is_compound_assignment()) instead? http://codereview.chromium.org/487017/diff/2003/2004#newcode6217 src/x64/codegen-x64.cc:6217: type_ = ILLEGAL; Again, could consider letting UnloadReference change the type to simplify the protocol. http://codereview.chromium.org/487017/diff/2003/2005 File src/x64/codegen-x64.h (right): http://codereview.chromium.org/487017/diff/2003/2005#newcode56 src/x64/codegen-x64.h:56: enum Type { ILLEGAL = -1, SLOT = 0, NAMED = 1, KEYED = 2 }; It seems strange that ILLEGAL now means both "illegal left-hand side (throw reference error)" and "already been unloaded". Perhaps "INVALID" (or even UNLOADED or NOT_LOADED) is a better name. http://codereview.chromium.org/487017/diff/2003/2005#newcode59 src/x64/codegen-x64.h:59: Expression* expression, Are these tabs used for indentation? http://codereview.chromium.org/487017 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
