Urgh, this change is getting massive. With the size of this I'm afraid I
have
missed something. Let's see if we can chop changes into smaller parts going
forward. :-)
I have some comments. Definitely looks like this is getting there.
http://codereview.chromium.org/6894003/diff/35002/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):
http://codereview.chromium.org/6894003/diff/35002/src/arm/ic-arm.cc#newcode1136
src/arm/ic-arm.cc:1136: :
ExternalReference(IC_Utility(kKeyedStoreIC_Miss), masm->isolate());
Really funky indentation. ;-)
http://codereview.chromium.org/6894003/diff/35002/src/arm/ic-arm.cc#newcode1153
src/arm/ic-arm.cc:1153:
ExternalReference(IC_Utility(kKeyedStoreIC_Slow), masm->isolate());
What is the idea with the slow case here? Goes directly to the runtime
system without being treated as a miss so that we do not perform an IC
state transition? So this is for when we bail out of a store ic because
something that has got nothing to do with the type of the object failed?
http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):
http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#newcode3093
src/arm/stub-cache-arm.cc:3093: KeyedLoadIC keyed_load_ic(isolate());
Isn't this called from a KeyedLoadIC instance? Can't we pass in the
other stub to the generation instead? I find it a little strange to
create a KeyedLoadIC instance here.
http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#newcode3095
src/arm/stub-cache-arm.cc:3095:
keyed_load_ic.ComputeMonomorphicBuiltinFastElementStub(
Are any of these stubs actually builtin? This is the dynamically
generated stub that does not perform map checks, right? Maybe find a
name that does not contain 'Builtin'?
http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#newcode3103
src/arm/stub-cache-arm.cc:3103: false);
Creating an enum for this would make this call site a bit easier to
read. CHECK_SMI/DONT_CHECK_SMI?
http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#newcode3190
src/arm/stub-cache-arm.cc:3190: KeyedStoreIC keyed_store_ic(isolate());
I would prefer passing in the actual stub here and dealing with
allocation failures at the higher level at the caller. Creating
KeyedStoreICs outside of ic.cc seems a bit fishy to me.
Same comment for the other occurrences. :-)
http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#newcode3785
src/arm/stub-cache-arm.cc:3785: __ ldr(r3, FieldMemOperand(receiver,
JSObject::kElementsOffset));
Have you always checked that the receiver is not a smi when you get
here?
http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h
File src/code-stubs.h (right):
http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h#newcode993
src/code-stubs.h:993: class KeyedStoreStub : public CodeStub {
Why not just have this one and have a minor key for Object and one for
Array? They share code generation.
http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h#newcode1037
src/code-stubs.h:1037: Major MajorKey() {
Just don't implement Major and Minor key?
http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h#newcode1069
src/code-stubs.h:1069: #define DECLARE_EXTERNAL_ARRAY_STUB(op, type)
\
Do you really need all of these different types here. They share code
generation, right? So, why not just have ExternalArrayLoadStub with a
minor key that is the type (byte, unsigned, ...)?
http://codereview.chromium.org/6894003/diff/35002/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/6894003/diff/35002/src/ia32/stub-cache-ia32.cc#newcode718
src/ia32/stub-cache-ia32.cc:718:
Builtins::kKeyedLoadIC_MissForceGeneric);
four-space indent?
http://codereview.chromium.org/6894003/diff/35002/src/ia32/stub-cache-ia32.cc#newcode3641
src/ia32/stub-cache-ia32.cc:3641: NearLabel done;
Accidental edit? There are more than this one I think.
http://codereview.chromium.org/6894003/diff/35002/src/ic.cc
File src/ic.cc (right):
http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1070
src/ic.cc:1070: if (is_js_array) {
If you simplify the stub hierarchy this can be just:
KeyedLoadStub stub(is_js_array);
return stub.TryGetCode();
http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1082
src/ic.cc:1082: switch (array_type) {
ExternalArrayLoadStub stub(array_type);
return stub.TryGetCode();
?
http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1656
src/ic.cc:1656: // the MONOMORPHIC stubs to jump into for all of the
receiver types that it
Is this comment correct any more? You might need the monomorphic stub
for the typed arrays to get the external array type, but these are not
the stubs that you use to compose the actual megamorphic stubs, right?
http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1692
src/ic.cc:1692: Object* maybe_cached_stub =
receiver->map()->FindInCodeCache(megamorphic_name,
I'm not sure I understand why you cache this stub on the current
receiver's map. It seems to me that this can create a rather strange mix
of types that are unrelated to this load/store site? I'm concerned that
the type feedback generated here will be too polymorphic.
It seems to me that if we want to cache these stubs we should have some
sort of global stub cache instead of caching on one of the maps
supported by the stub. It would be nice to be able to share these by
having a mapping from a list of maps to a stub handling them.
http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1811
src/ic.cc:1811: MaybeObject* maybe_update =
receiver->UpdateMapCodeCache(name, result);
It seems to me that something like this should be on the stub cache. In
particular, we need to make sure that the profiler knows about all
generated code. Not clear to me if it does? It doesn't follow the normal
pattern at least, but maybe it can't?
http://codereview.chromium.org/6894003/diff/35002/src/ic.h
File src/ic.h (right):
http://codereview.chromium.org/6894003/diff/35002/src/ic.h#newcode337
src/ic.h:337: virtual MaybeObject*
ComputeMonomorphicBuiltinFastElementStub(
Remove the Builtin from the name since it is not a builtin but a normal
stub?
http://codereview.chromium.org/6894003/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
