Make KeyedLoads from a sloppy arguments array use a handler.

Make KeyedLoads from a sloppy arguments array use a handler.

Before, a custom stub was installed.

R=verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=24120

[mvstanton, 2014-09-17 13:05:15]

Patchset #2 (id:20001) has been deleted

[mvstanton, 2014-09-17 13:11:55]

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

[mvstanton, 2014-09-17 13:11:56]

Hi Toon,
Here is the change we discussed. The novelty is that I made a HandlerStub
(hydrogen) for the handler. A fair bit of handwritten code is removed, and if
this work extends into the KeyedStore case, even more.
PTAL, thanks!
--Michael

[Toon Verwaest, 2014-09-18 11:07:43]

Looks good, but I'd like to have at least a high-level comment about the layout
of the sloppy arguments object.

[mvstanton, 2014-09-18 14:01:48]

Patchset #5 (id:100001) has been deleted

[mvstanton, 2014-09-18 14:02:58]

Patchset #5 (id:120001) has been deleted

[mvstanton, 2014-09-18 14:05:36]

Hi Toon,
I added a comment, hopefully helpful. I also thought about just adding
pseudo-code to describe what was being done. Let me know if this seems better:

The HydrogenCodeStub attempts to do the following:

if (!index.IsSmi() || index < 0) MISS();
if (index + 2 < elements.length) {
  t = elements[index + 2];
  if (t == the_hole) {
    backing_store = elements[1];
    if (!backing_store.IsFixedArray()) MISS();
    if (backing_store.length >= index) MISS();
    return backing_store[index];
  } else {
    context = elements[0];
    return context[t];
  }
} else {
  backing_store = elements[1];
  if (!backing_store.IsFixedArray()) MISS();
  if (backing_store.length >= index) MISS();
  return backing_store[index];
}

Thx!
--Michael

[Toon Verwaest, 2014-09-22 08:29:47]

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

https://codereview.chromium.org/546683003/diff/160001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:633: //
Remove // before and after the block ;)

https://codereview.chromium.org/546683003/diff/160001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:636: // are stored in the context at indexes given by
elements[key + 2].  Unmapped
Space too many before Unmapped.

https://codereview.chromium.org/546683003/diff/160001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:663: positive_smi.If<HCompareNumericAndBranch>(key,
graph()->GetConstant0(),
Mmh you compare
key < 0 && key < length - 2
but then load using key - 2

What about key == 0 or key == 1?

https://codereview.chromium.org/546683003/diff/160001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:684: Handle<Object>
hole(isolate()->heap()->the_hole_value(), isolate());
isolate->factory()->the_hole_value() is already a handle.

[mvstanton, 2014-09-22 10:34:31]

Thanks for the look, here is the update,
--Michael

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

https://codereview.chromium.org/546683003/diff/160001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:633: //
On 2014/09/22 08:29:47, Toon Verwaest wrote:
> Remove // before and after the block ;)

This was my attempt to indicate "importance" :D. Done.

https://codereview.chromium.org/546683003/diff/160001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:636: // are stored in the context at indexes given by
elements[key + 2].  Unmapped
On 2014/09/22 08:29:47, Toon Verwaest wrote:
> Space too many before Unmapped.

Done.

https://codereview.chromium.org/546683003/diff/160001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:663: positive_smi.If<HCompareNumericAndBranch>(key,
graph()->GetConstant0(),
On 2014/09/22 08:29:47, Toon Verwaest wrote:
> Mmh you compare
> key < 0 && key < length - 2
> but then load using key - 2
> 
> What about key == 0 or key == 1?

As we just talked over, there is an add right before the load, so the load is on
elements[key + 2].

https://codereview.chromium.org/546683003/diff/160001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:684: Handle<Object>
hole(isolate()->heap()->the_hole_value(), isolate());
On 2014/09/22 08:29:47, Toon Verwaest wrote:
> isolate->factory()->the_hole_value() is already a handle.

Done. And I fixed another place in this file with the same mistake,
StoreGlobalStub.

[Toon Verwaest, 2014-09-22 11:22:55]

lgtm

[mvstanton, 2014-09-22 13:23:47]

Committed patchset #9 (id:220001) manually as 24120 (presubmit successful).