Reland "Suppress canonicalization of Unbox() instruction that can deoptimize."

This relands commit r41693 with additional fixes in canonicalization pass.

R=fschneider@google.com
BUG=

Review URL: https://codereview.chromium.org//726593002

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@41736 260f80e4-7a28-3924-810f-c04153c831b5

runtime/vm/flow_graph_optimizer.cc

@@ -589,7 +589,14 @@ bool FlowGraphOptimizer::Canonicalize() {
     BlockEntryInstr* entry = block_order_[i];
     for (ForwardInstructionIterator it(entry); !it.Done(); it.Advance()) {
       Instruction* current = it.Current();
+      if (current->HasUnmatchedInputRepresentations()) {
+        // Can't canonicalize this instruction until all conversions for its
+        // inputs are inserted.
+        continue;
+      }
+
       Instruction* replacement = current->Canonicalize(flow_graph());
+
       if (replacement != current) {
         // For non-definitions Canonicalize should return either NULL or
         // this.

runtime/vm/flow_graph_range_analysis.cc

@@ -2402,7 +2402,7 @@ void Range::Shr(const Range* left,
 }
 
 
-bool Range::And(const Range* left_range,
+void Range::And(const Range* left_range,
                 const Range* right_range,
                 RangeBoundary* result_min,
                 RangeBoundary* result_max) {
@@ -2414,16 +2414,17 @@ bool Range::And(const Range* left_range,
   if (Range::ConstantMin(right_range).ConstantValue() >= 0) {
     *result_min = RangeBoundary::FromConstant(0);
     *result_max = Range::ConstantMax(right_range);
-    return true;
+    return;
   }
 
   if (Range::ConstantMin(left_range).ConstantValue() >= 0) {
     *result_min = RangeBoundary::FromConstant(0);
     *result_max = Range::ConstantMax(left_range);
-    return true;
+    return;
   }
 
-  return false;
+  *result_min = RangeBoundary::MinConstant(RangeBoundary::kRangeBoundaryInt64);
+  *result_max = RangeBoundary::MaxConstant(RangeBoundary::kRangeBoundaryInt64);
 }
 
 
@@ -2524,7 +2525,7 @@ void Range::Sub(const Range* left_range,
 }
 
 
-bool Range::Mul(const Range* left_range,
+void Range::Mul(const Range* left_range,
                 const Range* right_range,
                 RangeBoundary* result_min,
                 RangeBoundary* result_max) {
@@ -2545,7 +2546,7 @@ bool Range::Mul(const Range* left_range,
     const int64_t r_max =
         OnlyNegativeOrZero(*left_range, *right_range) ? 0 : mul_max;
     *result_max = RangeBoundary::FromConstant(r_max);
-    return true;
+    return;
   }
 
   // TODO(vegorov): handle mixed sign case that leads to (-Infinity, 0] range.
@@ -2553,10 +2554,11 @@ bool Range::Mul(const Range* left_range,
       OnlyNegativeOrZero(*left_range, *right_range)) {
     *result_min = RangeBoundary::FromConstant(0);
     *result_max = RangeBoundary::PositiveInfinity();
-    return true;
+    return;
   }
 
-  return false;
+  *result_min = RangeBoundary::NegativeInfinity();
+  *result_max = RangeBoundary::PositiveInfinity();
 }
 
 
@@ -2603,39 +2605,39 @@ void Range::BinaryOp(const Token::Kind op,
     case Token::kADD:
       Range::Add(left_range, right_range, &min, &max, left_defn);
       break;
+
     case Token::kSUB:
       Range::Sub(left_range, right_range, &min, &max, left_defn);
       break;
-    case Token::kMUL: {
-      if (!Range::Mul(left_range, right_range, &min, &max)) {
-        *result = Range::Full(RangeBoundary::kRangeBoundaryInt64);
-        return;
-      }
+
+    case Token::kMUL:
+      Range::Mul(left_range, right_range, &min, &max);
       break;
-    }
-    case Token::kSHL: {
+
+    case Token::kSHL:
       Range::Shl(left_range, right_range, &min, &max);
       break;
-    }
-    case Token::kSHR: {
+
+    case Token::kSHR:
       Range::Shr(left_range, right_range, &min, &max);
       break;
-    }
 
     case Token::kBIT_AND:
-      if (!Range::And(left_range, right_range, &min, &max)) {
-        *result = Range::Full(RangeBoundary::kRangeBoundaryInt64);
-        return;
-      }
+      Range::And(left_range, right_range, &min, &max);
       break;
 
     case Token::kBIT_XOR:
       Range::Xor(left_range, right_range, &min, &max);
       break;
 
-    default:
+    case Token::kBIT_OR:
       *result = Range::Full(RangeBoundary::kRangeBoundaryInt64);
       return;
+
+    default:
+      *result = Range(RangeBoundary::NegativeInfinity(),
+                      RangeBoundary::PositiveInfinity());
+      return;
   }
 
   ASSERT(!min.IsUnknown() && !max.IsUnknown());

runtime/vm/flow_graph_range_analysis.h

@@ -440,7 +440,7 @@ class Range : public ZoneAllocated {
                   RangeBoundary* max,
                   Definition* left_defn);
 
-  static bool Mul(const Range* left_range,
+  static void Mul(const Range* left_range,
                   const Range* right_range,
                   RangeBoundary* min,
                   RangeBoundary* max);
@@ -454,7 +454,7 @@ class Range : public ZoneAllocated {
                   RangeBoundary* min,
                   RangeBoundary* max);
 
-  static bool And(const Range* left_range,
+  static void And(const Range* left_range,
                   const Range* right_range,
                   RangeBoundary* min,
                   RangeBoundary* max);

runtime/vm/flow_graph_range_analysis_test.cc

@@ -531,8 +531,9 @@ TEST_CASE(RangeAnd) {
                  static_cast<int64_t>(20),
                  static_cast<int64_t>(-20),
                  static_cast<int64_t>(20),
-                 RangeBoundary(),
-                 RangeBoundary());
+                 RangeBoundary::MinConstant(RangeBoundary::kRangeBoundaryInt64),
+                 RangeBoundary::MaxConstant(
+                    RangeBoundary::kRangeBoundaryInt64));
 
 #undef TEST_RANGE_AND
 }

runtime/vm/intermediate_language.cc

@@ -789,6 +789,18 @@ bool Instruction::IsDominatedBy(Instruction* dom) {
 }
 
 
+bool Instruction::HasUnmatchedInputRepresentations() const {
+  for (intptr_t i = 0; i < InputCount(); i++) {
+    Definition* input = InputAt(i)->definition();
+    if (RequiredInputRepresentation(i) != input->representation()) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+
 void Definition::ReplaceWith(Definition* other,
                              ForwardInstructionIterator* iterator) {
   // Record other's input uses.
@@ -1205,7 +1217,8 @@ bool BinaryInt32OpInstr::CanDeoptimize() const {
       return false;
 
     case Token::kSHL:
-      return true;
+      return can_overflow() ||
+          !RangeUtils::IsPositive(right()->definition()->range());
 
     case Token::kMOD: {
       UNREACHABLE();
@@ -1228,19 +1241,14 @@ bool BinarySmiOpInstr::CanDeoptimize() const {
     case Token::kBIT_OR:
     case Token::kBIT_XOR:
       return false;
-    case Token::kSHR: {
-      // Can't deopt if shift-count is known positive.
-      Range* right_range = this->right()->definition()->range();
-      return (right_range == NULL) || !right_range->IsPositive();
-    }
-    case Token::kSHL: {
-      Range* right_range = this->right()->definition()->range();
-      if ((right_range != NULL) && !can_overflow()) {
-        // Can deoptimize if right can be negative.
-        return !right_range->IsPositive();
-      }
-      return true;
-    }
+
+    case Token::kSHR:
+      return !RangeUtils::IsPositive(right()->definition()->range());
+
+    case Token::kSHL:
+      return can_overflow() ||
+          !RangeUtils::IsPositive(right()->definition()->range());
+
     case Token::kMOD: {
       Range* right_range = this->right()->definition()->range();
       return (right_range == NULL) || right_range->Overlaps(0, 0);
@@ -1994,7 +2002,7 @@ Definition* BoxInt64Instr::Canonicalize(FlowGraph* flow_graph) {
 
 
 Definition* UnboxInstr::Canonicalize(FlowGraph* flow_graph) {
-  if (!HasUses()) return NULL;
+  if (!HasUses() && !CanDeoptimize()) return NULL;
 
   // Fold away Unbox<rep>(Box<rep>(v)).
   BoxInstr* box_defn = value()->definition()->AsBox();
@@ -2026,7 +2034,7 @@ Definition* UnboxInstr::Canonicalize(FlowGraph* flow_graph) {
 
 
 Definition* UnboxIntegerInstr::Canonicalize(FlowGraph* flow_graph) {
-  if (!HasUses()) return NULL;
+  if (!HasUses() && !CanDeoptimize()) return NULL;
 
   // Fold away UnboxInteger<rep_to>(BoxInteger<rep_from>(v)).
   BoxIntegerInstr* box_defn = value()->definition()->AsBoxInteger();
@@ -2040,7 +2048,7 @@ Definition* UnboxIntegerInstr::Canonicalize(FlowGraph* flow_graph) {
           box_defn->value()->CopyWithType(),
           (representation() == kUnboxedInt32) ?
               GetDeoptId() : Isolate::kNoDeoptId);
-      if ((representation() == kUnboxedInt32) && is_truncating()) {
+      if ((representation() == kUnboxedInt32) && !CanDeoptimize()) {
         converter->mark_truncating();
       }
       flow_graph->InsertBefore(this, converter, env(), FlowGraph::kValue);
@@ -2071,6 +2079,9 @@ Definition* UnboxInt32Instr::Canonicalize(FlowGraph* flow_graph) {
 
     UnboxedConstantInstr* uc =
         new UnboxedConstantInstr(c->value(), kUnboxedInt32);
+    if (c->range() != NULL) {
+      uc->set_range(*c->range());
+    }
     flow_graph->InsertBefore(this, uc, NULL, FlowGraph::kValue);
     return uc;
   }
@@ -2250,19 +2261,12 @@ Instruction* BranchInstr::Canonicalize(FlowGraph* flow_graph) {
       return this;
     }
     ComparisonInstr* comp = replacement->AsComparison();
-    if ((comp == NULL) || comp->CanDeoptimize()) {
+    if ((comp == NULL) ||
+        comp->CanDeoptimize() ||
+        comp->HasUnmatchedInputRepresentations()) {
       return this;
     }
 
-    // Assert that the comparison is not serving as a pending deoptimization
-    // target for conversions.
-    for (intptr_t i = 0; i < comp->InputCount(); i++) {
-      if (comp->RequiredInputRepresentation(i) !=
-          comp->InputAt(i)->definition()->representation()) {
-        return this;
-      }
-    }
-
     // Replace the comparison if the replacement is used at this branch,
     // and has exactly one use.
     Value* use = comp->input_use_list();

runtime/vm/intermediate_language.h

@@ -741,6 +741,8 @@ FOR_EACH_ABSTRACT_INSTRUCTION(INSTRUCTION_TYPE_CHECK)
     lifetime_position_ = pos;
   }
 
+  bool HasUnmatchedInputRepresentations() const;
+
   // Returns representation expected for the input operand at the given index.
   virtual Representation RequiredInputRepresentation(intptr_t idx) const {
     return kTagged;

tests/language/vm/canonicalization_preserves_deopt_test.dart

@@ -0,0 +1,41 @@
+// Copyright (c) 2014, 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.
+// VMOptions=--optimization_counter_threshold=10 --no-use-osr
+
+import "package:expect/expect.dart";
+
+class X {
+  operator * (other) => "NaNNaNNaNNaNBatman";
+}
+
+foo(x) => (x * 1.0) is double;
+
+bar(x) {
+  try {
+    int i = (x * 1);
+    return true;
+  } catch (e) {
+    return false;
+  }
+}
+
+baz(x) => (x * 1) == x;
+
+main() {
+  for (var i = 0; i < 100; i++) {
+    Expect.isTrue(foo(1.0));
+    assert(() {
+      Expect.isTrue(bar(-1 << 63));
+      return true;
+    });
+    Expect.isTrue(baz(-1 << 63));
+  }
+  Expect.isFalse(foo(new X()));
+  assert(() {
+    Expect.isFalse(bar(new X()));
+    return true;
+  });
+  Expect.isFalse(baz(new X()));
+}
+