LGTM. Feel free to treat the comments below as TODO's (i.e., commit this
and
you or I can clean up the huge chunk of assembly code when we get a chance).
http://codereview.chromium.org/6824081/diff/11001/src/code-stubs.h
File src/code-stubs.h (right):
http://codereview.chromium.org/6824081/diff/11001/src/code-stubs.h#newcode690
src/code-stubs.h:690: int GetArgumentsBoilerplateIndex() const {
I think this function and the next (GetArgumentsObjectSize) are no
longer needed.
http://codereview.chromium.org/6824081/diff/11001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
http://codereview.chromium.org/6824081/diff/11001/src/ia32/code-stubs-ia32.cc#newcode3671
src/ia32/code-stubs-ia32.cc:3671: __ mov(edx, Operand(ebp,
StandardFrameConstants::kCallerFPOffset));
I think some bit of this code is shared by all three GenerateNew...
functions. We should probably put it in a separate helper.
http://codereview.chromium.org/6824081/diff/11001/src/ia32/code-stubs-ia32.cc#newcode3698
src/ia32/code-stubs-ia32.cc:3698: static const int kDisplacement = 2 *
kPointerSize;
It strikes me that kDisplacement is really just
StandardFrameConstants::kCallerSPOffset (though that's a longer name it
might be clearer to use it here).
Also, this whole code is actually off by one by pointing to the
receiver, which we don't need. So we could adjust by using
StandardFrameConstants::kCallerSPOffset. That might also be clearer.
http://codereview.chromium.org/6824081/diff/11001/src/ia32/code-stubs-ia32.cc#newcode3701
src/ia32/code-stubs-ia32.cc:3701: __ mov(ebx, Operand(esp, 1 *
kPointerSize));
We have just pushed this before the call. We could consider passing it
in a register instead (we would possibly have to spill it eventually).
http://codereview.chromium.org/6824081/diff/11001/src/ia32/code-stubs-ia32.cc#newcode3717
src/ia32/code-stubs-ia32.cc:3717: __ mov(ecx, Operand(edx,
ArgumentsAdaptorFrameConstants::kLengthOffset));
Also, instead of computing the non-adaptor receiver address in the
caller, and patching it for the adaptor case in the callee, we could
compute both cases in the callee---it might be clearer.
http://codereview.chromium.org/6824081/diff/11001/src/ia32/code-stubs-ia32.cc#newcode3749
src/ia32/code-stubs-ia32.cc:3749: __ add(Operand(ebx),
Immediate(GetArgumentsObjectSize()));
I think the size is a constant here.
http://codereview.chromium.org/6824081/diff/11001/src/ia32/code-stubs-ia32.cc#newcode3839
src/ia32/code-stubs-ia32.cc:3839: __ push(ecx);
It's unfortunate that the parameters are in the reverse order, that's
due to trying to avoid unneeded context slots for ones that are shadowed
by other parameters.
There may be a way to avoid this push (by using some trick that allows
us to use the index to compute addresses in the parameter map and
backing store from some 'phantom' base address), but I doubt the
performance is worth the extra complication.
http://codereview.chromium.org/6824081/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
