Turn ElementsTransitionAndStore stub into a HydrogenCodeStub.

Turn ElementsTransitionAndStore stub into a HydrogenCodeStub.

R=danno@chromium.org, mvstanton@chromium.org

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

[Benedikt Meurer, 2013-07-10 09:29:00]

Cleaning up the unused assembly code is scheduled for a follow-up CL.

[danno, 2013-07-10 09:52:36]

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

https://codereview.chromium.org/18881004/diff/1/src/arm/code-stubs-arm.cc#new...
src/arm/code-stubs-arm.cc:269: descriptor->deoptimization_handler_ =
Runtime::FunctionForId(
Yikes. In general, you should access members of the Stub during descriptor
initialization, this seems fragile. I would much prefer having one descriptor
per stub class and one stub class per descriptor .

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

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:937: set_current_block(NULL);
I think it's cleaner if you return value here and then the code below doesn't
have to be in an else.

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:945: AddStore(array, HObjectAccess::ForMap(),
target_map);
Why the special casing here? Either the stub should have no side-effects and
defer to the runtime, or it should handle the case completely. Why can you not
just use BuildUncheckedMonomorphicElementAccess with the "holeyified" elements
kind? Also, this case applies to all packed->holey transitions, I think, e.g.
the if should be:

if (GetPackedElementsKind(from_kind) == GetPackedElementsKind(to_kind) &&  
    IsFastPackedElementsKind(from_kind) &&
    IsFastHoleyElementsKind(to_kind)) {
...

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:956: IsFastObjectElementsKind(to_kind))) {
Can you create a predicate in elements-kind.h to the effect
DoesTransitionChangeElementsBufferFormat or something similar?

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:963:
if_fixed.If<HCompareNumericAndBranch>(elements_length,
You can use IfNot here and then put the meat of the code below in the "Then" and
omit the "Else"

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:974: BuildNewSpaceArrayCheck(elements_length,
to_kind);
Why can't you use the helper function "BuildGrowElementsCapacity" for this line
and the following ~10 or so?

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:996: BuildUncheckedMonomorphicElementAccess(array,
key, value, NULL,
You can probably factor this out to only be called once for both the elements
format change and simple map changes. For simple map changes, you will have to
first do a 

to_kind = GetHoleyElementsKind(to_kind);

https://codereview.chromium.org/18881004/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/18881004/diff/1/src/code-stubs.h#newcode2214
src/code-stubs.h:2214: class ElementsTransitionAndStoreStub : public
HydrogenCodeStub {
I kind of like the existing invariant that each stub has it's own descriptor and
there is always a 1-to-1 mapping. It think it would be cleaner if you make this
a superclass with two subclasses, one for strict and one for non-strict. then
you could override the script_mode() method (make it virtual), and everything
else should just work.

https://codereview.chromium.org/18881004/diff/1/src/code-stubs.h#newcode2259
src/code-stubs.h:2259: StoreModeBits::encode(store_mode_);
If you choose to do it this way, use the accessor functions.

[Benedikt Meurer, 2013-07-10 12:23:09]

Addressed comments. Also take into account the is_jsarray flag.

PTAL

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

https://codereview.chromium.org/18881004/diff/1/src/arm/code-stubs-arm.cc#new...
src/arm/code-stubs-arm.cc:269: descriptor->deoptimization_handler_ =
Runtime::FunctionForId(
On 2013/07/10 09:52:37, danno wrote:
> Yikes. In general, you should access members of the Stub during descriptor
> initialization, this seems fragile. I would much prefer having one descriptor
> per stub class and one stub class per descriptor .

Done.

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

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:937: set_current_block(NULL);
On 2013/07/10 09:52:37, danno wrote:
> I think it's cleaner if you return value here and then the code below doesn't
> have to be in an else.

Done.

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:945: AddStore(array, HObjectAccess::ForMap(),
target_map);
On 2013/07/10 09:52:37, danno wrote:
> Why the special casing here? Either the stub should have no side-effects and
> defer to the runtime, or it should handle the case completely. Why can you not
> just use BuildUncheckedMonomorphicElementAccess with the "holeyified" elements
> kind? Also, this case applies to all packed->holey transitions, I think, e.g.
> the if should be:
> 
> if (GetPackedElementsKind(from_kind) == GetPackedElementsKind(to_kind) &&  
>     IsFastPackedElementsKind(from_kind) &&
>     IsFastHoleyElementsKind(to_kind)) {
> ...

Done.

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:956: IsFastObjectElementsKind(to_kind))) {
On 2013/07/10 09:52:37, danno wrote:
> Can you create a predicate in elements-kind.h to the effect
> DoesTransitionChangeElementsBufferFormat or something similar?

Done.

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:963:
if_fixed.If<HCompareNumericAndBranch>(elements_length,
On 2013/07/10 09:52:37, danno wrote:
> You can use IfNot here and then put the meat of the code below in the "Then"
and
> omit the "Else"

Done.

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:974: BuildNewSpaceArrayCheck(elements_length,
to_kind);
On 2013/07/10 09:52:37, danno wrote:
> Why can't you use the helper function "BuildGrowElementsCapacity" for this
line
> and the following ~10 or so?

Done.

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:996: BuildUncheckedMonomorphicElementAccess(array,
key, value, NULL,
On 2013/07/10 09:52:37, danno wrote:
> You can probably factor this out to only be called once for both the elements
> format change and simple map changes. For simple map changes, you will have to
> first do a 
> 
> to_kind = GetHoleyElementsKind(to_kind);

Maybe I'm missing the point here, but BuildUncheckedMonomorphicElementAccess is
called only once.

https://codereview.chromium.org/18881004/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/18881004/diff/1/src/code-stubs.h#newcode2214
src/code-stubs.h:2214: class ElementsTransitionAndStoreStub : public
HydrogenCodeStub {
On 2013/07/10 09:52:37, danno wrote:
> I kind of like the existing invariant that each stub has it's own descriptor
and
> there is always a 1-to-1 mapping. It think it would be cleaner if you make
this
> a superclass with two subclasses, one for strict and one for non-strict. then
> you could override the script_mode() method (make it virtual), and everything
> else should just work.

Done.

https://codereview.chromium.org/18881004/diff/1/src/code-stubs.h#newcode2259
src/code-stubs.h:2259: StoreModeBits::encode(store_mode_);
On 2013/07/10 09:52:37, danno wrote:
> If you choose to do it this way, use the accessor functions.

Done.

[danno, 2013-07-10 12:33:44]

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

https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#ne...
src/code-stubs-hydrogen.cc:974: BuildNewSpaceArrayCheck(elements_length,
to_kind);
On 2013/07/10 09:52:37, danno wrote:
> Why can't you use the helper function "BuildGrowElementsCapacity" for this
line
> and the following ~10 or so?

Done.

https://codereview.chromium.org/18881004/diff/5002/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/18881004/diff/5002/src/code-stubs.h#newcode88
src/code-stubs.h:88: V(ElementsTransitionAndStoreNonStrict) \
I had another look at KeyedStoreIC_Miss and KeyedStore ICs. If you use a
StoreKeyedIC, you can get the strictness from the calling IC, it really is
redundant on the code stub. 
Please create a KeyedStoreIC in the transition and store stub runtime call in
C++, and use it to extract the strictness like we do for the plain
KeyedStoreICs.

Sorry for the noise :-/

[Benedikt Meurer, 2013-07-10 13:23:45]

Certainly easier this way... once you know that you can do it this way. :-)

https://codereview.chromium.org/18881004/diff/5002/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/18881004/diff/5002/src/code-stubs.h#newcode88
src/code-stubs.h:88: V(ElementsTransitionAndStoreNonStrict) \
On 2013/07/10 12:33:44, danno wrote:
> I had another look at KeyedStoreIC_Miss and KeyedStore ICs. If you use a
> StoreKeyedIC, you can get the strictness from the calling IC, it really is
> redundant on the code stub. 
> Please create a KeyedStoreIC in the transition and store stub runtime call in
> C++, and use it to extract the strictness like we do for the plain
> KeyedStoreICs.
> 
> Sorry for the noise :-/

Done.

[mvstanton, 2013-07-11 14:05:12]

This looks good to me (lgtm).

As we discussed, you'll turn the flag off again for purposes of maintaining a
baseline and staged entry to the tree.

[Benedikt Meurer, 2013-07-11 14:07:57]

On 2013/07/11 14:05:12, mvstanton wrote:
> This looks good to me (lgtm).
> 
> As we discussed, you'll turn the flag off again for purposes of maintaining a
> baseline and staged entry to the tree.

Latest patch set turns off compiled transitions as discussed.

[danno, 2013-07-11 14:27:54]

lgtm

[Benedikt Meurer, 2013-07-11 14:29:10]

Committed patchset #5 manually as r15635 (presubmit successful).

[Toon Verwaest, 2013-07-15 14:32:49]

This CL breaks crypto-sha1 and crypto-md5 in sunspider.

[Benedikt Meurer, 2013-07-15 17:18:41]

On 2013/07/15 14:32:49, Toon Verwaest wrote:
> This CL breaks crypto-sha1 and crypto-md5 in sunspider.

How did it break exactly?

[Toon Verwaest, 2013-07-16 05:46:22]

Sha1 segfaults; md5 returns the wrong result iirc.