[vm/compiler] Generate non-speculative Unbox instructions for Phis

TEST=runtime/tests/vm/dart/regress_flutter83094_test.dart
Issue flutter/flutter#83094

Change-Id: Ib4eebc993e06f6925f11bd18e5f29f22ba3c6322
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/201363
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Alexander Markov <[email protected]>

runtime/tests/vm/dart/regress_flutter83094_test.dart

@@ -0,0 +1,27 @@
+// Copyright (c) 2021, 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.
+
+// Verifies that compiler doesn't crash due to incompatible types
+// when unboxing input of a Phi.
+// Regression test for https://github.com/flutter/flutter/issues/83094.
+
+import 'package:expect/expect.dart';
+
+class A {
+  @pragma('vm:never-inline')
+  double getMaxIntrinsicWidth() => 1.toDouble();
+}
+
+A _leading = A();
+
+@pragma('vm:never-inline')
+double computeMaxIntrinsicWidth(double height, double horizontalPadding) {
+  final leadingWidth =
+      _leading == null ? 0 : _leading.getMaxIntrinsicWidth() as int;
+  return horizontalPadding + leadingWidth;
+}
+
+main() {
+  Expect.throws(() => computeMaxIntrinsicWidth(1, 2));
+}

runtime/tests/vm/dart_2/regress_flutter83094_test.dart

@@ -0,0 +1,27 @@
+// Copyright (c) 2021, 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.
+
+// Verifies that compiler doesn't crash due to incompatible types
+// when unboxing input of a Phi.
+// Regression test for https://github.com/flutter/flutter/issues/83094.
+
+import 'package:expect/expect.dart';
+
+class A {
+  @pragma('vm:never-inline')
+  double getMaxIntrinsicWidth() => 1.toDouble();
+}
+
+A _leading = A();
+
+@pragma('vm:never-inline')
+double computeMaxIntrinsicWidth(double height, double horizontalPadding) {
+  final leadingWidth =
+      _leading == null ? 0 : _leading.getMaxIntrinsicWidth() as int;
+  return horizontalPadding + leadingWidth;
+}
+
+main() {
+  Expect.throws(() => computeMaxIntrinsicWidth(1, 2));
+}

runtime/vm/compiler/backend/flow_graph.cc

