Vector ICs: platform support for vector-based stores.

Vector ICs: platform support for vector-based stores.

The last changes for vector store functionality, they are in 3 areas:

1) The new vector [keyed] store code stubs - implementation.
2) IC and handler compiler adjustments
3) Odds and ends. A change in ast.cc, a test update, a small Oracle fix.

BUG=

Committed: https://crrev.com/63af1b3aec6547e7cdf502666ff79c562de8b679
Cr-Commit-Position: refs/heads/master@{#30570}

[mvstanton, 2015-09-02 09:56:03]

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

[mvstanton, 2015-09-02 09:56:03]

Hi guys,
This CL makes vector store support complete in the tree.

Benedikt: could you look at code-stubs-<platform>.cc,
Jakob, everything else.

I'll ask for mips porting help in a moment...
Thanks!
--Michael

[mvstanton, 2015-09-02 10:01:02]

Mips team, could you guys address mips/mips64 port? Let me know any questions,
thanks!

[Benedikt Meurer, 2015-09-02 19:25:35]

bmeurer@chromium.org changed required reviewers:
+ jkummerow@chromium.org

[Benedikt Meurer, 2015-09-02 19:25:35]

LGTM from me on code-stubs (although the ia32 dance looks hairy; I bet we need
to do something about that performance-wise...)

[paul.l..., 2015-09-03 05:21:46]

Hi Michael, the MIPS ports are ready in
https://codereview.chromium.org/1318353003/

[Jakob Kummerow, 2015-09-03 11:27:40]

Looks good. Some comments.

https://codereview.chromium.org/1328603003/diff/40001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/1328603003/diff/40001/src/ast.cc#newcode353
src/ast.cc:353: if (saw_computed_name &&
This is surprising: having seen a computed name previously affects how the
current property is handled? Are you sure this is correct? (I bet we lack test
coverage for this; NeedsHomeObject() is fairly arcane.)

https://codereview.chromium.org/1328603003/diff/40001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1328603003/diff/40001/src/flag-definitions.h#...
src/flag-definitions.h:704: DEFINE_BOOL(vector_stores, true, "use vectors for
store ics")
Don't forget to turn this off again :-)

https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedba...
File test/cctest/test-feedback-vector.cc (right):

https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:419: CHECK_EQ(FLAG_vector_stores ? 4 : 2,
feedback_vector->ICSlots());
suggestion:

if (FLAG_vector_stores) {
  CHECK_EQ(4, feedback_vector->ICSlots());
  CHECK(feedback_vector->GetKind(FeedbackVectorICSlot(0)) == Code::LOAD_IC);
  CHECK(                                              1            STORE_IC);
  CHECK(                                              2            LOAD_IC);
  CHECK(                                              3            STORE_IC);
} else {
  CHECK_EQ(2, feedback_vector->ICSlots());
  CHECK(feedback_vector->GetKind(FeedbackVectorICSlot(0)) == Code::LOAD_IC);
  CHECK(feedback_vector->GetKind(FeedbackVectorICSlot(1)) == Code::LOAD_IC);
}

Provides more test coverage now, and is trivial to simplify when
FLAG_vector_stores is removed.

https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:453: FeedbackVectorICSlot(FLAG_vector_stores
? 3 : 2)) == Code::CALL_IC);
suggestion:

int slot = 0;
CHECK(...(FeedbackVectorICSlot(slot++)) == Code::CALL_IC);
CHECK(...(FeedbackVectorICSlot(slot++)) == Code::LOAD_IC);
if (FLAG_vector_stores) {
  CHECK(...(FeedbackVectorICSlot(slot++)) == Code::STORE_IC);
}
...

https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:503: // Function f has 3 LoadICs, one for
each o, but the ICs share the same
lolwut?

[mvstanton, 2015-09-03 15:25:27]

Thanks all!

https://codereview.chromium.org/1328603003/diff/40001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/1328603003/diff/40001/src/ast.cc#newcode353
src/ast.cc:353: if (saw_computed_name &&
On 2015/09/03 11:27:39, Jakob wrote:
> This is surprising: having seen a computed name previously affects how the
> current property is handled? Are you sure this is correct? (I bet we lack test
> coverage for this; NeedsHomeObject() is fairly arcane.)

You are totally right. It is true though. As we discussed offline, I will be
refactoring this so that each ObjectLiteral::Property knows how many slots it
needs, and the code will be less fragile (also, it'll be tractable from the
point of view of the oracle).

https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedba...
File test/cctest/test-feedback-vector.cc (right):

https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:419: CHECK_EQ(FLAG_vector_stores ? 4 : 2,
feedback_vector->ICSlots());
On 2015/09/03 11:27:40, Jakob wrote:
> suggestion:
> 
> if (FLAG_vector_stores) {
>   CHECK_EQ(4, feedback_vector->ICSlots());
>   CHECK(feedback_vector->GetKind(FeedbackVectorICSlot(0)) == Code::LOAD_IC);
>   CHECK(                                              1            STORE_IC);
>   CHECK(                                              2            LOAD_IC);
>   CHECK(                                              3            STORE_IC);
> } else {
>   CHECK_EQ(2, feedback_vector->ICSlots());
>   CHECK(feedback_vector->GetKind(FeedbackVectorICSlot(0)) == Code::LOAD_IC);
>   CHECK(feedback_vector->GetKind(FeedbackVectorICSlot(1)) == Code::LOAD_IC);
> }
> 
> Provides more test coverage now, and is trivial to simplify when
> FLAG_vector_stores is removed.

I like it, done.

https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:453: FeedbackVectorICSlot(FLAG_vector_stores
? 3 : 2)) == Code::CALL_IC);
On 2015/09/03 11:27:40, Jakob wrote:
> suggestion:
> 
> int slot = 0;
> CHECK(...(FeedbackVectorICSlot(slot++)) == Code::CALL_IC);
> CHECK(...(FeedbackVectorICSlot(slot++)) == Code::LOAD_IC);
> if (FLAG_vector_stores) {
>   CHECK(...(FeedbackVectorICSlot(slot++)) == Code::STORE_IC);
> }
> ...

