Hi Jakob, thanks for the comments.
I've responded, and introduced the Ast Node traversal fixing of the
Code::Kind for the ICSlots that we talked about offline.
The good thing about that is that:
* the structure of the vector can be said to be fully fixed when it's
allocated, with no leftover work to be done. Thusly I could make
TypeFeedbackVector::SetKind(slot, kind) a private method.
* No need to finish this work in full code and various multiplying
locations when/if full code isn't expected to compile even once.
This SetKind()/GetKind() stuff is only done when FLAG_vector_ics
is true, so the tip of tree won't experience the extra storage space
for keeping track of that information.
Also, I could reduce storage with a bitset approach, but for now I'm
happy to have the data centralized in FeedbackVectorSpec, where it can
be optimized if need be.
--Michael
https://codereview.chromium.org/754303003/diff/20001/src/ic/ia32/ic-ia32.cc
File src/ic/ia32/ic-ia32.cc (right):
https://codereview.chromium.org/754303003/diff/20001/src/ic/ia32/ic-ia32.cc#newcode773
src/ic/ia32/ic-ia32.cc:773: DCHECK(!edi.is(receiver) && !edi.is(name));
On 2014/11/26 15:14:14, Jakob wrote:
&& !edi.is(slot) && !edi.is(vector)
Done, also in x64 which needed this temporary register as well.
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode461
src/ic/ic.cc:461: (FLAG_vector_ics && (target->kind() == Code::LOAD_IC
||
On 2014/11/26 15:14:14, Jakob wrote:
Can you re-use UseVector() here instead of duplicating the condition?
Done.
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode599
src/ic/ic.cc:599: // Determine our state.
On 2014/11/26 15:14:15, Jakob wrote:
nit: I don't think this comment adds much value.
Done.
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode601
src/ic/ic.cc:601: State state = nexus->StateFromFeedback();
On 2014/11/26 15:14:14, Jakob wrote:
Save the previous state before calling ConfigurePremonomorphic maybe?
doh! good catch, thanks!
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode854
src/ic/ic.cc:854: DCHECK(kind() == Code::LOAD_IC || kind() ==
Code::KEYED_LOAD_IC);
On 2014/11/26 15:14:15, Jakob wrote:
You can drop this DCHECK; ConfigureVectorState() contains the same
check (and is
what will need to be updated in the future anyway; this call site
doesn't need
to care).
Done.
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode1359
src/ic/ic.cc:1359: return target();
On 2014/11/26 15:14:15, Jakob wrote:
Consider "return Handle<Code>::null();" to make it obvious that the
return value
is not supposed to be used for patching?
I think this use of target() solved a problem for me ("elegantly") at
one point but now it's just strange. I'll go to Handle<Code>::null().
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode2320
src/ic/ic.cc:2320: DCHECK(FLAG_vector_ics);
On 2014/11/26 15:14:14, Jakob wrote:
I think I'd swap this around:
if (FLAG_vector_ics) {
DCHECK(args.length() == 4);
for consistency with (1) places where we DCHECK the args.length, and
(2) places
where we decide how to behave based on the flag. (Also makes it more
mechanical
to remove the flag some day.)
Cool. This is a relic of a time in the work when both types existed at
once. Thk God those days are done...fixing :).
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode2347
src/ic/ic.cc:2347: DCHECK(FLAG_vector_ics);
On 2014/11/26 15:14:15, Jakob wrote:
again
Done.
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode2374
src/ic/ic.cc:2374: DCHECK(FLAG_vector_ics);
On 2014/11/26 15:14:14, Jakob wrote:
again
Done.
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode2967
src/ic/ic.cc:2967: DCHECK(FLAG_vector_ics);
On 2014/11/26 15:14:14, Jakob wrote:
again
Done.
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc
File src/type-feedback-vector.cc (right):
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode170
src/type-feedback-vector.cc:170: // Now clear vector-based ICs. They are
all CallICs.
On 2014/11/26 15:14:15, Jakob wrote:
nit: s/They are all CallICs.//
Done.
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode180
src/type-feedback-vector.cc:180: // Discover the correct Code::Kind.
On 2014/11/26 15:14:15, Jakob wrote:
nit: this comment doesn't add value
Done.
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode184
src/type-feedback-vector.cc:184: // bool is_invalidated =
nexus.IsInvalidatedWeakIC();
On 2014/11/26 15:14:15, Jakob wrote:
leftover?
Yes, I think so, removed.
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode236
src/type-feedback-vector.cc:236: state = UNINITIALIZED;
On 2014/11/26 15:14:15, Jakob wrote:
consider to directly "return UNINITIALIZED;" (and similar below).
Done.
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode247
src/type-feedback-vector.cc:247: } else if (length > 2) {
On 2014/11/26 15:14:15, Jakob wrote:
s/if (length > 2)//, it's guaranteed anyway.
Done.
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode275
src/type-feedback-vector.cc:275: } else if (length > 3) {
On 2014/11/26 15:14:15, Jakob wrote:
s/if (length > 3)//, it's guaranteed anyway.
Done.
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode331
src/type-feedback-vector.cc:331:
SetFeedback(*vector()->GenericSentinel(GetIsolate()),
SKIP_WRITE_BARRIER);
On 2014/11/26 15:14:15, Jakob wrote:
Looks like return raw pointers in the various vector()->FooSentinel()
functions
might make more sense. Feel free to leave that for later, though.
Yes, I'll come back for that later. I think I want a handle version and
a raw version just like that exposed by factory-> and heap-> for these
symbols. The handle version is used more during compilation and the raw
version during slot maintenance.
https://codereview.chromium.org/754303003/diff/20001/src/type-info.cc
File src/type-info.cc (right):
https://codereview.chromium.org/754303003/diff/20001/src/type-info.cc#newcode312
src/type-info.cc:312: // Are all the receiver maps string maps?
On 2014/11/26 15:14:15, Jakob wrote:
I think this comment is now subsumed by the descriptive method name.
Done.
https://codereview.chromium.org/754303003/diff/20001/src/type-info.cc#newcode333
src/type-info.cc:333: // Are all the receiver maps string maps?
On 2014/11/26 15:14:15, Jakob wrote:
again
Done.
https://codereview.chromium.org/754303003/diff/20001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):
https://codereview.chromium.org/754303003/diff/20001/test/cctest/test-heap.cc#newcode3334
test/cctest/test-heap.cc:3334: CHECK(nexus.StateFromFeedback() ==
MONOMORPHIC);
On 2014/11/26 15:14:15, Jakob wrote:
uhm... s/MONOMORPHIC/desired_state/ maybe?
Yes. Oh brother.
https://codereview.chromium.org/754303003/diff/20001/test/cctest/test-heap.cc#newcode3456
test/cctest/test-heap.cc:3456: CheckVectorIC(f, 0, POLYMORPHIC);
On 2014/11/26 15:14:15, Jakob wrote:
I guess this is a bug here then, since POLYMORPHIC is ignored by
CheckVectorIC().
Okay, CheckVectorIC() fixed. I did some hasty refactoring to the CL to
introduce that function and apparently didn't test it :p.
https://codereview.chromium.org/754303003/
--
--
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.
