Add DoubleToSmi optimistically, preventing boxing/unboxing of doubles and propagate smi-nessof the …

runtime/vm/flow_graph_optimizer.cc

Index: runtime/vm/flow_graph_optimizer.cc
===================================================================
--- runtime/vm/flow_graph_optimizer.cc	(revision 16185)
+++ runtime/vm/flow_graph_optimizer.cc	(working copy)
@@ -1203,9 +1203,17 @@
   if ((recognized_kind == MethodRecognizer::kDoubleToInteger) &&
       (class_ids[0] == kDoubleCid)) {
     AddCheckClass(call, call->ArgumentAt(0)->value()->Copy());
-    DoubleToIntegerInstr* d2int_instr =
-        new DoubleToIntegerInstr(call->ArgumentAt(0)->value(), call);
-    call->ReplaceWith(d2int_instr, current_iterator());
+    ASSERT(call->HasICData());
+    const ICData& ic_data = *call->ic_data();
+    Definition* d2i_instr = NULL;
+    if (ic_data.deopt_reason() == kDeoptDoubleToSmi) {
+      // Do not repeatedly deoptimize because result didn't fit into Smi.
+      d2i_instr = new DoubleToIntegerInstr(call->ArgumentAt(0)->value(), call);

[Vyacheslav Egorov (Google), 2012/12/17 12:53:44]

It would be great if we could test things like this.

[In V8 we are using "natives syntax" to peak into optimization status:
%GetOptimizationStatus(func) it would be great if we could write tests using
some low level API available only to tests]

[srdjan, 2012/12/18 00:12:47]

I do manual tests, by tracing deoptimization. I agree it would be nice to have a
framework to better test various optimizations. The worst thing that can happen
is a performance regression.

+    } else {
+      // Optimistically assume result fits into Smi.
+      d2i_instr = new DoubleToSmiInstr(call->ArgumentAt(0)->value(), call);
+    }
+    call->ReplaceWith(d2i_instr, current_iterator());
     RemovePushArguments(call);
     return true;
   }
@@ -4047,6 +4055,12 @@
 }
 
 
+void ConstantPropagator::VisitDoubleToSmi(DoubleToSmiInstr* instr) {
+  // TODO(kmillikin): Handle conversion.
+  SetValue(instr, non_constant_);

[Vyacheslav Egorov (Google), 2012/12/17 12:53:44]

I think we should stop posponing this to TODOs when implementation is
straightforward.

Our ConstantPropagator is already full of them.

[srdjan, 2012/12/18 00:12:47]

I think there some work needed to implement constant propagation, e.g.. the
input here is an unboxed double typically, not a double object... etc.

I'd rather have those changes in a different CL

[Vyacheslav Egorov (Google), 2012/12/18 11:45:10]

Agreed about separate CL.

I was under impression that we run constant propagation before representation
selection but apparently this is not the case. I am not sure why. As it is know
there is a bit of "obstruction" inserted between double constants and their
uses. 

+}
+
+
 void ConstantPropagator::VisitConstant(ConstantInstr* instr) {
   SetValue(instr, instr->value());
 }