Remove hand-written assembly ArrayPush stubs

Remove hand-written assembly ArrayPush stubs

R=mstarzinger@chromium.org, verwaest@chromium.org

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

[danno, 2014-04-11 12:36:27]

PTAL, adding Michael for spec conformance smell test

[Toon Verwaest, 2014-04-11 13:06:57]

Great in theory, but some bugs in the execution ;)

https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc#oldcode...
src/hydrogen.cc:7663: if (receiver_map->instance_type() != JS_ARRAY_TYPE) return
false;
This is invalid, since .push is spec'ed as reading the length, writing the
element, updating the length. 
1) regular objects don't have a HObjectAccess::ForArrayLength(...) and
2) the length may be observable through accessors.
Additionally, I presume UncheckedMono... doesn't actually do anything to a
"length" field for non-JSArrays, so the fact that the length is updated by .push
is relevant.

Please add tests to this effect.

https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:7666: if (!receiver_map->is_extensible()) return false;
I think is_extensible isn't necessary cause we guarantee we have dictionary
elements for those cases. We should definitely ASSERT though.

Both these conditions should also hold for ArrayPop. In fact, they are supposed
to hold for all BuildUncheckMonomorphicElementAccess users, so we should ASSERT
them in that method.

[Michael Starzinger, 2014-04-11 13:12:08]

https://codereview.chromium.org/233293005/diff/100001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/233293005/diff/100001/src/array.js#newcode475
src/array.js:475: %SetProperty(this, i+n, %_Arguments(i), 0, 0);
This will break for primitive values as "this" because there is not ToObject
conversion anywhere and primitive values seem to be ignored by
Runtime_SetProperty.

Also this passes "sloppy" as the language mode, which causes this [[Put]] not to
throw if the element cannot be put according to [[CanPut]].

Both of the above cases are already broken right now. So I am fine with leaving
this as it is, but can we leave a TODO that this is a temporary workaround? You
can assign this TODO to me if you want.

[danno, 2014-04-16 14:34:38]

Please take another look

https://codereview.chromium.org/233293005/diff/100001/src/array.js
File src/array.js (right):

https://codereview.chromium.org/233293005/diff/100001/src/array.js#newcode475
src/array.js:475: %SetProperty(this, i+n, %_Arguments(i), 0, 0);
On 2014/04/11 13:12:08, Michael Starzinger wrote:
> This will break for primitive values as "this" because there is not ToObject
> conversion anywhere and primitive values seem to be ignored by
> Runtime_SetProperty.
> 
> Also this passes "sloppy" as the language mode, which causes this [[Put]] not
to
> throw if the element cannot be put according to [[CanPut]].
> 
> Both of the above cases are already broken right now. So I am fine with
leaving
> this as it is, but can we leave a TODO that this is a temporary workaround?
You
> can assign this TODO to me if you want.

Done.

https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc#oldcode...
src/hydrogen.cc:7663: if (receiver_map->instance_type() != JS_ARRAY_TYPE) return
false;
On 2014/04/11 13:06:57, Toon Verwaest wrote:
> This is invalid, since .push is spec'ed as reading the length, writing the
> element, updating the length. 
> 1) regular objects don't have a HObjectAccess::ForArrayLength(...) and
> 2) the length may be observable through accessors.
> Additionally, I presume UncheckedMono... doesn't actually do anything to a
> "length" field for non-JSArrays, so the fact that the length is updated by
.push
> is relevant.
> 
> Please add tests to this effect.

Done.

https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc#newcode...
src/hydrogen.cc:7666: if (!receiver_map->is_extensible()) return false;
On 2014/04/11 13:06:57, Toon Verwaest wrote:
> I think is_extensible isn't necessary cause we guarantee we have dictionary
> elements for those cases. We should definitely ASSERT though.
> 
> Both these conditions should also hold for ArrayPop. In fact, they are
supposed
> to hold for all BuildUncheckMonomorphicElementAccess users, so we should
ASSERT
> them in that method.

I changed these into ASSERTs and added them to pop, too. Unfortuantely,
BuildUncheckMonomorphicElementAccess doesn't take a map so we can't use it as a
bottleneck.

[Michael Starzinger, 2014-04-16 15:56:24]

LGTM for array.js, I'll leave the rest to Toon. I will take care of the TODO in
array.js in the following days.

[Toon Verwaest, 2014-04-17 09:03:35]

lgtm

[danno, 2014-04-17 11:38:11]

Committed patchset #10 manually as r20839 (presubmit successful).