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 On 2009/12/18 11:42:54, Kevin Millikin wrote:
Just the apply property of frame[3].
All of these comments rewritten. http://codereview.chromium.org/487017/diff/8006/10006#newcode673 src/x64/codegen-x64.cc:673: // Frame[3]: the function that we fetched .apply on. On 2009/12/18 11:42:54, Kevin Millikin wrote:
Not necessarily a function.
Right. This is now called "applicand" http://codereview.chromium.org/487017/diff/8006/10006#newcode738 src/x64/codegen-x64.cc:738: frame_->SyncRange(0, frame_->element_count() - 1); On 2009/12/18 11:42:54, Kevin Millikin wrote:
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. This entire sequence always ends in a function call, and it would be much clearer if it was in a spilled scope, starting before LoadAndSpill(receiver). This seems like a change that should be done separately from the change to Reference. I could leave this reference code as is, and clean it up with a later change to TryApplyLazy, or I could fix TryApplyLazy first. This definitely needs a rewrite, with a spilled frame, I think. http://codereview.chromium.org/487017/diff/8006/10006#newcode807 src/x64/codegen-x64.cc:807: Result a2 = frame_->Pop(); On 2009/12/18 11:42:54, Kevin Millikin wrote:
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);
I'm not even sure why we are loading them in the wrong order to begin with. In the fast case, we don't use the .apply function at Frame[2], and in the slow case, we always want to call Frame[3].Frame[2](Frame[1], Frame[0]), which has 3 and 2 in the wrong order. I would like to do this as a separate fix, perhaps first. http://codereview.chromium.org/487017/diff/8006/10006#newcode824 src/x64/codegen-x64.cc:824: frame_->Nip(1); On 2009/12/18 11:42:54, Kevin Millikin wrote:
Is it necessary to Nip (removing values from the middle of the frame
can be
avoided by not eagerly putting them on)?
I see what you are doing here, passing the Result rather than putting it on the stack, and it looks better. Can we use this if we have a spilled frame, or should we do it more directly in that case?
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/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 On 2009/12/18 11:42:54, Kevin Millikin wrote:
Not execution stack, virtual frame. Not "is", "can be".
Done. 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 On 2009/12/18 11:42:54, Kevin Millikin wrote:
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).
Done. http://codereview.chromium.org/487017/diff/8006/10007#newcode80 src/x64/codegen-x64.h:80: ASSERT(!is_unloaded()); On 2009/12/18 11:42:54, Kevin Millikin wrote:
Why shouldn't size() be called for unloaded references? It seems like
their
size is 0.
Done. http://codereview.chromium.org/487017
-- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
