[crankshaft] Loads and stores to typed arrays have to reference the backing store holder

[crankshaft] Loads and stores to typed arrays have to reference the backing store holder

The backing store is only held alive indirectly via the array buffer
referenced by the holder (typed array), so it's not enough to keep the
elements alive (or even just the external pointer loaded from the
elements).

R=mstarzinger@chromium.org,bmeurer@chromium.org
LOG=n
BUG=v8:1827
Committed: https://crrev.com/086d4598473fe25b6b8f802998d6e85e90625136
Cr-Commit-Position: refs/heads/master@{#32644}

[Benedikt Meurer, 2015-12-07 05:02:03]

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

[Benedikt Meurer, 2015-12-07 05:02:03]

As mentioned several times already, I'm really worried about the "lack of love"
in our TypedArray implementation. I know it is not the fault of a single person
or a single group that our current implementation status is the way it is, but
I'm also not sure that we are actually making things better following the
current "duck tape approach" that has been applied since 1 - 1 1/2 years now.

Applying "duck tape metrics" this is LGTM with nits from my side.

https://codereview.chromium.org/1493983004/diff/60001/src/crankshaft/hydrogen...
File src/crankshaft/hydrogen-instructions.h (right):

https://codereview.chromium.org/1493983004/diff/60001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen-instructions.h:6574: DCHECK_EQ(index, 3);
Nit: DCHECK_EQ(3, index)

https://codereview.chromium.org/1493983004/diff/60001/src/crankshaft/hydrogen...
src/crankshaft/hydrogen-instructions.h:6999: DCHECK_EQ(index, 3);
Nit: DCHECK_EQ(3, index)

https://codereview.chromium.org/1493983004/diff/60001/src/crankshaft/x64/lith...
File src/crankshaft/x64/lithium-x64.cc (right):

https://codereview.chromium.org/1493983004/diff/60001/src/crankshaft/x64/lith...
src/crankshaft/x64/lithium-x64.cc:2201: LOperand* backing_store_owner =
UseAny(instr->backing_store_owner());
I think UseAny in this case will prevent aliasing of backing_store_owner and
result, which means you need at least 4 free registers for this instruction,
even tho 3 would suffice. But given that we are sort of sloppy about the
constraints in this case anyway (i.e. UseRegister always on instr->elements())
it's probably fine to say UseAny here.

https://codereview.chromium.org/1493983004/diff/60001/src/crankshaft/x64/lith...
src/crankshaft/x64/lithium-x64.cc:2299: LOperand* backing_store_owner =
UseAny(instr->backing_store_owner());
Hm, I'm not sure if UseAny is the ideal choice here; but my knowledge about the
lithium allocator is close to zero at this point, so it's probably OK even tho
not 100% ideal.

[Benedikt Meurer, 2015-12-07 05:02:16]

Description was changed from

==========
Loads and stores to typed arrays have to reference the backing store holder

The backing store is only held alive indirectly via the array buffer
referenced by the holder (typed array), so it's not enough to keep the
elements alive (or even just the external pointer loaded from the
elements).

R=mstarzinger@chromium.org,bmeurer@chromium.org
LOG=n
BUG=v8:1827
==========

to

==========
[crankshaft] Loads and stores to typed arrays have to reference the backing
store holder

The backing store is only held alive indirectly via the array buffer
referenced by the holder (typed array), so it's not enough to keep the
elements alive (or even just the external pointer loaded from the
elements).

R=mstarzinger@chromium.org,bmeurer@chromium.org
LOG=n
BUG=v8:1827
==========

[jochen (gone - plz use gerrit), 2015-12-07 08:08:00]

jochen@chromium.org changed reviewers:
+ jarin@chromium.org

[jochen (gone - plz use gerrit), 2015-12-07 08:08:00]

jochen@chromium.org changed required reviewers:
- mstarzinger@chromium.org

[jochen (gone - plz use gerrit), 2015-12-07 08:08:00]

jarin@, can you comment on the use of UseAny() vs. UseRegister()? I was under
the impression that UseAny() would allow the register allocator to not use a
register..

[jochen (gone - plz use gerrit), 2015-12-07 08:14:09]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-12-07 08:14:10]

(jarin@ said offline to use UseAny)

[jochen (gone - plz use gerrit), 2015-12-07 08:14:10]

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/1493983004/#ps80001
(title: "updates")

[commit-bot: I haz the power, 2015-12-07 08:14:13]

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

[commit-bot: I haz the power, 2015-12-07 08:37:33]

Description was changed from

==========
[crankshaft] Loads and stores to typed arrays have to reference the backing
store holder

The backing store is only held alive indirectly via the array buffer
referenced by the holder (typed array), so it's not enough to keep the
elements alive (or even just the external pointer loaded from the
elements).

R=mstarzinger@chromium.org,bmeurer@chromium.org
LOG=n
BUG=v8:1827
==========

to

==========
[crankshaft] Loads and stores to typed arrays have to reference the backing
store holder

The backing store is only held alive indirectly via the array buffer
referenced by the holder (typed array), so it's not enough to keep the
elements alive (or even just the external pointer loaded from the
elements).

R=mstarzinger@chromium.org,bmeurer@chromium.org
LOG=n
BUG=v8:1827
==========

[commit-bot: I haz the power, 2015-12-07 08:37:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-07 08:37:51]

Description was changed from

==========
[crankshaft] Loads and stores to typed arrays have to reference the backing
store holder

The backing store is only held alive indirectly via the array buffer
referenced by the holder (typed array), so it's not enough to keep the
elements alive (or even just the external pointer loaded from the
elements).

R=mstarzinger@chromium.org,bmeurer@chromium.org
LOG=n
BUG=v8:1827
==========

to

==========
[crankshaft] Loads and stores to typed arrays have to reference the backing
store holder

The backing store is only held alive indirectly via the array buffer
referenced by the holder (typed array), so it's not enough to keep the
elements alive (or even just the external pointer loaded from the
elements).

R=mstarzinger@chromium.org,bmeurer@chromium.org
LOG=n
BUG=v8:1827

Committed: https://crrev.com/086d4598473fe25b6b8f802998d6e85e90625136
Cr-Commit-Position: refs/heads/master@{#32644}
==========

[commit-bot: I haz the power, 2015-12-07 08:37:51]

Patchset 5 (id:??) landed as
https://crrev.com/086d4598473fe25b6b8f802998d6e85e90625136
Cr-Commit-Position: refs/heads/master@{#32644}