Could you have another look Kevin. Resolving the value check in
CallFunctionStub
was easy. It is no longer needed.
I'll start the porting work.
http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):
http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#newcode1447
src/ia32/builtins-ia32.cc:1447: // Preserve the number of arguments on
the stack. Must preserve both
On 2011/05/18 15:57:23, Kevin Millikin wrote:
"both" --> ""
Done.
http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#newcode1495
src/ia32/builtins-ia32.cc:1495: __ mov(edi, -1); // account for
receiver
On 2011/05/18 15:57:23, Kevin Millikin wrote:
Not your code, but this loop could also count down the number of
arguments left
to push (after the current iteration, so it starts at arg_count to
include the
receiver). I.e., do not mutate the base address and instead make use
of the
mutated loop variable:
__ mov(edi, ebx);
__ bind(©);
__ push(Operand(eax, edi, times_4));
__ dec(edi);
__ j(not_sign, ©);
Won't this push the arguments in the wrong order? I would like to not
change this in this change. This is getting big and hairy as it is.
http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#newcode1514
src/ia32/builtins-ia32.cc:1514: // ebx = expected - actual.
On 2011/05/18 15:57:23, Kevin Millikin wrote:
Then here, I think you can use more or less the same trick:
__ lea(edi, Operand(ebp, eax, times_4, offset));
__ bind(©);
__ push(Operand(edi, eax, times_4));
__ dec(eax);
__ j(not_sign, ©);
__ bind(&fill);
__ push(Immediate(masm->isolate()->factory()->undefined_value()));
__ dec(ebx);
__ j(not_sign, &fill);
Same as above. I think this will push arguments in the wrong order. I
would like to leave as is for now.
http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc#newcode3916
src/ia32/code-stubs-ia32.cc:3916: if (ReceiverMightBeValue() ||
NonValueReceiverMightBeImplicit()) {
On 2011/05/18 15:57:23, Kevin Millikin wrote:
I can't figure out if these are separate bits or if they are mutually
exclusive.
If they are separate bits, this should be written as:
if (NonValueReceiverMightBeImplicit()) {
}
if (ReceiverMightBeValue()) {
}
If mutually exclusive:
if (NonValueReceiverMightBeImplicit()) {
} else if (ReceiverMightBeValue()) {
}
I can't actually hit the non-object case in the call function stub. I
cannot see how we can ever get in that situation. Certainly non of our
tests hit that case (I tested with an int3()). I'll just get rid of the
receiver value check completely.
Soren and I digged into it today. It was put in when we used
CallFunctionStub for keyed calls. We don't do that anymore (only for
synthetic where we know it is not a value) and therefore we cannot hit
this case.
http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc#newcode3927
src/ia32/code-stubs-ia32.cc:3927: __ mov(ebx, FieldOperand(ebx,
GlobalObject::kGlobalReceiverOffset));
On 2011/05/18 15:57:23, Kevin Millikin wrote:
This should go in eax? Otherwise the ReceiverMightBeValue TO_OBJECT
case below
is going to still see the hole.
Doesn't matter. We unconditionally jmp over the TO_OBJECT case if we see
the hole value here.
Will be dead code now.
http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc#newcode3929
src/ia32/code-stubs-ia32.cc:3929: // Mark as implicit receiver call.
On 2011/05/18 15:57:23, Kevin Millikin wrote:
We should be able to avoid setting ecx to check it below and see which
InvokeFunction to use (InvokeFunction will then set ecx again).
Instead we
could probably make use of the hole value/non-hole value we currently
have in
eax if it were preserved.
Done.
http://codereview.chromium.org/7039036/diff/5001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):
http://codereview.chromium.org/7039036/diff/5001/src/ia32/full-codegen-ia32.cc#newcode2209
src/ia32/full-codegen-ia32.cc:2209:
NON_VALUE_RECEIVER_MIGHT_BE_IMPLICIT);
On 2011/05/18 15:57:23, Kevin Millikin wrote:
Why not MIGHT_BE_VALUE | MIGHT_BE_IMPLICIT? Don't we dodge the value
check this
way?
(I had convinced myself a couple of weeks ago that this could be
NO_CALL_FUNCTION_FLAGS so I actually think it's OK, but I
conservatively didn't
change it then because I didn't think it would make a performance
difference.)
Yes, this does dodge the value check. I convinced myself that this was
OK. I'll add some more test cases to have proper test coverage for this
case.
Come to think of it I cannot actually see how we can get a non-value in
here. Adding an int3 where we call ToObject doesn't show anything in our
tests hitting the non-value CallFunctionStub path.
I will get rid of it completely.
http://codereview.chromium.org/7039036/diff/5001/src/ia32/full-codegen-ia32.cc#newcode2260
src/ia32/full-codegen-ia32.cc:2260: EmitCallWithStub(expr,
RECEIVER_MIGHT_BE_VALUE);
On 2011/05/18 15:57:23, Kevin Millikin wrote:
Why MIGHT_BE_VALUE? It wasn't before, but I can see that it might be
implicit
in the case of not being bound by a with.
Yes, I need to change the name of the flags to make this clearer. This
is as you say MIGHT_BE_IMPLICIT.
http://codereview.chromium.org/7039036/diff/5001/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):
http://codereview.chromium.org/7039036/diff/5001/src/ia32/macro-assembler-ia32.cc#newcode1515
src/ia32/macro-assembler-ia32.cc:1515: void
MacroAssembler::SetReceiverType(Register dst, ReceiverType type) {
On 2011/05/18 15:57:23, Kevin Millikin wrote:
I'm not sure it helps to have the register as an argument, since it is
fixed.
Maybe it's there to be explicit at the call sites? If so, you could
assert it's
ecx here.
The point was exactly to make it clear at the call site which register
is used. Otherwise it becomes very hard to figure out what is going on
when reading the call site. I will add the assert. Thanks!
http://codereview.chromium.org/7039036/diff/5001/src/v8globals.h
File src/v8globals.h (right):
http://codereview.chromium.org/7039036/diff/5001/src/v8globals.h#newcode506
src/v8globals.h:506: enum ReceiverType {
On 2011/05/18 15:57:23, Kevin Millikin wrote:
Maybe this is not quite the right name. There is no explicit receiver
in with
(...) { ... f() ... }. Maybe it's better to put the focus on the kind
of call:
enum CallKind { CALL_AS_METHOD, CALL_AS_FUNCTION }?
I agree. Your suggestion is better. Done.
http://codereview.chromium.org/7039036/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
