Distinction between FeedbackVectorICSlot and FeedbackVectorSlot eliminated.

Distinction between FeedbackVectorICSlot and FeedbackVectorSlot eliminated.

This CL also allows to use arbitrary number of feedback vector elements for particular slot kind.

Committed: https://crrev.com/90998947bc8c6d7ecee45dfede2a063746e20381
Cr-Commit-Position: refs/heads/master@{#31050}

[Igor Sheludko, 2015-09-30 14:21:43]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2015-09-30 14:21:48]

Patchset #1 (id:20001) has been deleted

[Igor Sheludko, 2015-09-30 14:21:53]

Patchset #1 (id:40001) has been deleted

[Igor Sheludko, 2015-09-30 14:21:58]

Patchset #1 (id:60001) has been deleted

[Igor Sheludko, 2015-09-30 14:50:26]

Patchset #1 (id:80001) has been deleted

[Igor Sheludko, 2015-09-30 16:02:57]

Patchset #1 (id:100001) has been deleted

[Igor Sheludko, 2015-09-30 16:03:03]

Patchset #1 (id:120001) has been deleted

[Igor Sheludko, 2015-09-30 16:03:07]

Patchset #1 (id:140001) has been deleted

[Igor Sheludko, 2015-09-30 16:30:19]

Patchset #1 (id:160001) has been deleted

[Igor Sheludko, 2015-09-30 16:30:55]

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

[Igor Sheludko, 2015-09-30 16:30:56]

PTAL

https://codereview.chromium.org/1370303004/diff/170001/src/heap/heap.cc
File src/heap/heap.cc (left):

https://codereview.chromium.org/1370303004/diff/170001/src/heap/heap.cc#oldco...
src/heap/heap.cc:2731: dummy_vector->Set(FeedbackVectorICSlot(0),
I guess we wanted to use FeedbackVectorICSlot(i) here...

[Igor Sheludko, 2015-10-01 07:39:14]

ishell@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Igor Sheludko, 2015-10-01 07:39:15]

Michi, PTAL at heap and TurboFan parts.

[Michael Starzinger, 2015-10-01 08:36:43]

LGTM for heap, factory and compiler. Didn't look at the rest.

[mvstanton, 2015-10-01 10:24:31]

Thanks Igor this is great. A couple of nits and one main issue...

https://codereview.chromium.org/1370303004/diff/170001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1370303004/diff/170001/src/ast.h#newcode2388
src/ast.h:2388: FeedbackVectorSlot yield_first_feedback_slot_;
It's okay to make the size of Yield bigger, but if this was a node like Property
I wouldn't want to increase the size. Is it still possible to do the kind of
trick we did here with Yield? That is, subsequent calls to AddLoadICSlot() will
give you the next slot (modulo a number of words determined by the slot kind)?

https://codereview.chromium.org/1370303004/diff/170001/src/heap/heap.cc
File src/heap/heap.cc (left):

