Use same range info when emitting code and computing if instruction can deopt.

For BinarySmiOpInstr and ShiftMintOpInstr we use range information to both
decide if instruction can deopt and decide which checks should be emitted
when emitting native code for this instruction.

However because range information is attached to the definition and not
uses it often gets out of sync as we mutate the graph. For example we might
first make a decision that an instruction can't deoptimize based on more
precise range information but then use less precise information in the
backend for deciding which parts of the instruction to emit (because
redifinition or a phi-instruction was removed from the graph). This mismatch
can lead to a crash if less precise information tells backend that one of the
inputs need to be checked - because there is no deoptimization label to jump
to.

This CL is addressing this problem by ensuring that range information is
cached at the use and both ComputeCanDeoptimize() and backend use the same
range information.

Additionally this CL kills overly generic MergedMath instruction and replaces it
with TruncDivModInstr.

BUG=#29620
[email protected]

Review-Url: https://codereview.chromium.org/2891113002 .

Author: mraleph

runtime/tests/vm/dart/regress29620_test.dart

@@ -0,0 +1,61 @@
+// Copyright (c) 2017, 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.
+
+// Regression test for dartbug.com/29620: check that decision to deoptimize
+// and decisions which parts of the instruction to emit use the same
+// range information for instruction inputs.
+
+// VMOptions=--enable-inlining-annotations --optimization_counter_threshold=10 --no-use-osr --no-background-compilation
+
+import "package:expect/expect.dart";
+
+const alwaysInline = "AlwaysInline";
+const neverInline = "NeverInline";
+
+class Flag {
+  var value;
+  Flag(this.value);
+
+  static final FLAG = new Flag(0);
+}
+
+@alwaysInline
+void checkRange(bit) {
+  if (bit < 0 || bit > 31) {
+    throw "bit must be in [0, 31]";
+  }
+}
+
+@alwaysInline
+bool isSet(flags, bit) {
+  checkRange(bit);
+  // Note: > 0 here instead of == 0 to prevent merging into
+  // TestSmi instruction.
+  return (flags & (1 << bit)) > 0;
+}
+
+@neverInline
+bool bug(flags) {
+  var bit = Flag.FLAG.value;
+  checkRange(bit);
+  for (var i = 0; i < 1; i++) {
+    bit = Flag.FLAG.value;
+    checkRange(bit);
+  }
+
+  // In early optimization stages `bit` would be a Phi(...). This Phi would be
+  // dominated by checkRange and thus range analysis will infer [0, 31] range
+  // for it - and thus a EliminateEnvironment will make decision that
+  // (1 << bit) can't deoptimize and will detach environment from it. Later
+  // passes will eliminate Phi for `bit` as it is redundant and as a result we
+  // will loose precise range information for `bit` and backend will try
+  // to emit a range check and a deoptimization.
+  return isSet(flags, bit);
+}
+
+main() {
+  for (var i = 0; i < 100; i++) {
+    bug(1);
+  }
+}

runtime/vm/constant_propagator.cc

@@ -1119,7 +1119,7 @@ void ConstantPropagator::VisitInvokeMathCFunction(
 }
 
 
