Added comments. I like the overall approach. It's pretty simple.
In addition to the comments, please use enums instead of bools (as you
already
told me you would).
https://codereview.chromium.org/172523002/diff/160001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/172523002/diff/160001/src/code-stubs.h#newcode837
src/code-stubs.h:837: bool strict_native() const { return
state_.strict_native(); }
is_strict_or_native?
https://codereview.chromium.org/172523002/diff/160001/src/debug.h
File src/debug.h (right):
https://codereview.chromium.org/172523002/diff/160001/src/debug.h#newcode454
src/debug.h:454: static void GenerateCallICDebugBreak(MacroAssembler*
masm);
Seems like this function is dead. If so, can you remove it?
https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia32.cc#newcode2424
src/ia32/code-stubs-ia32.cc:2424: void
CallICStub::GenerateCall(MacroAssembler* masm, bool monomorphic,
one arg per line
https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia32.cc#newcode2528
src/ia32/code-stubs-ia32.cc:2528: __ cmp(ecx, edi);
Wouldn't it be faster to flip over the conditions, ie, inline the fast
path as early as possible and jump farther only in the miss / generic
cases.
https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia32.cc#newcode2534
src/ia32/code-stubs-ia32.cc:2534: // If we got here we went megamorphic.
Don't bother missing, just update.
we went -> If we get here, go from monomorphic to megamorphic. Don't ..
https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia32.cc#newcode2544
src/ia32/code-stubs-ia32.cc:2544: // Verify still monomorphic, miss
otherwise
Verify whether the input function matches the recorded monomorphic
target?
https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia32.cc#newcode2548
src/ia32/code-stubs-ia32.cc:2548: __ j(equal, &mono);
Same here as above. This path should be inlined here.
https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):
https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-ia32.cc#newcode2609
src/ia32/full-codegen-ia32.cc:2609: CallIC(ic); // NOTE: no type
feedback id.
What about something like:
// Don't assign a type feedback id to the IC, since type feedback is
provided by the vector above.
https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-ia32.cc#newcode2652
src/ia32/full-codegen-ia32.cc:2652: Handle<Code> ic =
CallIC::initialize_stub(isolate(), arg_count,
Doesn't this fit on one line? Otherwise
... CallIC::initialize_stub(
isolate(), arg_count, true);
https://codereview.chromium.org/172523002/diff/160001/src/ia32/full-codegen-ia32.cc#newcode2671
src/ia32/full-codegen-ia32.cc:2671: void
FullCodeGenerator::EmitCallWithStub(Call* expr) {
I guess we can now remove this function and just use EmitCallWithIC?
CallWithStub became a CallWithIC, so at least the name makes no sense
anymore.
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc
File src/ic.cc (right):
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode95
src/ic.cc:95: if (new_target->kind() != Code::CALL_IC) {
Isn't this only relevant for KEYED_STORE_IC?
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1800
src/ic.cc:1800: if (feedback->IsJSFunction()) {
Shouldn't this be feedback->IsJSFunction() && function->IsJSFunction() ?
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1802
src/ic.cc:1802: // otherwise the slow path will be taken.
Weird sentence
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1805
src/ic.cc:1805: // Do we need to patch? Only if we currently don't have
the default stub
We only need to patch if ...
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1815
src/ic.cc:1815: // Ugh. Do nothing if the function isn't a function, the
slow path is taken.
Remove ugh.
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1836
src/ic.cc:1836: // Ideally we can just patch the cell and return.
Are we sure that we can only get here because of snapshot code?
Why not just: Stay monomorphic if the type feedback in the vector was
cleared, but the IC wasn't (and is still monomorphic).
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1846
src/ic.cc:1846: Handle<String> name;
Don't you want to initialize this to something like the empty_string?
https://codereview.chromium.org/172523002/diff/160001/src/ic.h
File src/ic.h (right):
https://codereview.chromium.org/172523002/diff/160001/src/ic.h#newcode314
src/ic.h:314: static int ExtractArgcFromMinorKey(int minor_key) {
minor key -> ExtraICState? Only code stubs have major / minor keys. The
rest of the IC mechanism talks about extra IC state (which is encoded in
the code flags, independent of where the minor / major stuff is
encoded).
https://codereview.chromium.org/172523002/diff/160001/src/ic.h#newcode330
src/ic.h:330: STATIC_ASSERT(Code::kArgumentsBits == 16);
What about just using kArgumentsBits in the counts below so we don't
need to change it by hand later?
https://codereview.chromium.org/172523002/diff/160001/src/ic.h#newcode336
src/ic.h:336: // We have a few extra bits.
Not sure how many you have left. Aren't these encoded in
ExtraICStateField? That's only 20 bits isn't it?
https://codereview.chromium.org/172523002/
--
--
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/d/optout.
