Add support for keyed-call on arrays of fast elements

Add support for keyed-call on arrays of fast elements

R=danno@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=17746

[Toon Verwaest, 2013-09-24 16:24:43]

PTAL. Ia32 only for now.

[danno, 2013-10-02 08:49:10]

https://codereview.chromium.org/23537067/diff/6001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/23537067/diff/6001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:586: }
This is a total hack. I am not sure how to do it better, but my intuition is
that it more properly belongs in the the graph or compiler-specific pieces that
calculate the size of the parameter area. Maybe the graph builder needs a
virtual method to compute the parameter count that by default uses the existing
logic and is overridden in this case to look at argc.

https://codereview.chromium.org/23537067/diff/6001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:598: map_array, kind_index,
static_cast<HValue*>(NULL), FAST_ELEMENTS);
Please find a way to share this code in some way with
HGraphBuilder::JSArrayBuilder::EmitMapCode.

https://codereview.chromium.org/23537067/diff/6001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/23537067/diff/6001/src/code-stubs.h#newcode1039
src/code-stubs.h:1039: class ArgcBits: public BitField<int, 5, 20> {};
Don't we have a constant somewhere with the number of max args and max arg bits?
Please try to link them together somehow, either by using a shared constant or
adding a STATIC_ASSERT() here that depends on existing constants.

https://codereview.chromium.org/23537067/diff/6001/src/code-stubs.h#newcode2360
src/code-stubs.h:2360: class FPRegisters:       public BitField<bool,           
    0, 1> {};
nit: crazy unnecessary spacing on the line above.

https://codereview.chromium.org/23537067/diff/6001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/23537067/diff/6001/src/deoptimizer.cc#newcode...
src/deoptimizer.cc:1428: PrintF("  translating %s =>
CallStubFailureTrampolineStub, height=%d\n",
You can use a single statement and just switch the stub kind string with a %s
argument.

https://codereview.chromium.org/23537067/diff/6001/src/deoptimizer.cc#newcode...
src/deoptimizer.cc:1580:
CallStubFailureTrampolineStub().FindCodeInCache(&trampoline, isolate_);
Is this only going to every be used with CallStubs? It would be nice if the
trampoline didn't have such a specific name, e.g.
StubFailureNormalContinuationTrampoline and
StubfailureTailCallContinuationTrampoline rather than CallStub specific.

https://codereview.chromium.org/23537067/diff/6001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/23537067/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:2194: bool tailcall)
Can you please turn this boolean into an enum so that it's clear at the call
site what the meaning of the parameter is, e.g.

enum CallFunctionMode {
  NORMAL_FUNCTION_CALL,
  TAIL_FUNCTION_CALL
}

This would need to get propagated through the various signatures and storage
sites, too.

https://codereview.chromium.org/23537067/diff/6001/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/23537067/diff/6001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:4375: if (instr->hydrogen()->tailcall()) {
this should probably be called IsTailCall()

https://codereview.chromium.org/23537067/diff/6001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/23537067/diff/6001/src/ic.cc#newcode840
src/ic.cc:840: if (object->IsJSArray() && key->IsSmi()) {
A comment or two would be very useful here to describe the high-level logic of
following fragment.

https://codereview.chromium.org/23537067/diff/6001/src/ic.cc#newcode2419
src/ic.cc:2419: Object* key = args[0];
Oh, this is awful. Please take a look at ArrayConstructor, which through the
CodeStubFailureTrampoline has access to the caller's arguments in args[0].
Please extend the current stub failure mechanism to set up the failure
trampoline frame correctly so that you can just do:

Arguments callerArgs* = reinterpret_cast<Arguments*>(args[0]);

https://codereview.chromium.org/23537067/diff/6001/test/mjsunit/keyed-array-c...
File test/mjsunit/keyed-array-call.js (right):

https://codereview.chromium.org/23537067/diff/6001/test/mjsunit/keyed-array-c...
test/mjsunit/keyed-array-call.js:30: a.test = function(a) { return a+30; }
How about tests that actually test the holey version, too?

[Toon Verwaest, 2013-10-02 16:28:16]

Addressed comments, PTAL again.

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/code-stubs-hydr...
File src/code-stubs-hydrogen.cc (right):

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/code-stubs-hydr...
src/code-stubs-hydrogen.cc:586: }
On 2013/10/02 08:49:11, danno wrote:
> This is a total hack. I am not sure how to do it better, but my intuition is
> that it more properly belongs in the the graph or compiler-specific pieces
that
> calculate the size of the parameter area. Maybe the graph builder needs a
> virtual method to compute the parameter count that by default uses the
existing
> logic and is overridden in this case to look at argc.

Done.

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/code-stubs-hydr...
src/code-stubs-hydrogen.cc:598: map_array, kind_index,
static_cast<HValue*>(NULL), FAST_ELEMENTS);
On 2013/10/02 08:49:11, danno wrote:
> Please find a way to share this code in some way with
> HGraphBuilder::JSArrayBuilder::EmitMapCode. 

