Fast elements conversions in crankshaft using generated code.

Fast elements conversions in crankshaft using generated code.

[Yang, 2011-10-19 15:52:14]

Please take a look.

[Jakob Kummerow, 2011-10-19 18:36:31]

High-level comment:
I don't think overloading the functionality of the GenerateFoo() methods to
serve two pretty different purposes is the best approach here. It's sort of OK
when looking at this CL alone, but it's not a good basis for future
modifications (such as the upcoming --trace-elements-transitions flag) because
things will just become too complicated.
So what I'd suggest is to split each of them up into three methods (naming is
just a rough draft):
• GenerateStoreStubWithTransition, used to generate a stub that's called from
the keyed store ICs, handles the appropriate runtime call (and, in the future,
tracing flag) for this use case
• GenerateTransitionStub, used to generate a builtin that's called from
Crankshaft (could replace KeyedStoreIC::GenerateTransitionElements*)
• GenerateElementsTransitionInternal, called from the other two to do the
majority of the work

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

http://codereview.chromium.org/8344045/diff/1/src/code-stubs.h#newcode1058
src/code-stubs.h:1058: class FromBits:       public BitField<ElementsKind,    0,
4> {};
why this change? (I'm not saying it's bad; it's just surprising that you've
changed one ElementsKind bit field and not the other.)

http://codereview.chromium.org/8344045/diff/1/test/mjsunit/elements-transitio...
File test/mjsunit/elements-transition.js (right):

http://codereview.chromium.org/8344045/diff/1/test/mjsunit/elements-transitio...
test/mjsunit/elements-transition.js:41: print("smi->double [" + test_double +
Unintentional debugging leftover?

[danno, 2011-10-20 06:12:05]

My top level feedback is the similar to Jakob's. 

I would prefer to see clearer separation of responsibility the the generate
code. The core element transition code should be generated separately (and
packaged separately in the C++ source) from the action that is taken after the
transition. 

This separation was not clear before this CL (the core Generate method did the
tail call on failure, but didn't do the actual store), and I think it would be
good to clear it up as part of the refactor: one core routine that generates the
code for the transition and only the transition, and two 
 callers of that code that add the action that needs to be take in the success
and "failure" (GC) cases.

[Yang, 2011-10-20 08:38:34]

On 2011/10/20 06:12:05, danno wrote:
> My top level feedback is the similar to Jakob's. 
> 
> I would prefer to see clearer separation of responsibility the the generate
> code. The core element transition code should be generated separately (and
> packaged separately in the C++ source) from the action that is taken after the
> transition. 
> 
> This separation was not clear before this CL (the core Generate method did the
> tail call on failure, but didn't do the actual store), and I think it would be
> good to clear it up as part of the refactor: one core routine that generates
the
> code for the transition and only the transition, and two 
>  callers of that code that add the action that needs to be take in the success
> and "failure" (GC) cases.

Replaced by http://codereview.chromium.org/8355035/.