Allow me to follow up on this suggestion in a follow-up that reduces the
repetitiveness of these lines. We have a FeedbackVectorSpec class, and we can
ask if the spec differs from the vector. This way I'll be able to write an array
like { Code::LOAD_IC, Code::STORE_IC, ... } and compare.

https://codereview.chromium.org/1328603003/diff/40001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:503: // Function f has 3 LoadICs, one for
each o, but the ICs share the same
On 2015/09/03 11:27:40, Jakob wrote:
> lolwut?

Cosmically weird comment removed :).

[mvstanton, 2015-09-03 15:25:39]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-09-03 15:25:40]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1328603003/#ps100001
(title: "Fix rebase issue.")

[commit-bot: I haz the power, 2015-09-03 15:25:52]

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

[commit-bot: I haz the power, 2015-09-03 15:25:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-03 15:25:55]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Jakob Kummerow, 2015-09-03 15:46:06]

lgtm

[mvstanton, 2015-09-03 16:52:09]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-09-03 16:52:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1328603003/#ps120001
(title: "mips build fixes.")

[commit-bot: I haz the power, 2015-09-03 16:52:21]

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

[commit-bot: I haz the power, 2015-09-03 17:18:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-09-03 17:18:33]

Patchset 7 (id:??) landed as
https://crrev.com/63af1b3aec6547e7cdf502666ff79c562de8b679
Cr-Commit-Position: refs/heads/master@{#30570}

[Michael Achenbach, 2015-09-03 17:43:48]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1303053004/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Breaks compile on arm:
http://build.chromium.org/p/client.v8/builders/V8%20Arm%20-%20builder/builds/....