PTAL. I did some of the easier modifications you proposed.
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 { On 2011/04/27 11:49:28, Kevin Millikin wrote:
I think this function and the next (GetArgumentsObjectSize) are no
longer
needed.
Done. 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)); On 2011/04/27 11:49:28, Kevin Millikin wrote:
I think some bit of this code is shared by all three GenerateNew...
functions.
We should probably put it in a separate helper.
Put in a TODO. 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; On 2011/04/27 11:49:28, Kevin Millikin wrote:
It strikes me that kDisplacement is really just StandardFrameConstants::kCallerSPOffset (though that's a longer name
it might be
clearer to use it here).
Done.
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.
I'm not entirely sure what you are referring to here. 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)); On 2011/04/27 11:49:28, Kevin Millikin wrote:
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).
Does it matter much? 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)); On 2011/04/27 11:49:28, Kevin Millikin wrote:
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.
True. I leave that for later. 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())); On 2011/04/27 11:49:28, Kevin Millikin wrote:
I think the size is a constant here.
Done (here and elsewhere). http://codereview.chromium.org/6824081/diff/11001/src/ia32/code-stubs-ia32.cc#newcode3839 src/ia32/code-stubs-ia32.cc:3839: __ push(ecx); On 2011/04/27 11:49:28, Kevin Millikin wrote:
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.
Yes, I felt that clarity was more important here. http://codereview.chromium.org/6824081/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