https://codereview.chromium.org/1370303004/diff/170001/src/heap/heap.cc#oldco...
src/heap/heap.cc:2731: dummy_vector->Set(FeedbackVectorICSlot(0),
On 2015/09/30 16:30:56, Igor Sheludko wrote:
> I guess we wanted to use FeedbackVectorICSlot(i) here...

Good catch.

https://codereview.chromium.org/1370303004/diff/170001/src/type-feedback-vect...
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/1370303004/diff/170001/src/type-feedback-vect...
src/type-feedback-vector.cc:112: StaticFeedbackVectorSpec spec(arraysize(kinds),
kinds);
This confused me a minute, but then I see what you are doing - you are laying
out the actual words instead of speaking in terms of slots.

It seems like a mismatch - also over in heap.cc you have to do that.

Is it possible, to take the kinds[] input as slots instead of a specification
for the detailed layout of those slots?

It leaks information about the internal layout and exact size of each kind of ic
slot. It'd be better to hide the fact that for a KEYED_LOAD_IC slot, it's second
(and third, etc.) words will be coded as INVALID.

As an example, you'd have

FVSK kinds[] = {FVSK::KEYED_LOAD_IC, FVSK::GENERIC,
                FVSK::KEYED_LOAD_IC};
...

https://codereview.chromium.org/1370303004/diff/170001/src/type-feedback-vect...
src/type-feedback-vector.cc:163: TypeFeedbackMetadataIterator iter(this);
This iterator is pretty cool!

https://codereview.chromium.org/1370303004/diff/170001/src/type-feedback-vect...
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/1370303004/diff/170001/src/type-feedback-vect...
src/type-feedback-vector.h:33: // The slot corresponds to "some" stub that
occupies one feedback vector
nit: Maybe here the comment could be more that it's a general purpose slot. It
might not even be a stub, it might just be full-code that uses it. Instead of
STUB, maybe GENERAL, GENERIC, NORMAL or something like that is a good name...

https://codereview.chromium.org/1370303004/diff/170001/test/cctest/test-feedb...
File test/cctest/test-feedback-vector.h (right):

https://codereview.chromium.org/1370303004/diff/170001/test/cctest/test-feedb...
test/cctest/test-feedback-vector.h:16: class FeedbackVectorHelper {
I wonder if we might want this in /src/type-feedback-vector.h...

[Igor Sheludko, 2015-10-01 12:44:06]

Patchset #2 (id:190001) has been deleted

[Igor Sheludko, 2015-10-01 12:46:30]

Thanks for the comments!

https://codereview.chromium.org/1370303004/diff/170001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1370303004/diff/170001/src/ast.h#newcode2388
src/ast.h:2388: FeedbackVectorSlot yield_first_feedback_slot_;
On 2015/10/01 10:24:31, mvstanton wrote:
> It's okay to make the size of Yield bigger, but if this was a node like
Property
> I wouldn't want to increase the size. Is it still possible to do the kind of
> trick we did here with Yield? That is, subsequent calls to AddLoadICSlot()
will
> give you the next slot (modulo a number of words determined by the slot kind)?

Yes, it's possible but let's leave it simple for now.

https://codereview.chromium.org/1370303004/diff/170001/src/type-feedback-vect...
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/1370303004/diff/170001/src/type-feedback-vect...
src/type-feedback-vector.cc:112: StaticFeedbackVectorSpec spec(arraysize(kinds),
kinds);
On 2015/10/01 10:24:31, mvstanton wrote:
> This confused me a minute, but then I see what you are doing - you are laying
> out the actual words instead of speaking in terms of slots.
> 
> It seems like a mismatch - also over in heap.cc you have to do that.
> 
> Is it possible, to take the kinds[] input as slots instead of a specification
> for the detailed layout of those slots?
> 
> It leaks information about the internal layout and exact size of each kind of
ic
> slot. It'd be better to hide the fact that for a KEYED_LOAD_IC slot, it's
second
> (and third, etc.) words will be coded as INVALID.
> 
> As an example, you'd have
> 
> FVSK kinds[] = {FVSK::KEYED_LOAD_IC, FVSK::GENERIC,
>                 FVSK::KEYED_LOAD_IC};
> ...

I remade StaticFeedbackVectorSpec to be less confusing.

https://codereview.chromium.org/1370303004/diff/170001/src/type-feedback-vect...
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/1370303004/diff/170001/src/type-feedback-vect...
src/type-feedback-vector.h:33: // The slot corresponds to "some" stub that
occupies one feedback vector
On 2015/10/01 10:24:31, mvstanton wrote:
> nit: Maybe here the comment could be more that it's a general purpose slot. It
> might not even be a stub, it might just be full-code that uses it. Instead of
> STUB, maybe GENERAL, GENERIC, NORMAL or something like that is a good name...

Done.

https://codereview.chromium.org/1370303004/diff/170001/test/cctest/test-feedb...
File test/cctest/test-feedback-vector.h (right):

https://codereview.chromium.org/1370303004/diff/170001/test/cctest/test-feedb...
test/cctest/test-feedback-vector.h:16: class FeedbackVectorHelper {
On 2015/10/01 10:24:31, mvstanton wrote:
> I wonder if we might want this in /src/type-feedback-vector.h...

I think the production code must always use slots returned by Spec::AddSlot()
methods.

[mvstanton, 2015-10-01 13:35:52]

Really great, LGTM!

[Igor Sheludko, 2015-10-01 13:45:20]

The CQ bit was checked by ishell@chromium.org

[Igor Sheludko, 2015-10-01 13:45:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1370303004/#ps230001
(title: "Fixed release builds")

[commit-bot: I haz the power, 2015-10-01 13:45:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1370303004/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1370303004/230001

[commit-bot: I haz the power, 2015-10-01 13:48:11]

Committed patchset #3 (id:230001)

[commit-bot: I haz the power, 2015-10-01 13:48:30]

Patchset 3 (id:??) landed as
https://crrev.com/90998947bc8c6d7ecee45dfede2a063746e20381
Cr-Commit-Position: refs/heads/master@{#31050}

[Michael Achenbach, 2015-10-01 14:34:57]

Breaks vector stores. I don't revert. Fix later is ok.