I just commented on the x64 version (from patch set 7). It seems like the right idea, but I'm hoping you can simplify or clarify a few more things.
http://codereview.chromium.org/487017/diff/8006/10006 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/8006/10006#newcode672 src/x64/codegen-x64.cc:672: // Frame[2]: the function Function.prototype.apply, or some other .apply Just the apply property of frame[3]. http://codereview.chromium.org/487017/diff/8006/10006#newcode673 src/x64/codegen-x64.cc:673: // Frame[3]: the function that we fetched .apply on. Not necessarily a function. http://codereview.chromium.org/487017/diff/8006/10006#newcode738 src/x64/codegen-x64.cc:738: frame_->SyncRange(0, frame_->element_count() - 1); I'm not sure this is quite the way to write it. You should either (a) use a SpilledScope so we get (some) debug asserts that we don't have any virtual elements on the frame and so we can see where it ends, or (b) allow the register allocator to work for this code. For instance, I don't think there's any particular reason to spill the entire frame so that you can fetch the function from memory into rdi explicitly. If it was already in some other register, that would be fine for this snippet. http://codereview.chromium.org/487017/diff/8006/10006#newcode807 src/x64/codegen-x64.cc:807: Result a2 = frame_->Pop(); This code snippet seems like it could be streamlined too. There's no need to move the args from memory to a register and then back to memory (for the call). Why not just: Result app = frame_->ElementAt(2); Result fun = frame_->ElementAt(3); frame_->SetElementAt(2, &fun); frame_->SetElementAt(3, &app); http://codereview.chromium.org/487017/diff/8006/10006#newcode824 src/x64/codegen-x64.cc:824: frame_->Nip(1); Is it necessary to Nip (removing values from the middle of the frame can be avoided by not eagerly putting them on)? From invoke: Result result = allocator()->Allocate(rax); ASERT(result.is_valid()); frame_->Drop(); // Discard the function. done.Jump(&result); And here: Result res = frame_->CallStub(&call_function, 3); if (try_lazy) done.Bind(&res); frame_->RestoreContextRegister(); frame_->SetElementAt(0, &res); // Overwrite the duplicate of the receiver of '.apply'. Please be careful about the terminology in comments. The callee is not necessarily a function. http://codereview.chromium.org/487017/diff/8006/10006#newcode1822 src/x64/codegen-x64.cc:1822: // unloading the reference itself (which preserves the top of stack, This comment is misleading. Doesn't SetValue now remove the reference? http://codereview.chromium.org/487017/diff/8006/10006#newcode1826 src/x64/codegen-x64.cc:1826: frame_->Drop(); Why not drop the value and the duplicate? frame_->Drop(2) http://codereview.chromium.org/487017/diff/8006/10006#newcode1831 src/x64/codegen-x64.cc:1831: each.SetValue(NOT_CONST_INIT); And drop the value here frame_->Drop() Even better (you should probably just make it a separate change) is that GetValue and SetValue can return Results instead of communicating them via the frame. Makes the code a little more straightforward. http://codereview.chromium.org/487017/diff/8006/10006#newcode1881 src/x64/codegen-x64.cc:1881: { Reference ref(this, node->catch_var()); No real need to have a Reference here. I'm pretty sure this is always a VariableProxy that is a Slot with type LOCAL. http://codereview.chromium.org/487017/diff/8006/10006#newcode2673 src/x64/codegen-x64.cc:2673: if (target.size() == 1) { if (target.type() == Reference::NAMED) seems clearer. http://codereview.chromium.org/487017/diff/8006/10006#newcode2944 src/x64/codegen-x64.cc:2944: { I don't understand why this block scope is here? The destructor for the reference just asserts it's unloaded and the destructor for the result unuses it but it's unused by the push. http://codereview.chromium.org/487017/diff/8006/10006#newcode2954 src/x64/codegen-x64.cc:2954: { Likewise this block scope. I'm scratching my head trying to figure out why it's here. http://codereview.chromium.org/487017/diff/8006/10006#newcode3349 src/x64/codegen-x64.cc:3349: if (is_postfix) frame_->SetElementAt(is_const ? 0 : target.size(), OK, but it seems awkward. If Reference::size just reports the space currently used by the reference on the frame (ie, illegal and unloaded are both 0), then you can just use the old code. http://codereview.chromium.org/487017/diff/8006/10006#newcode4919 src/x64/codegen-x64.cc:4919: { Reference shadow_ref(this, scope_->arguments_shadow()); It's simplest to just avoid the Reference type completely here (you already don't use it for getting). Slot* shadow = scope_->arguments_shadow()->var()->slot(); Slot* arguments = scope_->arguments()->var()->slot(); ... if (!skip_arguments) { StoreToSlot(arguments, NOT_CONST_INIT); if (mode == LAZY_ARGUMENTS_ALLOCATION) done.Bind(); } StoreToSlot(shadow, NOT_CONST_INIT); http://codereview.chromium.org/487017/diff/8006/10007 File src/x64/codegen-x64.h (right): http://codereview.chromium.org/487017/diff/8006/10007#newcode47 src/x64/codegen-x64.h:47: // reference on the execution stack. The reference is consumed Not execution stack, virtual frame. Not "is", "can be". http://codereview.chromium.org/487017/diff/8006/10007#newcode50 src/x64/codegen-x64.h:50: // it has been consumed, and is in state UNLOADED. For variables Not necessarily in state UNLOADED (or consumed, actually, since ILLEGAL doesn't get consumed). Asserts that it is not on the virtual frame. Talk of variables is misleading because global variables are treated as properties (and so are references to arguments in functions that use the arguments object). http://codereview.chromium.org/487017/diff/8006/10007#newcode80 src/x64/codegen-x64.h:80: ASSERT(!is_unloaded()); Why shouldn't size() be called for unloaded references? It seems like their size is 0. http://codereview.chromium.org/487017 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
