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#newcode586
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#newcode598
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#newcode1428
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#newcode1580
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.h#newcode2194
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-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/23537067/diff/6001/src/ia32/lithium-codegen-ia32.cc#newcode4375
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-call.js
File test/mjsunit/keyed-array-call.js (right):
https://codereview.chromium.org/23537067/diff/6001/test/mjsunit/keyed-array-call.js#newcode30
test/mjsunit/keyed-array-call.js:30: a.test = function(a) { return a+30;
}
How about tests that actually test the holey version, too?
https://codereview.chromium.org/23537067/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
