Looking good. Bunch of minor comments.
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));
&& !edi.is(slot) && !edi.is(vector)
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
||
Can you re-use UseVector() here instead of duplicating the condition?
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode599
src/ic/ic.cc:599: // Determine our state.
nit: I don't think this comment adds much value.
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode601
src/ic/ic.cc:601: State state = nexus->StateFromFeedback();
Save the previous state before calling ConfigurePremonomorphic maybe?
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);
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).
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode1359
src/ic/ic.cc:1359: return target();
Consider "return Handle<Code>::null();" to make it obvious that the
return value is not supposed to be used for patching?
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode2320
src/ic/ic.cc:2320: DCHECK(FLAG_vector_ics);
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.)
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode2347
src/ic/ic.cc:2347: DCHECK(FLAG_vector_ics);
again
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode2374
src/ic/ic.cc:2374: DCHECK(FLAG_vector_ics);
again
https://codereview.chromium.org/754303003/diff/20001/src/ic/ic.cc#newcode2967
src/ic/ic.cc:2967: DCHECK(FLAG_vector_ics);
again
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.
nit: s/They are all CallICs.//
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode180
src/type-feedback-vector.cc:180: // Discover the correct Code::Kind.
nit: this comment doesn't add value
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode184
src/type-feedback-vector.cc:184: // bool is_invalidated =
nexus.IsInvalidatedWeakIC();
leftover?
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode236
src/type-feedback-vector.cc:236: state = UNINITIALIZED;
consider to directly "return UNINITIALIZED;" (and similar below).
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode247
src/type-feedback-vector.cc:247: } else if (length > 2) {
s/if (length > 2)//, it's guaranteed anyway.
https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector.cc#newcode275
src/type-feedback-vector.cc:275: } else if (length > 3) {
s/if (length > 3)//, it's guaranteed anyway.
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);
Looks like return raw pointers in the various vector()->FooSentinel()
functions might make more sense. Feel free to leave that for later,
though.
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?
I think this comment is now subsumed by the descriptive method name.
https://codereview.chromium.org/754303003/diff/20001/src/type-info.cc#newcode333
src/type-info.cc:333: // Are all the receiver maps string maps?
again
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);
uhm... s/MONOMORPHIC/desired_state/ maybe?
https://codereview.chromium.org/754303003/diff/20001/test/cctest/test-heap.cc#newcode3456
test/cctest/test-heap.cc:3456: CheckVectorIC(f, 0, POLYMORPHIC);
I guess this is a bug here then, since POLYMORPHIC is ignored by
CheckVectorIC().
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.
