Flesh out vector ic state query and set mechanisms.

Flesh out vector ic state query and set mechanisms.

The IC system now fully integrates the vector concept and can
handle loads and keyed loads vector-based.

BUG=
R=jkummerow@chromium.org

Committed: https://chromium.googlesource.com/v8/v8/+/c142994f742b52cd59aa15d0961d4b0034bcbf88

[mvstanton, 2014-11-25 11:43:52]

mvstanton@chromium.org changed reviewers:
+ jkummerow@chromium.org

[mvstanton, 2014-11-25 11:43:53]

Hi Jakob,
This CL contains:

* All the changes to IC class to handle vector load and keyed load.
* Remaining infrastructure in type-feedback-vector.h/.cc to support query and
update of a state nexus.
* Complete updates to the Oracle to support gathering vector feedback.

What is missing is:

* The hydrogen code stubs that dispatch for load and keyed load, along with some
hydrogen instruction and lithium changes for those, and
* platform-dependent changes to the handler compiler to deal with the
vector/slot register combo.

[Jakob Kummerow, 2014-11-26 15:14:15]

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#n...
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...
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...
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...
src/type-feedback-vector.cc:184: // bool is_invalidated =
nexus.IsInvalidatedWeakIC();
leftover?

https://codereview.chromium.org/754303003/diff/20001/src/type-feedback-vector...
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...
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...
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...
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...
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...
test/cctest/test-heap.cc:3456: CheckVectorIC(f, 0, POLYMORPHIC);
I guess this is a bug here then, since POLYMORPHIC is ignored by
CheckVectorIC().

[mvstanton, 2014-11-27 13:04:02]

Patchset #4 (id:60001) has been deleted

[mvstanton, 2014-11-27 13:09:27]

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#n...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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.

[Jakob Kummerow, 2014-11-27 14:15:58]

LGTM

https://codereview.chromium.org/754303003/diff/80001/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/754303003/diff/80001/src/ast-numbering.cc#new...
src/ast-numbering.cc:79: if (FLAG_vector_ics) {
Do we even need this flag check? Wouldn't !FLAG_vector_ics imply
reqs.ic_slots==0, so we wouldn't get here?

https://codereview.chromium.org/754303003/diff/80001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/754303003/diff/80001/src/factory.h#newcode13
src/factory.h:13: // Interface for handle based allocation.
nit: move this comment down two lines, it describes "class Factory".

[mvstanton, 2014-11-27 15:06:12]

Thanks so much, --Michael

https://codereview.chromium.org/754303003/diff/80001/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/754303003/diff/80001/src/ast-numbering.cc#new...
src/ast-numbering.cc:79: if (FLAG_vector_ics) {
On 2014/11/27 14:15:58, Jakob wrote:
> Do we even need this flag check? Wouldn't !FLAG_vector_ics imply
> reqs.ic_slots==0, so we wouldn't get here?

Good point but no, because in the live tree we do have CALL_IC which is
vector-based. Since the live tree only has that one kind of IC, the
TypeFeedbackVector omits storing the little index that describes Kind when
FLAG_vector_ics is false.

https://codereview.chromium.org/754303003/diff/80001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/754303003/diff/80001/src/factory.h#newcode13
src/factory.h:13: // Interface for handle based allocation.
On 2014/11/27 14:15:58, Jakob wrote:
> nit: move this comment down two lines, it describes "class Factory".

Done.

[mvstanton, 2014-11-27 16:36:46]

Committed patchset #5 (id:100001) manually as
c142994f742b52cd59aa15d0961d4b0034bcbf88 (presubmit successful).