@@ -1869,22 +1869,26 @@ void FlowGraph::InsertConversion(Representation from,
                                  bool is_environment_use) {
   ASSERT(from != to);
   Instruction* insert_before;
-  Instruction* deopt_target;
   PhiInstr* phi = use->instruction()->AsPhi();
   if (phi != NULL) {
     ASSERT(phi->is_alive());
     // For phis conversions have to be inserted in the predecessor.
     auto predecessor = phi->block()->PredecessorAt(use->use_index());
     insert_before = predecessor->last_instruction();
     ASSERT(insert_before->GetBlock() == predecessor);
-    deopt_target = NULL;
   } else {
-    deopt_target = insert_before = use->instruction();
+    insert_before = use->instruction();
+  }
+  const Instruction::SpeculativeMode speculative_mode =
+      use->instruction()->SpeculativeModeOfInput(use->use_index());
+  Instruction* deopt_target = nullptr;
+  if (speculative_mode == Instruction::kGuardInputs || to == kUnboxedInt32) {
+    deopt_target = insert_before;
   }
 
   Definition* converted = NULL;
   if (IsUnboxedInteger(from) && IsUnboxedInteger(to)) {
-    const intptr_t deopt_id = (to == kUnboxedInt32) && (deopt_target != NULL)
+    const intptr_t deopt_id = (to == kUnboxedInt32) && (deopt_target != nullptr)
                                   ? deopt_target->DeoptimizationTarget()
                                   : DeoptId::kNone;
     converted =
@@ -1893,49 +1897,50 @@ void FlowGraph::InsertConversion(Representation from,
     converted = new Int32ToDoubleInstr(use->CopyWithType());
   } else if ((from == kUnboxedInt64) && (to == kUnboxedDouble) &&
              CanConvertInt64ToDouble()) {
-    const intptr_t deopt_id = (deopt_target != NULL)
+    const intptr_t deopt_id = (deopt_target != nullptr)
                                   ? deopt_target->DeoptimizationTarget()
                                   : DeoptId::kNone;
     ASSERT(CanUnboxDouble());
     converted = new Int64ToDoubleInstr(use->CopyWithType(), deopt_id);
   } else if ((from == kTagged) && Boxing::Supports(to)) {
-    const intptr_t deopt_id = (deopt_target != NULL)
+    const intptr_t deopt_id = (deopt_target != nullptr)
                                   ? deopt_target->DeoptimizationTarget()
                                   : DeoptId::kNone;
-    converted = UnboxInstr::Create(
-        to, use->CopyWithType(), deopt_id,
-        use->instruction()->SpeculativeModeOfInput(use->use_index()));
+    converted =
+        UnboxInstr::Create(to, use->CopyWithType(), deopt_id, speculative_mode);
   } else if ((to == kTagged) && Boxing::Supports(from)) {
     converted = BoxInstr::Create(from, use->CopyWithType());
   } else {
     // We have failed to find a suitable conversion instruction.
     // Insert two "dummy" conversion instructions with the correct
     // "from" and "to" representation. The inserted instructions will
     // trigger a deoptimization if executed. See #12417 for a discussion.
-    const intptr_t deopt_id = (deopt_target != NULL)
+    // If the use is not speculative, then this code should be unreachable.
+    // Insert Stop for a graceful error and aid unreachable code elimination.
+    if (speculative_mode == Instruction::kNotSpeculative) {
+      StopInstr* stop = new (Z) StopInstr("Incompatible conversion.");
+      InsertBefore(insert_before, stop, nullptr, FlowGraph::kEffect);
+    }
+    const intptr_t deopt_id = (deopt_target != nullptr)
                                   ? deopt_target->DeoptimizationTarget()
                                   : DeoptId::kNone;
     ASSERT(Boxing::Supports(from));
     ASSERT(Boxing::Supports(to));
     Definition* boxed = BoxInstr::Create(from, use->CopyWithType());
     use->BindTo(boxed);
-    InsertBefore(insert_before, boxed, NULL, FlowGraph::kValue);
-    converted = UnboxInstr::Create(to, new (Z) Value(boxed), deopt_id);
+    InsertBefore(insert_before, boxed, nullptr, FlowGraph::kValue);
+    converted = UnboxInstr::Create(to, new (Z) Value(boxed), deopt_id,
+                                   speculative_mode);
   }
-  ASSERT(converted != NULL);
-  InsertBefore(insert_before, converted, use->instruction()->env(),
+  ASSERT(converted != nullptr);
+  InsertBefore(insert_before, converted,
+               (deopt_target != nullptr) ? deopt_target->env() : nullptr,
                FlowGraph::kValue);
   if (is_environment_use) {
     use->BindToEnvironment(converted);
   } else {
     use->BindTo(converted);
   }
-
-  if ((to == kUnboxedInt32) && (phi != NULL)) {
-    // Int32 phis are unboxed optimistically. Ensure that unboxing
-    // has deoptimization target attached from the goto instruction.
-    CopyDeoptTarget(converted, insert_before);
-  }
 }
 
 void FlowGraph::InsertConversionsFor(Definition* def) {

runtime/vm/compiler/backend/il.h

@@ -2481,9 +2481,12 @@ class PhiInstr : public Definition {
 
   virtual void set_representation(Representation r) { representation_ = r; }
 
-  // In AOT mode Phi instructions do not check types of inputs when unboxing.
+  // Only Int32 phis in JIT mode are unboxed optimistically.
   virtual SpeculativeMode SpeculativeModeOfInput(intptr_t index) const {
-    return CompilerState::Current().is_aot() ? kNotSpeculative : kGuardInputs;
+    return (CompilerState::Current().is_aot() ||
+            (representation_ != kUnboxedInt32))
+               ? kNotSpeculative
+               : kGuardInputs;
   }
 
   virtual uword Hash() const {
@@ -3140,6 +3143,9 @@ class GotoInstr : public TemplateInstruction<0, NoThrow> {
     return true;
   }
 
+  // May require a deoptimization target for int32 Phi input conversions.
+  virtual intptr_t DeoptimizationTarget() const { return GetDeoptId(); }
+
   virtual bool ComputeCanDeoptimize() const { return false; }
 
   virtual bool HasUnknownSideEffects() const { return false; }
@@ -4097,6 +4103,7 @@ class InstanceCallBaseInstr : public TemplateDartCall<0> {
       }
       idx--;
     }
+    if (interface_target_.IsNull()) return kGuardInputs;
     return interface_target_.is_unboxed_parameter_at(idx) ? kNotSpeculative
                                                           : kGuardInputs;
   }