Hi Toon,
Thanks for the comments, and sorry for the delay in addressing! PTAL,
thanks,
--Michael
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(); }
On 2014/03/10 13:50:44, Toon Verwaest wrote:
is_strict_or_native?
The move to enums addressed this satisfactorily I think.
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);
On 2014/03/10 13:50:44, Toon Verwaest wrote:
Seems like this function is dead. If so, can you remove it?
Done.
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,
On 2014/03/10 13:50:44, Toon Verwaest wrote:
one arg per line
Done.
https://codereview.chromium.org/172523002/diff/160001/src/ia32/code-stubs-ia32.cc#newcode2528
src/ia32/code-stubs-ia32.cc:2528: __ cmp(ecx, edi);
On 2014/03/10 13:50:44, Toon Verwaest wrote:
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.
Excellent idea, it reads cleaner too.
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.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
we went -> If we get here, go from monomorphic to megamorphic. Don't
..
Done.
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
On 2014/03/10 13:50:44, Toon Verwaest wrote:
Verify whether the input function matches the recorded monomorphic
target?
After rearranging the function, I kept your ideal phrase "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);
On 2014/03/10 13:50:44, Toon Verwaest wrote:
Same here as above. This path should be inlined here.
Done.
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.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
What about something like:
// Don't assign a type feedback id to the IC, since type feedback is
provided by
the vector above.
Done.
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,
On 2014/03/10 13:50:44, Toon Verwaest wrote:
Doesn't this fit on one line? Otherwise
... CallIC::initialize_stub(
isolate(), arg_count, true);
Done.
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) {
On 2014/03/10 13:50:44, Toon Verwaest wrote:
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.
Cool, this refactored very nicely.
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) {
On 2014/03/10 13:50:44, Toon Verwaest wrote:
Isn't this only relevant for KEYED_STORE_IC?
Done.
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1800
src/ic.cc:1800: if (feedback->IsJSFunction()) {
On 2014/03/10 13:50:44, Toon Verwaest wrote:
Shouldn't this be feedback->IsJSFunction() && function->IsJSFunction()
?
I went ahead and just returned without changing anything now if
!function->IsJSFunction().
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1802
src/ic.cc:1802: // otherwise the slow path will be taken.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
Weird sentence
Removed.
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
On 2014/03/10 13:50:44, Toon Verwaest wrote:
We only need to patch if ...
Done.
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.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
Remove ugh.
Done.
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1836
src/ic.cc:1836: // Ideally we can just patch the cell and return.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
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).
I went to objects-visiting-inl.h and make sure that monomorphic call ics
are cleared in sync with the associated feedback cells. This is the
simplest thing for now. So if our type cell is uninitialized we won't
have a specialized MONOMORPHIC CallIC in place.
If this turns out to be a performance issue I can address it with your
suggestion, which sounds good.
https://codereview.chromium.org/172523002/diff/160001/src/ic.cc#newcode1846
src/ic.cc:1846: Handle<String> name;
On 2014/03/10 13:50:44, Toon Verwaest wrote:
Don't you want to initialize this to something like the empty_string?
Yes, I could simplify that even more.
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#newcode330
src/ic.h:330: STATIC_ASSERT(Code::kArgumentsBits == 16);
On 2014/03/10 13:50:44, Toon Verwaest wrote:
What about just using kArgumentsBits in the counts below so we don't
need to
change it by hand later?
Done.
https://codereview.chromium.org/172523002/diff/160001/src/ic.h#newcode336
src/ic.h:336: // We have a few extra bits.
On 2014/03/10 13:50:44, Toon Verwaest wrote:
Not sure how many you have left. Aren't these encoded in
ExtraICStateField?
That's only 20 bits isn't it?
Old comment, removed.
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.
