Recognize List constructor and turn it into CreateArray ...

Recognize List. constructor and turn it inot CreateArray.

R=johnmccutchan@google.com, zra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=36138

[Cutch, 2014-05-12 19:50:02]

lgtm

[Vyacheslav Egorov (Google), 2014-05-13 08:10:38]

Constants can start flowing into the constructor after other phases as well,
e.g. store to load forwarding can forward one. 

I wonder if we should start moving some of these specializations into a pass
that is run more often (e.g. Canonicalization?) instead of bolting more special
passes on the side.

[Florian Schneider, 2014-05-13 11:17:31]

I also think that we don't need a separate pass for this.

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4253: num_elements->BoundConstant().IsSmi())
{
Why does num_elements need to be constant? CreateArrayInstr takes an arbitrary
length operand as input.

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4256: CreateArrayInstr* create_array =
How about doing this in the flow graph builder instead? _List. is a native
function, so we could just emit a CreateArrayInstr right away.

[srdjan, 2014-05-13 18:07:18]

Thanks for feedback. Adapted CreateArrayInstr to accept any input.

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4253: num_elements->BoundConstant().IsSmi())
{
On 2014/05/13 11:17:32, Florian Schneider wrote:
> Why does num_elements need to be constant? CreateArrayInstr takes an arbitrary
> length operand as input.

CreateArrayInstr expects a positive integer. Added code to handle exceptions.

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4256: CreateArrayInstr* create_array =
On 2014/05/13 11:17:32, Florian Schneider wrote:
> How about doing this in the flow graph builder instead? _List. is a native
> function, so we could just emit a CreateArrayInstr right away.

Done.

[Cutch, 2014-05-13 18:20:03]

lgtm

[srdjan, 2014-05-13 18:24:43]

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4256: CreateArrayInstr* create_array =
On 2014/05/13 18:07:18, srdjan wrote:
> On 2014/05/13 11:17:32, Florian Schneider wrote:
> > How about doing this in the flow graph builder instead? _List. is a native
> > function, so we could just emit a CreateArrayInstr right away.
> 
> Done.

Actually not done. FlowGraphBuilder is run in unoptimized code as well, do not
want to add method recognition etc there.  Also, there may be requirements in
the VM that a static call in unoptimized code remains static call.

[zra, 2014-05-13 21:45:53]

arm/mips changes lgtm

[srdjan, 2014-05-13 23:09:29]

Committed patchset #17 manually as r36138 (presubmit successful).

[Florian Schneider, 2014-05-14 08:23:27]

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4253: num_elements->BoundConstant().IsSmi())
{
On 2014/05/13 18:07:18, srdjan wrote:
> On 2014/05/13 11:17:32, Florian Schneider wrote:
> > Why does num_elements need to be constant? CreateArrayInstr takes an
arbitrary
> > length operand as input.
> 
> CreateArrayInstr expects a positive integer. Added code to handle exceptions.

Sorry to be not precise enough: I meant checking for the correct length here:
You can use CheckArrayBoundInstr for that. It will deoptmize in case of a
negative length.

Inserting the check in the allocation stub is not ideal: it can't be eliminated
there in case we actually have a constant, so it will always cost extra.

[srdjan, 2014-05-14 15:21:01]

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/285543002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4253: num_elements->BoundConstant().IsSmi())
{
On 2014/05/14 08:23:28, Florian Schneider wrote:
> On 2014/05/13 18:07:18, srdjan wrote:
> > On 2014/05/13 11:17:32, Florian Schneider wrote:
> > > Why does num_elements need to be constant? CreateArrayInstr takes an
> arbitrary
> > > length operand as input.
> > 
> > CreateArrayInstr expects a positive integer. Added code to handle
exceptions.
> 
> Sorry to be not precise enough: I meant checking for the correct length here:
> You can use CheckArrayBoundInstr for that. It will deoptmize in case of a
> negative length.
> 
> Inserting the check in the allocation stub is not ideal: it can't be
eliminated
> there in case we actually have a constant, so it will always cost extra.
> 

This code is just the first step for optimized allocation in case of constants:
inline allocation code skipping all kind of checks and object tag computations.
This especially visible in case of frequent growable array allocations
(allocates List(4)). Experimental code showed 4% improvement on Havlak.