First step toward an optimizing compiler:

First step toward an optimizing compiler:
- Fix ic-data instrumentation
- Optimize '+' for Smi-s only, by replacing the instance call with inlined operations.
- Add deoptimization stub
- Add deoptimzation support.
Committed: https://code.google.com/p/dart/source/detail?r=7952

[Florian Schneider, 2012-05-24 00:20:38]

https://chromiumcodereview.appspot.com/10431006/diff/8003/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/8003/runtime/vm/compiler...
runtime/vm/compiler.cc:159: isolate->set_computation_id(0);
How about making the computation_id per FlowGraphBuilder instead of per-isolate?
Then there is no need to manually reset it.

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/flow_graph...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/flow_graph...
runtime/vm/flow_graph_compiler_x64.cc:68: assem->Bind(entry_label());
Maybe make a temporary definition of the __ macro for better readability?

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/flow_graph...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/flow_graph...
runtime/vm/flow_graph_optimizer.cc:66: if (classes[0]->raw() == cls2.raw()) {
I think this should be classes[1] here:

if (classes[0]->raw() == cls1.raw() &&
    classes[1]->raw() == cls2.raw()) {
  return true;
}

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/flow_graph...
runtime/vm/flow_graph_optimizer.cc:108:
comp->use()->definition()->replace_computation(bin_op);
As discussed offline, with a pointer to the Bind/DoInstr this would simply
become:

comp->instr()->replace_computation(bin_op);

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/il_printer.cc
File runtime/vm/il_printer.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/il_printer...
runtime/vm/il_printer.cc:103: f->Print(")");
Thanks for catching these!

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/intermedia...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/intermedia...
runtime/vm/intermediate_language.h:162: UseVal* use_;
Maybe store a Instruction* that points to the corresponding BindInstr or
DoInstr.

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/intermedia...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/intermedia...
runtime/vm/intermediate_language_x64.cc:518: Register kTemp = R10;
It is a little confusing to use "k" as prefix for registers. Maybe just 

Register temp = R10;

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/intermedia...
runtime/vm/intermediate_language_x64.cc:546: if (EmitSmiBinaryOp(compiler,
this)) {
Maybe rename it to TryEmitSmiBinaryOp to indicate that it may fail.

[srdjan, 2012-05-24 01:36:26]

Please take another look.

https://chromiumcodereview.appspot.com/10431006/diff/8003/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/8003/runtime/vm/compiler...
runtime/vm/compiler.cc:159: isolate->set_computation_id(0);
On 2012/05/24 00:20:38, Florian Schneider wrote:
> How about making the computation_id per FlowGraphBuilder instead of
per-isolate?
> Then there is no need to manually reset it.

It is used by FlowGraphOptimizer as well and by any other future passes. Also
note that there cannot be any variable statics in FlowGraphBuilder. The easiest
way for computations and instructions to allocate an id is via isolate storage.

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/flow_graph...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/flow_graph...
runtime/vm/flow_graph_compiler_x64.cc:68: assem->Bind(entry_label());
On 2012/05/24 00:20:39, Florian Schneider wrote:
> Maybe make a temporary definition of the __ macro for better readability?

Done

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/flow_graph...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/flow_graph...
runtime/vm/flow_graph_optimizer.cc:66: if (classes[0]->raw() == cls2.raw()) {
On 2012/05/24 00:20:39, Florian Schneider wrote:
> I think this should be classes[1] here:
> 
> if (classes[0]->raw() == cls1.raw() &&
>     classes[1]->raw() == cls2.raw()) {
>   return true;
> }

Aargh, thanks.

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/flow_graph...
runtime/vm/flow_graph_optimizer.cc:108:
comp->use()->definition()->replace_computation(bin_op);
On 2012/05/24 00:20:39, Florian Schneider wrote:
> As discussed offline, with a pointer to the Bind/DoInstr this would simply
> become:
> 
> comp->instr()->replace_computation(bin_op);

Yes. This was originally done as a step toward eliminating BindInstr/DoInstr.
However, after some discussion, this may not be the right path.

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/intermedia...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/intermedia...
runtime/vm/intermediate_language.h:162: UseVal* use_;
On 2012/05/24 00:20:39, Florian Schneider wrote:
> Maybe store a Instruction* that points to the corresponding BindInstr or
> DoInstr.

Done.

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/intermedia...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/18/runtime/vm/intermedia...
runtime/vm/intermediate_language_x64.cc:546: if (EmitSmiBinaryOp(compiler,
this)) {
On 2012/05/24 00:20:39, Florian Schneider wrote:
> Maybe rename it to TryEmitSmiBinaryOp to indicate that it may fail.

Done.

[Florian Schneider, 2012-05-24 02:13:29]

lgtm

https://chromiumcodereview.appspot.com/10431006/diff/16001/runtime/vm/flow_gr...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/16001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:64: if (!classes.is_empty()) {
It seems that after the check (ic_data.num_args_tested() != 2), classes can't be
empty. Maybe say instead:

ASSERT(!classes.is_empty());

https://chromiumcodereview.appspot.com/10431006/diff/16001/runtime/vm/interme...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/16001/runtime/vm/interme...
runtime/vm/intermediate_language_x64.cc:586: __ pushq(right);
It would be nice to avoid having to push the arguments back before emitting the
instance call code if inlining does not work out.

Would it be possible to determine already in the FlowGraphOptimizer whether to
inline or not, so that we don't have to deal with the non-inlined case here?

[srdjan, 2012-05-24 16:41:18]

https://chromiumcodereview.appspot.com/10431006/diff/16001/runtime/vm/flow_gr...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/16001/runtime/vm/flow_gr...
runtime/vm/flow_graph_optimizer.cc:64: if (!classes.is_empty()) {
On 2012/05/24 02:13:30, Florian Schneider wrote:
> It seems that after the check (ic_data.num_args_tested() != 2), classes can't
be
> empty. Maybe say instead:
> 
> ASSERT(!classes.is_empty());

Converted to assert:
ASSERT(classes.length() == 2)

https://chromiumcodereview.appspot.com/10431006/diff/16001/runtime/vm/interme...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10431006/diff/16001/runtime/vm/interme...
runtime/vm/intermediate_language_x64.cc:586: __ pushq(right);
On 2012/05/24 02:13:30, Florian Schneider wrote:
> It would be nice to avoid having to push the arguments back before emitting
the
> instance call code if inlining does not work out.
> 
> Would it be possible to determine already in the FlowGraphOptimizer whether to
> inline or not, so that we don't have to deal with the non-inlined case here?

This code is only transition code (adding comment).
 Yes, the optimizer should convert to binop only if the inlining is possible.

  // TODO(srdjan): Remove this code once BinaryOpComp has been implemeneted
  // for all intended operations.