Improve constant propagation for Mint and Smi.

runtime/vm/flow_graph_optimizer.cc

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/flow_graph_optimizer.h"
 
 #include "vm/bit_vector.h"
 #include "vm/cha.h"
 #include "vm/flow_graph_builder.h"
 #include "vm/flow_graph_compiler.h"
@@ skipping 5320 matching lines @@
 void ConstantPropagator::VisitAllocateContext(AllocateContextInstr* instr) {
   SetValue(instr, non_constant_);
 }
 
 
 void ConstantPropagator::VisitCloneContext(CloneContextInstr* instr) {
   SetValue(instr, non_constant_);
 }
 
 
-void ConstantPropagator::VisitBinarySmiOp(BinarySmiOpInstr* instr) {
-  const Object& left = instr->left()->definition()->constant_value();
-  const Object& right = instr->right()->definition()->constant_value();
+void ConstantPropagator::HandleBinaryOp(Definition* instr,
+                                        Token::Kind op_kind,
+                                        const Value& left_val,
+                                        const Value& right_val) {
+  const Object& left = left_val.definition()->constant_value();
+  const Object& right = right_val.definition()->constant_value();
   if (IsNonConstant(left) || IsNonConstant(right)) {
     SetValue(instr, non_constant_);

[Kevin Millikin (Google), 2013/05/22 07:50:35]

I think this is where we would support arithmetic simplifications.  Maybe add a
TODO to consider adding and subtracting 0, multiplication or division by 1,
multiplication by 0, etc.

[srdjan, 2013/05/22 15:13:33]

Added comment:
// TODO(srdjan): Add arithemtic simplifications, e.g, add with 0.

   } else if (IsConstant(left) && IsConstant(right)) {
-    if (left.IsSmi() && right.IsSmi()) {
-      const Smi& left_smi = Smi::Cast(left);
-      const Smi& right_smi = Smi::Cast(right);
-      switch (instr->op_kind()) {
+    if (left.IsInteger() && right.IsInteger()) {
+      const Integer& left_int = Integer::Cast(left);
+      const Integer& right_int = Integer::Cast(right);
+      switch (op_kind) {
         case Token::kADD:
         case Token::kSUB:
         case Token::kMUL:
         case Token::kTRUNCDIV:
         case Token::kMOD: {
-          const Object& result = Integer::ZoneHandle(
-              left_smi.ArithmeticOp(instr->op_kind(), right_smi));
+          Instance& result = Integer::ZoneHandle(
+              left_int.ArithmeticOp(op_kind, right_int));
+          result = result.Canonicalize();
           SetValue(instr, result);
           break;
         }
         case Token::kSHL:
         case Token::kSHR: {

[Kevin Millikin (Google), 2013/05/22 07:50:35]

I'm not sure about the official VM style, but I usually leave these braces off
when there are no declarations in the scope.

[srdjan, 2013/05/22 15:13:33]

Removed braces.

-          const Object& result = Integer::ZoneHandle(
-              left_smi.ShiftOp(instr->op_kind(), right_smi));
-          SetValue(instr, result);
+          if (left.IsSmi() && right.IsSmi()) {
+            Instance& result = Integer::ZoneHandle(
+                Smi::Cast(left_int).ShiftOp(op_kind, Smi::Cast(right_int)));
+            result = result.Canonicalize();
+            SetValue(instr, result);
+          } else {
+            SetValue(instr, non_constant_);
+          }
           break;
         }
         case Token::kBIT_AND:
         case Token::kBIT_OR:
         case Token::kBIT_XOR: {
-          const Object& result = Integer::ZoneHandle(
-              left_smi.BitOp(instr->op_kind(), right_smi));
+          Instance& result = Integer::ZoneHandle(
+              left_int.BitOp(op_kind, right_int));
+          result = result.Canonicalize();
           SetValue(instr, result);
           break;
         }
         default:
           // TODO(kmillikin): support other smi operations.

[Kevin Millikin (Google), 2013/05/22 07:50:35]

This comment is a bit out of date now.  I think the only operation not supported
here is Token::kDIV.

Maybe just add a SetValue(instr, non_constant_) case for that an an unreachable
default?

[srdjan, 2013/05/22 15:13:33]

Done.

           SetValue(instr, non_constant_);
       }
     } else {
       // TODO(kmillikin): support other types.
       SetValue(instr, non_constant_);
     }
   }
 }
 
 
+void ConstantPropagator::VisitBinarySmiOp(BinarySmiOpInstr* instr) {
+  HandleBinaryOp(instr, instr->op_kind(), *instr->left(), *instr->right());
+}
+
+
 void ConstantPropagator::VisitBoxInteger(BoxIntegerInstr* instr) {
   // TODO(kmillikin): Handle box operation.
   SetValue(instr, non_constant_);
 }
 
 
 void ConstantPropagator::VisitUnboxInteger(UnboxIntegerInstr* instr) {
   // TODO(kmillikin): Handle unbox operation.
   SetValue(instr, non_constant_);
 }
 
 
 void ConstantPropagator::VisitBinaryMintOp(
     BinaryMintOpInstr* instr) {
-  // TODO(kmillikin): Handle binary operations.
-  SetValue(instr, non_constant_);
+  HandleBinaryOp(instr, instr->op_kind(), *instr->left(), *instr->right());
 }
 
 
 void ConstantPropagator::VisitShiftMintOp(
     ShiftMintOpInstr* instr) {
-  // TODO(kmillikin): Handle shift operations.
-  SetValue(instr, non_constant_);
+  HandleBinaryOp(instr, instr->op_kind(), *instr->left(), *instr->right());
 }
 
 
 void ConstantPropagator::VisitUnaryMintOp(
     UnaryMintOpInstr* instr) {
   // TODO(kmillikin): Handle unary operations.
   SetValue(instr, non_constant_);
 }
 
 
@@ skipping 1073 matching lines @@
 
   // Insert materializations at environment uses.
   const Class& cls = Class::Handle(alloc->constructor().Owner());
   for (intptr_t i = 0; i < exits.length(); i++) {
     CreateMaterializationAt(exits[i], alloc, cls, *fields);
   }
 }
 
 
 }  // namespace dart