Make KeyedStores from a sloppy arguments array use a handler.

Make KeyedStores from a sloppy arguments array use a handler.

This finishes up work begun by CL (https://codereview.chromium.org/546683003/) some months ago.

BUG=

Committed: https://crrev.com/3a1d7335cf6a623f39c306e0bc0e82ccccc54818
Cr-Commit-Position: refs/heads/master@{#28683}

[mvstanton, 2015-05-28 09:53:07]

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

[mvstanton, 2015-05-28 09:53:51]

Hi Jakob,
You'll like this one!
To aid vectorization of store ics, I need to extend the work begun earlier for
loads. Thanks for the look,
--Michael

[Jakob Kummerow, 2015-05-28 11:55:01]

Yup, I like the platform-specific changes in particular. Shall we ask the MIPS
guys to review their ports? ;-)

Two questions though.

https://codereview.chromium.org/1149053004/diff/20001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/1149053004/diff/20001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:902: if (is_load) environment()->Push(result);
Doesn't the store stub need to return the value to allow chained assignments?
The handwritten stub appears to have done this implicitly by virtue of passing
in the value in eax and never overwriting it. I think you want
"environment()->Push(is_load ? result : value);" here.

https://codereview.chromium.org/1149053004/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1149053004/diff/20001/src/ic/ic.cc#newcode2052
src/ic/ic.cc:2052: TRACE_GENERIC_IC(isolate(), "KeyedStoreIC", "arguments
receiver");
I find this code confusing. TRACE_GENERIC should be right where we make the
decision that we don't have a stub for the given situation. Since you're now
falling through to StoreElementStub() below, this decision and the tracing call
should move there.
Am I missing something?

[mvstanton, 2015-05-28 13:23:52]

Hi Jakob, thanks for the comments.
I punted on a new test for the bug you identified in ic.cc. I did find
mjsunit/regress-keyed-store-non-strict-arguments.js which when run with
--trace-ic allows you to see the stub going megamorphic as expected. At least
now that happens correctly instead of remaining monomorphic.

Thanks again,
--Michael

https://codereview.chromium.org/1149053004/diff/20001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/1149053004/diff/20001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:902: if (is_load) environment()->Push(result);
On 2015/05/28 11:55:00, Jakob wrote:
> Doesn't the store stub need to return the value to allow chained assignments?
> The handwritten stub appears to have done this implicitly by virtue of passing
> in the value in eax and never overwriting it. I think you want
> "environment()->Push(is_load ? result : value);" here.

Good idea, it's much cleaner that way.

https://codereview.chromium.org/1149053004/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1149053004/diff/20001/src/ic/ic.cc#newcode2052
src/ic/ic.cc:2052: TRACE_GENERIC_IC(isolate(), "KeyedStoreIC", "arguments
receiver");
On 2015/05/28 11:55:00, Jakob wrote:
> I find this code confusing. TRACE_GENERIC should be right where we make the
> decision that we don't have a stub for the given situation. Since you're now
> falling through to StoreElementStub() below, this decision and the tracing
call
> should move there.
> Am I missing something?

You certainly found a bug! I need an else before the key_is_smi_like test. I'll
keep the decision to go generic if the language mode is inappropriate for the
sloppy arguments elements map here in the spirit of minimum permutation for now.
But I like your idea of moving all these generic exceptions into the
StoreElementStub for later.

[Jakob Kummerow, 2015-05-28 13:41:53]

lgtm

[mvstanton, 2015-05-28 14:12:07]

The CQ bit was checked by mvstanton@chromium.org

[commit-bot: I haz the power, 2015-05-28 14:12:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149053004/40001

[commit-bot: I haz the power, 2015-05-28 14:56:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-28 14:56:25]

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