-void ConstantPropagator::VisitMergedMath(MergedMathInstr* instr) {
+void ConstantPropagator::VisitTruncDivMod(TruncDivModInstr* instr) {
   // TODO(srdjan): Handle merged instruction.
   SetValue(instr, non_constant_);
 }

runtime/vm/flow_graph.cc

@@ -2280,20 +2280,18 @@ void FlowGraph::TryMergeTruncDivMod(
         (*merge_candidates)[k] = NULL;  // Clear it.
         ASSERT(curr_instr->HasUses());
         AppendExtractNthOutputForMerged(
-            curr_instr, MergedMathInstr::OutputIndexOf(curr_instr->op_kind()),
+            curr_instr, TruncDivModInstr::OutputIndexOf(curr_instr->op_kind()),
             kTagged, kSmiCid);
         ASSERT(other_binop->HasUses());
         AppendExtractNthOutputForMerged(
-            other_binop, MergedMathInstr::OutputIndexOf(other_binop->op_kind()),
-            kTagged, kSmiCid);
-
-        ZoneGrowableArray<Value*>* args = new (Z) ZoneGrowableArray<Value*>(2);
-        args->Add(new (Z) Value(curr_instr->left()->definition()));
-        args->Add(new (Z) Value(curr_instr->right()->definition()));
+            other_binop,
+            TruncDivModInstr::OutputIndexOf(other_binop->op_kind()), kTagged,
+            kSmiCid);
 
         // Replace with TruncDivMod.
-        MergedMathInstr* div_mod = new (Z) MergedMathInstr(
-            args, curr_instr->deopt_id(), MergedMathInstr::kTruncDivMod);
+        TruncDivModInstr* div_mod = new (Z) TruncDivModInstr(
+            curr_instr->left()->CopyWithType(),
+            curr_instr->right()->CopyWithType(), curr_instr->deopt_id());
         curr_instr->ReplaceWith(div_mod, NULL);
         other_binop->ReplaceUsesWith(div_mod);
         other_binop->RemoveFromGraph();

runtime/vm/flow_graph_range_analysis.cc

@@ -2256,6 +2256,12 @@ void Range::Clamp(RangeBoundary::RangeSize size) {
 }
 
 
+void Range::ClampToConstant(RangeBoundary::RangeSize size) {
+  min_ = min_.LowerBound().Clamp(size);
+  max_ = max_.UpperBound().Clamp(size);
+}
+
+
 void Range::Shl(const Range* left,
                 const Range* right,
                 RangeBoundary* result_min,
@@ -2886,10 +2892,32 @@ void BinaryIntegerOpInstr::InferRangeHelper(const Range* left_range,
 }
 
 
+static void CacheRange(Range** slot,
+                       const Range* range,
+                       RangeBoundary::RangeSize size) {
+  if (range != NULL) {
+    if (*slot == NULL) {
+      *slot = new Range();
+    }
+    **slot = *range;
+
+    // Eliminate any symbolic dependencies from the range information.
+    (*slot)->ClampToConstant(size);
+  } else if (*slot != NULL) {
+    **slot = Range();  // Clear cached range information.
+  }
+}
+
+
 void BinarySmiOpInstr::InferRange(RangeAnalysis* analysis, Range* range) {
+  const Range* right_smi_range = analysis->GetSmiRange(right());
   // TODO(vegorov) completely remove this once GetSmiRange is eliminated.
-  InferRangeHelper(analysis->GetSmiRange(left()),
-                   analysis->GetSmiRange(right()), range);
+  if (op_kind() == Token::kSHR || op_kind() == Token::kSHL ||
+      op_kind() == Token::kMOD || op_kind() == Token::kTRUNCDIV) {
+    CacheRange(&right_range_, right_smi_range,
+               RangeBoundary::kRangeBoundarySmi);
+  }
+  InferRangeHelper(analysis->GetSmiRange(left()), right_smi_range, range);
 }
 
 
@@ -2906,6 +2934,8 @@ void BinaryMintOpInstr::InferRange(RangeAnalysis* analysis, Range* range) {
 
 
 void ShiftMintOpInstr::InferRange(RangeAnalysis* analysis, Range* range) {
+  CacheRange(&shift_range_, right()->definition()->range(),
+             RangeBoundary::kRangeBoundaryInt64);
   InferRangeHelper(left()->definition()->range(),
                    right()->definition()->range(), range);
 }

runtime/vm/flow_graph_range_analysis.h

@@ -396,6 +396,9 @@ class Range : public ZoneAllocated {
   // Clamp this to be within size.
   void Clamp(RangeBoundary::RangeSize size);
 
+  // Clamp this to be within size and eliminate symbols.
+  void ClampToConstant(RangeBoundary::RangeSize size);
+
   static void Add(const Range* left_range,
                   const Range* right_range,
                   RangeBoundary* min,
@@ -469,6 +472,16 @@ class RangeUtils : public AllStatic {
   static bool IsPositive(Range* range) {
     return !Range::IsUnknown(range) && range->IsPositive();
   }
+
+  static bool Overlaps(Range* range, intptr_t min, intptr_t max) {
+    return Range::IsUnknown(range) || range->Overlaps(min, max);
+  }
+
+  static bool CanBeZero(Range* range) { return Overlaps(range, 0, 0); }
+
+  static bool OnlyLessThanOrEqualTo(Range* range, intptr_t value) {
+    return !Range::IsUnknown(range) && range->OnlyLessThanOrEqualTo(value);
+  }
 };
 
 

runtime/vm/flow_graph_type_propagator.cc

@@ -1520,7 +1520,7 @@ CompileType InvokeMathCFunctionInstr::ComputeType() const {
 }
 
 
-CompileType MergedMathInstr::ComputeType() const {
+CompileType TruncDivModInstr::ComputeType() const {
   return CompileType::Dynamic();
 }
 

runtime/vm/il_printer.cc

@@ -738,8 +738,7 @@ void MathUnaryInstr::PrintOperandsTo(BufferFormatter* f) const {
 }
 
 
-void MergedMathInstr::PrintOperandsTo(BufferFormatter* f) const {
-  f->Print("'%s', ", MergedMathInstr::KindToCString(kind()));
+void TruncDivModInstr::PrintOperandsTo(BufferFormatter* f) const {
   Definition::PrintOperandsTo(f);
 }
 

runtime/vm/intermediate_language.cc

@@ -1392,8 +1392,9 @@ bool BinaryInt32OpInstr::ComputeCanDeoptimize() const {
       return false;
 
     case Token::kSHL:
-      return can_overflow() ||
-             !RangeUtils::IsPositive(right()->definition()->range());
+      // Currently only shifts by in range constant are supported, see
+      // BinaryInt32OpInstr::IsSupported.
+      return can_overflow();
 
     case Token::kMOD: {
       UNREACHABLE();
@@ -1413,22 +1414,25 @@ bool BinarySmiOpInstr::ComputeCanDeoptimize() const {
       return false;
 
     case Token::kSHR:
-      return !RangeUtils::IsPositive(right()->definition()->range());
+      return !RangeUtils::IsPositive(right_range());
 
     case Token::kSHL:
-      return can_overflow() ||
-             !RangeUtils::IsPositive(right()->definition()->range());
+      return can_overflow() || !RangeUtils::IsPositive(right_range());
+
+    case Token::kMOD:
+      return RangeUtils::CanBeZero(right_range());
 
-    case Token::kMOD: {
-      Range* right_range = this->right()->definition()->range();
-      return (right_range == NULL) || right_range->Overlaps(0, 0);
-    }
     default:
       return can_overflow();
   }
 }
 
 
+bool ShiftMintOpInstr::has_shift_count_check() const {
+  return !RangeUtils::IsWithin(shift_range(), 0, kMintShiftCountLimit);
+}
+
+
 bool BinaryIntegerOpInstr::RightIsPowerOfTwoConstant() const {
   if (!right()->definition()->IsConstant()) return false;
   const Object& constant = right()->definition()->AsConstant()->value();
@@ -4246,33 +4250,14 @@ const RuntimeEntry& CaseInsensitiveCompareUC16Instr::TargetFunction() const {
 }
 
 
-MergedMathInstr::MergedMathInstr(ZoneGrowableArray<Value*>* inputs,
-                                 intptr_t deopt_id,
-                                 MergedMathInstr::Kind kind)
-    : PureDefinition(deopt_id), inputs_(inputs), kind_(kind) {
-  ASSERT(inputs_->length() == InputCountFor(kind_));
-  for (intptr_t i = 0; i < inputs_->length(); ++i) {
-    ASSERT((*inputs)[i] != NULL);
-    (*inputs)[i]->set_instruction(this);
-    (*inputs)[i]->set_use_index(i);
-  }
-}
-
-
-intptr_t MergedMathInstr::OutputIndexOf(MethodRecognizer::Kind kind) {
-  switch (kind) {
-    case MethodRecognizer::kMathSin:
-      return 1;
-    case MethodRecognizer::kMathCos:
-      return 0;
-    default:
-      UNIMPLEMENTED();
-      return -1;
-  }
+TruncDivModInstr::TruncDivModInstr(Value* lhs, Value* rhs, intptr_t deopt_id)
+    : TemplateDefinition(deopt_id) {
+  SetInputAt(0, lhs);
+  SetInputAt(1, rhs);
 }
 
 
-intptr_t MergedMathInstr::OutputIndexOf(Token::Kind token) {
+intptr_t TruncDivModInstr::OutputIndexOf(Token::Kind token) {
   switch (token) {
     case Token::kTRUNCDIV:
       return 0;