This patch is getting very close now. Make sure that there are tests and
address
these comments and I think we are ready to go.
http://codereview.chromium.org/2280007/diff/56001/57014
File src/codegen.cc (right):
http://codereview.chromium.org/2280007/diff/56001/57014#newcode264
src/codegen.cc:264: #if V8_TARGET_ARCH_IA32
I guess you can get rid of the #ifdef now?
http://codereview.chromium.org/2280007/diff/56001/57004
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/2280007/diff/56001/57004#newcode5971
src/ia32/codegen-ia32.cc:5971:
frame_->CallCallIC(RelocInfo::CODE_TARGET,
Either make this CallKeyedCallIC or change the full code generator to
share the EmitCallIC code passing in a code kind?
http://codereview.chromium.org/2280007/diff/56001/57010
File src/ia32/ic-ia32.cc (right):
http://codereview.chromium.org/2280007/diff/56001/57010#newcode1299
src/ia32/ic-ia32.cc:1299: __ push(ecx);
You should move the push and pop of ecx inside the
Enter/LeaveInternalFrame so that the GC knows that there is a potential
heap-object on the stack here. The current code is not GC safe.
http://codereview.chromium.org/2280007/diff/56001/57011
File src/ic.cc (right):
http://codereview.chromium.org/2280007/diff/56001/57011#newcode404
src/ic.cc:404: Handle<Object> object,
Indentation.
http://codereview.chromium.org/2280007/diff/56001/57012
File src/ic.h (right):
http://codereview.chromium.org/2280007/diff/56001/57012#newcode188
src/ic.h:188: class CallIC_Base: public IC {
Remove the underscore? CallICBase
http://codereview.chromium.org/2280007/diff/56001/57012#newcode234
src/ic.h:234: { ASSERT(target()->is_keyed_call_stub());}
Strange indentation:
KeyedCallIC() : CallICBase(Code::KEYED_CALL_IC) {
ASSERT(...);
}
http://codereview.chromium.org/2280007/diff/56001/57022
File src/log.cc (right):
http://codereview.chromium.org/2280007/diff/56001/57022#newcode1295
src/log.cc:1295: case Code::KEYED_CALL_IC:
It would be nice to know in the profiles if it is a keyed or a named
call IC. Please have a separate "A keyed call IC from the snapshot" and
have a separate logger tag for keyed calls.
http://codereview.chromium.org/2280007/diff/56001/57013
File src/stub-cache.cc (right):
http://codereview.chromium.org/2280007/diff/56001/57013#newcode711
src/stub-cache.cc:711: Code::ComputeFlags(kind, NOT_IN_LOOP,
MEGAMORPHIC, NORMAL, argc, 1);
We can avoid the extra argument to compute flags here in another way.
This is really a bit of a hack to make sure that we can use the stub
cache to share the call/keyed call miss stubs for the same arg count.
Instead of adding an extra argument, you can just use the
MONOMORPHIC_PROTOTYPE_FAILURE InlineCacheState since this is
pseudo-type which no actual IC stub has.
We should make sure to add a comment here stating that the use of
MONOMORPHIC_PROTOTYPE_FAILURE is a hack to share the stubs while making
sure that these stubs are never confused with normal IC stubs in the
cache.
http://codereview.chromium.org/2280007/diff/56001/57003
File src/stub-cache.h (right):
http://codereview.chromium.org/2280007/diff/56001/57003#newcode206
src/stub-cache.h:206: static Object* ComputeCallDebugBreak(int argc,
Code::Kind = Code::CALL_IC);
Let's avoid default arguments.
http://codereview.chromium.org/2280007/diff/56001/57003#newcode338
src/stub-cache.h:338: Handle<Code> ComputeCallMiss(int argc, Code::Kind
= Code::CALL_IC);
Let's avoid default arguments.
http://codereview.chromium.org/2280007/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