Done.

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/code-stubs.h
File src/code-stubs.h (right):

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/code-stubs.h#ne...
src/code-stubs.h:1039: class ArgcBits: public BitField<int, 5, 20> {};
On 2013/10/02 08:49:11, danno wrote:
> Don't we have a constant somewhere with the number of max args and max arg
bits?
> Please try to link them together somehow, either by using a shared constant or
> adding a STATIC_ASSERT() here that depends on existing constants.

Done.

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/code-stubs.h#ne...
src/code-stubs.h:2360: class FPRegisters:       public BitField<bool,           
    0, 1> {};
On 2013/10/02 08:49:11, danno wrote:
> nit: crazy unnecessary spacing on the line above.

Done.

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/deoptimizer.cc#...
src/deoptimizer.cc:1428: PrintF("  translating %s =>
CallStubFailureTrampolineStub, height=%d\n",
On 2013/10/02 08:49:11, danno wrote:
> You can use a single statement and just switch the stub kind string with a %s
> argument.

Done.

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/deoptimizer.cc#...
src/deoptimizer.cc:1580:
CallStubFailureTrampolineStub().FindCodeInCache(&trampoline, isolate_);
On 2013/10/02 08:49:11, danno wrote:
> Is this only going to every be used with CallStubs? It would be nice if the
> trampoline didn't have such a specific name, e.g.
> StubFailureNormalContinuationTrampoline and
> StubfailureTailCallContinuationTrampoline rather than CallStub specific.

Done.

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/hydrogen-instru...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/hydrogen-instru...
src/hydrogen-instructions.h:2194: bool tailcall)
On 2013/10/02 08:49:11, danno wrote:
> Can you please turn this boolean into an enum so that it's clear at the call
> site what the meaning of the parameter is, e.g.
> 
> enum CallFunctionMode {
>   NORMAL_FUNCTION_CALL,
>   TAIL_FUNCTION_CALL
> }
> 
> This would need to get propagated through the various signatures and storage
> sites, too.

Done.

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/ia32/lithium-co...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/ia32/lithium-co...
src/ia32/lithium-codegen-ia32.cc:4375: if (instr->hydrogen()->tailcall()) {
On 2013/10/02 08:49:11, danno wrote:
> this should probably be called IsTailCall()

Done.

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/23537067/diff/6001/src/ic.cc#newcode2419
src/ic.cc:2419: Object* key = args[0];
On 2013/10/02 08:49:11, danno wrote:
> Oh, this is awful. Please take a look at ArrayConstructor, which through the
> CodeStubFailureTrampoline has access to the caller's arguments in args[0].
> Please extend the current stub failure mechanism to set up the failure
> trampoline frame correctly so that you can just do:
> 
> Arguments callerArgs* = reinterpret_cast<Arguments*>(args[0]);

Done.

[danno, 2013-10-11 16:13:24]

https://codereview.chromium.org/23537067/diff/13001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/23537067/diff/13001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:1403: if (instr->IsTailCall()) return
DefineFixed(result, eax);
nit: I think I would it better if you did:

LInstruction* result = DefineFixed(result, eax);
return instr->IsTailCall() ? result : MarkAsCall(result);

https://codereview.chromium.org/23537067/diff/13001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/23537067/diff/13001/src/ic.cc#newcode841
src/ic.cc:841: if (object->IsJSArray() && key->IsSmi()) {
Some comments for this logic would be good. It's not clear to me what the next
15 lines is trying to accomplish without perusing the code.

[Toon Verwaest, 2013-11-04 13:59:44]

Addressed comments, PTAL again.

https://chromiumcodereview.appspot.com/23537067/diff/13001/src/ia32/lithium-i...
File src/ia32/lithium-ia32.cc (right):

https://chromiumcodereview.appspot.com/23537067/diff/13001/src/ia32/lithium-i...
src/ia32/lithium-ia32.cc:1403: if (instr->IsTailCall()) return
DefineFixed(result, eax);
On 2013/10/11 16:13:24, danno wrote:
> nit: I think I would it better if you did:
> 
> LInstruction* result = DefineFixed(result, eax);
> return instr->IsTailCall() ? result : MarkAsCall(result);

Done.

https://chromiumcodereview.appspot.com/23537067/diff/13001/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/23537067/diff/13001/src/ic.cc#newcode841
src/ic.cc:841: if (object->IsJSArray() && key->IsSmi()) {
On 2013/10/11 16:13:24, danno wrote:
> Some comments for this logic would be good. It's not clear to me what the next
> 15 lines is trying to accomplish without perusing the code.

Done.

[danno, 2013-11-14 13:18:56]

lgtm

[Toon Verwaest, 2013-11-14 13:46:41]

Committed patchset #9 manually as r17746 (presubmit successful).