http://codereview.chromium.org/487017/diff/8006/10006 File src/x64/codegen-x64.cc (right):
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, On 2009/12/18 11:42:54, Kevin Millikin wrote:
This comment is misleading. Doesn't SetValue now remove the
reference? Done. http://codereview.chromium.org/487017/diff/8006/10006#newcode1826 src/x64/codegen-x64.cc:1826: frame_->Drop(); On 2009/12/18 11:42:54, Kevin Millikin wrote:
Why not drop the value and the duplicate?
frame_->Drop(2)
Done. http://codereview.chromium.org/487017/diff/8006/10006#newcode1829 src/x64/codegen-x64.cc:1829: // that it doesn't matter whether a value (eg, ebx pushed above) is On 2009/12/17 09:59:02, Lasse Reichstein wrote:
ebx->rbx
Done. http://codereview.chromium.org/487017/diff/8006/10006#newcode1831 src/x64/codegen-x64.cc:1831: each.SetValue(NOT_CONST_INIT); On 2009/12/18 11:42:54, Kevin Millikin wrote:
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.
Will be done in a separate change. http://codereview.chromium.org/487017/diff/8006/10006#newcode1881 src/x64/codegen-x64.cc:1881: { Reference ref(this, node->catch_var()); On 2009/12/18 11:42:54, Kevin Millikin wrote:
No real need to have a Reference here. I'm pretty sure this is always
a
VariableProxy that is a Slot with type LOCAL.
Left unchanged. http://codereview.chromium.org/487017/diff/8006/10006#newcode2673 src/x64/codegen-x64.cc:2673: if (target.size() == 1) { On 2009/12/18 11:42:54, Kevin Millikin wrote:
if (target.type() == Reference::NAMED) seems clearer.
Done. http://codereview.chromium.org/487017/diff/8006/10006#newcode2734 src/x64/codegen-x64.cc:2734: frame_->Push(&receiver); On 2009/12/17 09:59:02, Lasse Reichstein wrote:
Could this be made faster by doing, effectively: pop(tmp) push(Operand(sp)) // aka Dup. mov(Operand(sp,kPointerSize), tmp) It seems like one less operation - but maybe not if the frame is
virtual. I think the frame may well be virtual, and it is clearer the way it is currently written, I think. http://codereview.chromium.org/487017/diff/8006/10006#newcode2958 src/x64/codegen-x64.cc:2958: frame_->Push(&receiver); On 2009/12/18 08:16:55, William Hesse wrote:
On 2009/12/17 09:59:02, Lasse Reichstein wrote: > Again a swap of the top two elements of the frame. > Perhaps give the frame a Swap operation.
This may be a good idea. I'm not sure how much to optimize the
resulting Swap
operation, though.
The real problem is that calling x.foo(a,b,c) needs to evaluate x, load x.foo, then call x.foo( x, a, b,c). So x is needed to load x.foo, but also needed on the stack above x.foo, to the the "this" argument to the call. 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(), On 2009/12/18 11:42:54, Kevin Millikin wrote:
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.
Done. http://codereview.chromium.org/487017
-- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
