VectorICs: keyed element loads were kicking out non-smi keys unnecessarily

VectorICs: keyed element loads were kicking out non-smi keys unnecessarily

Handlers should be in charge of this work. The change uncovered a bug in
vector-ics related to keyed loads into strings. It's important for
StringCharCodeAtGenerator, a helper used in full code and in
LoadIndexedStringStub (a handler) to protect the vector and slot registers
when it makes a runtime call to convert a HeapNumber to a Smi.

It's still possible for the handler to MISS after this call, perhaps due
to out of bounds access. In that case, the vector and slot registers need
to be delivered safely to the MISS handler.

BUG=

Committed: https://crrev.com/6689cc27ebe60685c025de9ae1f09919093f8213
Cr-Commit-Position: refs/heads/master@{#27377}

[mvstanton, 2015-03-23 10:17:17]

Patchset #1 (id:1) has been deleted

[mvstanton, 2015-03-23 10:20:55]

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

[mvstanton, 2015-03-23 10:22:24]

Hi Yang, here is the CL we just discussed. Thanks for the look,
--Michael

[Yang, 2015-03-23 14:23:59]

LGTM with comments.

https://codereview.chromium.org/1028093002/diff/40001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1028093002/diff/40001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:2919: __
push(VectorLoadICDescriptor::VectorRegister());
If you use Push to push both registers at once, you may end up with a shorter
instruction if you use the correct order.

https://codereview.chromium.org/1028093002/diff/40001/src/arm64/code-stubs-ar...
File src/arm64/code-stubs-arm64.cc (right):

https://codereview.chromium.org/1028093002/diff/40001/src/arm64/code-stubs-ar...
src/arm64/code-stubs-arm64.cc:3331: __
Pop(VectorLoadICDescriptor::SlotRegister());
Can we use the equivalent Pop with two registers?

https://codereview.chromium.org/1028093002/diff/40001/src/mips/code-stubs-mip...
File src/mips/code-stubs-mips.cc (right):

https://codereview.chromium.org/1028093002/diff/40001/src/mips/code-stubs-mip...
src/mips/code-stubs-mips.cc:3087: __
pop(VectorLoadICDescriptor::VectorRegister());
Same here, corresponding Pop?

https://codereview.chromium.org/1028093002/diff/40001/test/mjsunit/string-ind...
File test/mjsunit/string-index.js (right):

https://codereview.chromium.org/1028093002/diff/40001/test/mjsunit/string-ind...
test/mjsunit/string-index.js:255: var keys = [0, Math.abs(2)];
You probably should check that this is not a smi via %IsSmi

[mvstanton, 2015-03-23 15:54:45]

Thanks for the comments, Yang. I've uploaded a patch. Note an unrelated change
in mips, the weakening of an assert that didn't make sense there anyway...it's
simply that no one ever ran that code :p.
Submitting,
--Michael

https://codereview.chromium.org/1028093002/diff/40001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/1028093002/diff/40001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:2919: __
push(VectorLoadICDescriptor::VectorRegister());
On 2015/03/23 14:23:59, Yang wrote:
> If you use Push to push both registers at once, you may end up with a shorter
> instruction if you use the correct order.

Good idea, and I went further and do one push instruction for all four registers
(and one pop for the 3 after the call).

https://codereview.chromium.org/1028093002/diff/40001/src/arm64/code-stubs-ar...
File src/arm64/code-stubs-arm64.cc (right):

https://codereview.chromium.org/1028093002/diff/40001/src/arm64/code-stubs-ar...
src/arm64/code-stubs-arm64.cc:3331: __
Pop(VectorLoadICDescriptor::SlotRegister());
On 2015/03/23 14:23:59, Yang wrote:
> Can we use the equivalent Pop with two registers?

Done. There is some hilarity here, because arm and arm64 have a different
convention for the Pop() instruction (left-to-right, versus right-to-left).

https://codereview.chromium.org/1028093002/diff/40001/src/mips/code-stubs-mip...
File src/mips/code-stubs-mips.cc (right):

https://codereview.chromium.org/1028093002/diff/40001/src/mips/code-stubs-mip...
src/mips/code-stubs-mips.cc:3087: __
pop(VectorLoadICDescriptor::VectorRegister());
On 2015/03/23 14:23:59, Yang wrote:
> Same here, corresponding Pop?

Done.

https://codereview.chromium.org/1028093002/diff/40001/test/mjsunit/string-ind...
File test/mjsunit/string-index.js (right):

https://codereview.chromium.org/1028093002/diff/40001/test/mjsunit/string-ind...
test/mjsunit/string-index.js:255: var keys = [0, Math.abs(2)];
On 2015/03/23 14:23:59, Yang wrote:
> You probably should check that this is not a smi via %IsSmi

Done. And the Math.floor(x) * 0.5 trick seems to work better than abs().

[mvstanton, 2015-03-23 15:59:54]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-03-23 15:59:54]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1028093002/#ps100001
(title: "REBASE.")

[commit-bot: I haz the power, 2015-03-23 16:00:15]

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

[commit-bot: I haz the power, 2015-03-23 16:09:33]

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

[commit-bot: I haz the power, 2015-03-23 16:09:34]

Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[mvstanton, 2015-03-23 17:48:13]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-03-23 17:48:13]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1028093002/#ps120001
(title: "Disable assert.")

[commit-bot: I haz the power, 2015-03-23 17:48:28]

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

[commit-bot: I haz the power, 2015-03-23 18:50:18]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-03-23 18:50:34]

Patchset 6 (id:??) landed as
https://crrev.com/6689cc27ebe60685c025de9ae1f09919093f8213
Cr-Commit-Position: refs/heads/master@{#27377}