[vm/compiler] Remove speculative inputs from IL instructions

All inputs of IL instructions are now non-speculative,
SelectRepresentations pass now always inserts non-speculative
unboxing instructions.

In call specializer, where JIT compiler speculates, speculative Unbox
instructions are now explicitly added for inputs which may have
incorrect type. Unbox instructions now have 'value_mode' (either
kHasValidType or kCheckType) instead of speculative mode.
Added canonicalization rule to change value mode if
compiler can prove that value has correct type.

Also, cleaned up speculations around UnboxedInt32 representation in
JIT mode, which aligns this representation with other unboxed
representations.

TEST=ci

Change-Id: I52f5c959c3f6515b1a7c9e8ab15b420beed0e3f5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/391492
Commit-Queue: Alexander Markov <[email protected]>
Reviewed-by: Slava Egorov <[email protected]>

Author: alexmarkov

runtime/vm/compiler/aot/aot_call_specializer.cc

@@ -290,8 +290,7 @@ Value* AotCallSpecializer::PrepareStaticOpInput(Value* input,
     if (input->Type()->ToNullableCid() == kSmiCid) {
       conversion = new (Z) SmiToDoubleInstr(input, call->source());
     } else if (FlowGraphCompiler::CanConvertInt64ToDouble()) {
-      conversion = new (Z) Int64ToDoubleInstr(input, DeoptId::kNone,
-                                              Instruction::kNotSpeculative);
+      conversion = new (Z) Int64ToDoubleInstr(input, DeoptId::kNone);
     } else {
       UNREACHABLE();
     }
@@ -443,9 +442,8 @@ Definition* AotCallSpecializer::TryOptimizeDivisionOperation(
     if (magnitude == 1) return right_definition;
     InsertBefore(instr, right_definition, /*env=*/nullptr, FlowGraph::kValue);
     right_value = new (Z) Value(right_definition);
-    return new (Z)
-        BinaryInt64OpInstr(Token::kBIT_AND, left_value, right_value,
-                           DeoptId::kNone, Instruction::kNotSpeculative);
+    return new (Z) BinaryInt64OpInstr(Token::kBIT_AND, left_value, right_value,
+                                      DeoptId::kNone);
   } else {
     ASSERT_EQUAL(op_kind, Token::kTRUNCDIV);
 #if !defined(TARGET_ARCH_IS_32_BIT)
@@ -484,16 +482,14 @@ Definition* AotCallSpecializer::TryOptimizeDivisionOperation(
       auto* rounding_adjustment = unboxed_constant(magnitude - 1);
       InsertBefore(instr, rounding_adjustment, /*env=*/nullptr,
                    FlowGraph::kValue);
-      rounding_adjustment = new (Z)
-          BinaryInt64OpInstr(Token::kBIT_AND, new (Z) Value(sign_bit_extended),
-                             new (Z) Value(rounding_adjustment), DeoptId::kNone,
-                             Instruction::kNotSpeculative);
+      rounding_adjustment = new (Z) BinaryInt64OpInstr(
+          Token::kBIT_AND, new (Z) Value(sign_bit_extended),
+          new (Z) Value(rounding_adjustment), DeoptId::kNone);
       InsertBefore(instr, rounding_adjustment, /*env=*/nullptr,
                    FlowGraph::kValue);
-      auto* const left_definition = new (Z)
-          BinaryInt64OpInstr(Token::kADD, left_value->CopyWithType(Z),
-                             new (Z) Value(rounding_adjustment), DeoptId::kNone,
-                             Instruction::kNotSpeculative);
+      auto* const left_definition = new (Z) BinaryInt64OpInstr(
+          Token::kADD, left_value->CopyWithType(Z),
+          new (Z) Value(rounding_adjustment), DeoptId::kNone);
       InsertBefore(instr, left_definition, /*env=*/nullptr, FlowGraph::kValue);
       left_value = new (Z) Value(left_definition);
       auto* const right_definition =
@@ -553,7 +549,7 @@ bool AotCallSpecializer::TryOptimizeIntegerOperation(TemplateDartCall<0>* instr,
         replacement = new (Z) EqualityCompareInstr(
             instr->source(), op_kind, left_value->CopyWithType(Z),
             right_value->CopyWithType(Z), kMintCid, DeoptId::kNone,
-            /*null_aware=*/either_can_be_null, Instruction::kNotSpeculative);
+            /*null_aware=*/either_can_be_null);
         break;
       }
       case Token::kLT:
@@ -562,9 +558,9 @@ bool AotCallSpecializer::TryOptimizeIntegerOperation(TemplateDartCall<0>* instr,
       case Token::kGTE:
         left_value = PrepareStaticOpInput(left_value, kMintCid, instr);
         right_value = PrepareStaticOpInput(right_value, kMintCid, instr);
-        replacement = new (Z) RelationalOpInstr(
-            instr->source(), op_kind, left_value, right_value, kMintCid,
-            DeoptId::kNone, Instruction::kNotSpeculative);
+        replacement =
+            new (Z) RelationalOpInstr(instr->source(), op_kind, left_value,
+                                      right_value, kMintCid, DeoptId::kNone);
         break;
       case Token::kMOD:
       case Token::kTRUNCDIV:
@@ -605,9 +601,8 @@ bool AotCallSpecializer::TryOptimizeIntegerOperation(TemplateDartCall<0>* instr,
         } else {
           left_value = PrepareStaticOpInput(left_value, kMintCid, instr);
           right_value = PrepareStaticOpInput(right_value, kMintCid, instr);
-          replacement = new (Z)
-              BinaryInt64OpInstr(op_kind, left_value, right_value,
-                                 DeoptId::kNone, Instruction::kNotSpeculative);
+          replacement = new (Z) BinaryInt64OpInstr(op_kind, left_value,
+                                                   right_value, DeoptId::kNone);
         }
         break;
       }
@@ -626,8 +621,8 @@ bool AotCallSpecializer::TryOptimizeIntegerOperation(TemplateDartCall<0>* instr,
 
     if (op_kind == Token::kNEGATE || op_kind == Token::kBIT_NOT) {
       left_value = PrepareStaticOpInput(left_value, kMintCid, instr);
-      replacement = new (Z) UnaryInt64OpInstr(
-          op_kind, left_value, DeoptId::kNone, Instruction::kNotSpeculative);
+      replacement =
+          new (Z) UnaryInt64OpInstr(op_kind, left_value, DeoptId::kNone);
     }
   }
 
@@ -679,8 +674,7 @@ bool AotCallSpecializer::TryOptimizeDoubleOperation(TemplateDartCall<0>* instr,
           right_value = PrepareStaticOpInput(right_value, kDoubleCid, instr);
           replacement = new (Z) EqualityCompareInstr(
               instr->source(), op_kind, left_value, right_value, kDoubleCid,
-              DeoptId::kNone, /*null_aware=*/false,
-              Instruction::kNotSpeculative);
+              DeoptId::kNone, /*null_aware=*/false);
           break;
         }
         break;
@@ -694,9 +688,9 @@ bool AotCallSpecializer::TryOptimizeDoubleOperation(TemplateDartCall<0>* instr,
       case Token::kGTE: {
         left_value = PrepareStaticOpInput(left_value, kDoubleCid, instr);
         right_value = PrepareStaticOpInput(right_value, kDoubleCid, instr);
-        replacement = new (Z) RelationalOpInstr(
-            instr->source(), op_kind, left_value, right_value, kDoubleCid,
-            DeoptId::kNone, Instruction::kNotSpeculative);
+        replacement =
+            new (Z) RelationalOpInstr(instr->source(), op_kind, left_value,
+                                      right_value, kDoubleCid, DeoptId::kNone);
         break;
       }
       case Token::kADD:
@@ -709,8 +703,7 @@ bool AotCallSpecializer::TryOptimizeDoubleOperation(TemplateDartCall<0>* instr,
         left_value = PrepareStaticOpInput(left_value, kDoubleCid, instr);
         right_value = PrepareStaticOpInput(right_value, kDoubleCid, instr);
         replacement = new (Z) BinaryDoubleOpInstr(
-            op_kind, left_value, right_value, DeoptId::kNone, instr->source(),
-            Instruction::kNotSpeculative);
+            op_kind, left_value, right_value, DeoptId::kNone, instr->source());
         break;
       }
 
@@ -739,8 +732,7 @@ bool AotCallSpecializer::TryOptimizeDoubleOperation(TemplateDartCall<0>* instr,
     if (op_kind == Token::kNEGATE) {
       left_value = PrepareStaticOpInput(left_value, kDoubleCid, instr);
       replacement = new (Z)
-          UnaryDoubleOpInstr(Token::kNEGATE, left_value, instr->deopt_id(),
-                             Instruction::kNotSpeculative);
+          UnaryDoubleOpInstr(Token::kNEGATE, left_value, instr->deopt_id());
     }
   }
 
@@ -1139,10 +1131,10 @@ bool AotCallSpecializer::TryReplaceInstanceOfWithRangeCheck(
   if (lower_limit == upper_limit) {
     ConstantInstr* cid_constant = flow_graph()->GetConstant(
         Smi::Handle(Z, Smi::New(lower_limit)), kUnboxedUword);
-    check_range = new (Z) EqualityCompareInstr(
-        call->source(), Token::kEQ, new Value(load_cid),
-        new Value(cid_constant), kIntegerCid, DeoptId::kNone, false,
-        Instruction::kNotSpeculative);
+    check_range = new (Z)
+        EqualityCompareInstr(call->source(), Token::kEQ, new Value(load_cid),
+                             new Value(cid_constant), kIntegerCid,
+                             DeoptId::kNone, /*null_aware=*/false);
   } else {
     check_range =
         new (Z) TestRangeInstr(call->source(), new (Z) Value(load_cid),

runtime/vm/compiler/backend/block_builder.h

@@ -100,20 +100,22 @@ class BlockBuilder : public ValueObject {
     return flow_graph_->GetConstant(Object::ZoneHandle());
   }
 
-  Definition* AddUnboxInstr(Representation rep, Value* value, bool is_checked) {
+  Definition* AddUnboxInstr(Representation rep,
+                            Value* value,
+                            UnboxInstr::ValueMode value_mode) {
     // Unbox floats by first unboxing a double then converting it to a float.
     auto const unbox_rep = rep == kUnboxedFloat
                                ? kUnboxedDouble
                                : Boxing::NativeRepresentation(rep);
-    Definition* unboxed_value =
-        AddDefinition(UnboxInstr::Create(unbox_rep, value, DeoptId::kNone));
+    Definition* unboxed_value = AddDefinition(
+        UnboxInstr::Create(unbox_rep, value, DeoptId::kNone, value_mode));
     if (rep != unbox_rep && unboxed_value->IsUnboxInteger()) {
       ASSERT(RepresentationUtils::ValueSize(rep) <
              RepresentationUtils::ValueSize(unbox_rep));
       // Mark unboxing of small unboxed integer representations as truncating.
       unboxed_value->AsUnboxInteger()->mark_truncating();
     }
-    if (is_checked) {
+    if (value_mode == UnboxInstr::ValueMode::kHasValidType) {
       // The type of |value| has already been checked and it is safe to
       // adjust reaching type. This is done manually because there is no type
       // propagation when building intrinsics.
@@ -129,8 +131,8 @@ class BlockBuilder : public ValueObject {
 
   Definition* AddUnboxInstr(Representation rep,
                             Definition* boxed,
-                            bool is_checked) {
-    return AddUnboxInstr(rep, new Value(boxed), is_checked);
+                            UnboxInstr::ValueMode value_mode) {
+    return AddUnboxInstr(rep, new Value(boxed), value_mode);
   }
 
   BranchInstr* AddBranch(ComparisonInstr* comp,

runtime/vm/compiler/backend/constant_propagator_test.cc

@@ -54,9 +54,9 @@ ISOLATE_UNIT_TEST_CASE(ConstantPropagation_PhiUnwrappingAndConvergence) {
     BlockBuilder builder(H.flow_graph(), b2);
     v1 = H.Phi(b2, {{b1, v0}, {b3, &v1}});
     builder.AddPhi(v1);
-    auto v2 = builder.AddDefinition(
-        new EqualityCompareInstr(InstructionSource(), Token::kEQ, new Value(v1),
-                                 new Value(v0), kSmiCid, S.GetNextDeoptId()));
+    auto v2 = builder.AddDefinition(new EqualityCompareInstr(
+        InstructionSource(), Token::kEQ, new Value(v1), new Value(v0), kSmiCid,
+        S.GetNextDeoptId(), /*null_aware=*/false));
     builder.AddBranch(new StrictCompareInstr(
                           InstructionSource(), Token::kEQ_STRICT, new Value(v2),
                           new Value(H.flow_graph()->GetConstant(Bool::True())),
@@ -245,7 +245,7 @@ ISOLATE_UNIT_TEST_CASE(ConstantPropagator_Regress35371) {
   auto make_int64_add = [](Definition* lhs, Definition* rhs,
                            intptr_t deopt_id) {
     return new BinaryInt64OpInstr(Token::kADD, new Value(lhs), new Value(rhs),
-                                  deopt_id, Instruction::kNotSpeculative);
+                                  deopt_id);
   };
 
   auto make_int32_add = [](Definition* lhs, Definition* rhs,

runtime/vm/compiler/backend/flow_graph.cc

@@ -1971,34 +1971,19 @@ void FlowGraph::InsertConversion(Representation from,
   } else {
     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 = nullptr;
   if (IsUnboxedInteger(from) && IsUnboxedInteger(to)) {
-    const intptr_t deopt_id = (to == kUnboxedInt32) && (deopt_target != nullptr)
-                                  ? deopt_target->DeoptimizationTarget()
-                                  : DeoptId::kNone;
-    converted =
-        new (Z) IntConverterInstr(from, to, use->CopyWithType(), deopt_id);
+    converted = new (Z)
+        IntConverterInstr(from, to, use->CopyWithType(), DeoptId::kNone);
   } else if ((from == kUnboxedInt32) && (to == kUnboxedDouble)) {
     converted = new Int32ToDoubleInstr(use->CopyWithType());
   } else if ((from == kUnboxedInt64) && (to == kUnboxedDouble) &&
              CanConvertInt64ToDouble()) {
-    const intptr_t deopt_id = (deopt_target != nullptr)
-                                  ? deopt_target->DeoptimizationTarget()
-                                  : DeoptId::kNone;
-    converted = new Int64ToDoubleInstr(use->CopyWithType(), deopt_id);
+    converted = new Int64ToDoubleInstr(use->CopyWithType(), DeoptId::kNone);
   } else if ((from == kTagged) && Boxing::Supports(to)) {
-    const intptr_t deopt_id = (deopt_target != nullptr)
-                                  ? deopt_target->DeoptimizationTarget()
-                                  : DeoptId::kNone;
-    converted =
-        UnboxInstr::Create(to, use->CopyWithType(), deopt_id, speculative_mode);
+    converted = UnboxInstr::Create(to, use->CopyWithType(), DeoptId::kNone,
+                                   UnboxInstr::ValueMode::kHasValidType);
   } else if ((to == kTagged) && Boxing::Supports(from)) {
     converted = BoxInstr::Create(from, use->CopyWithType());
   } else if ((to == kPairOfTagged) && (from == kTagged)) {
@@ -2042,23 +2027,16 @@ void FlowGraph::InsertConversion(Representation from,
     // trigger a deoptimization if executed. See #12417 for a discussion.
     // 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;
+    StopInstr* stop = new (Z) StopInstr("Incompatible conversion.");
+    InsertBefore(insert_before, stop, nullptr, FlowGraph::kEffect);
     Definition* boxed = BoxInstr::Create(from, use->CopyWithType());
     use->BindTo(boxed);
     InsertBefore(insert_before, boxed, nullptr, FlowGraph::kValue);
-    converted = UnboxInstr::Create(to, new (Z) Value(boxed), deopt_id,
-                                   speculative_mode);
+    converted = UnboxInstr::Create(to, new (Z) Value(boxed), DeoptId::kNone,
+                                   UnboxInstr::ValueMode::kHasValidType);
   }
   ASSERT(converted != nullptr);
-  InsertBefore(insert_before, converted,
-               (deopt_target != nullptr) ? deopt_target->env() : nullptr,
-               FlowGraph::kValue);
+  InsertBefore(insert_before, converted, nullptr, FlowGraph::kValue);
   if (is_environment_use) {
     use->BindToEnvironment(converted);
   } else {
@@ -2492,13 +2470,6 @@ bool FlowGraph::Canonicalize() {
     if (auto join = block->AsJoinEntry()) {
       for (PhiIterator it(join); !it.Done(); it.Advance()) {
         PhiInstr* current = it.Current();
-        if (current->HasUnmatchedInputRepresentations() &&
-            (current->SpeculativeModeOfInputs() == Instruction::kGuardInputs)) {
-          // Can't canonicalize this instruction until all conversions for its
-          // speculative inputs are inserted.
-          continue;
-        }
-
         Definition* replacement = current->Canonicalize(this);
         ASSERT(replacement != nullptr);
         RELEASE_ASSERT(unmatched_representations_allowed() ||
@@ -2512,13 +2483,6 @@ bool FlowGraph::Canonicalize() {
     }
     for (ForwardInstructionIterator it(block); !it.Done(); it.Advance()) {
       Instruction* current = it.Current();
-      if (current->HasUnmatchedInputRepresentations() &&
-          (current->SpeculativeModeOfInputs() == Instruction::kGuardInputs)) {
-        // Can't canonicalize this instruction until all conversions for its
-        // speculative inputs are inserted.
-        continue;
-      }
-
       Instruction* replacement = current->Canonicalize(this);
 
       if (replacement != current) {

runtime/vm/compiler/backend/flow_graph_compiler.cc

@@ -1794,17 +1794,13 @@ void FlowGraphCompiler::AllocateRegistersLocally(Instruction* instr) {
       switch (instr->RequiredInputRepresentation(i)) {
         case kUnboxedDouble:
           ASSERT(fpu_reg != kNoFpuRegister);
-          ASSERT(instr->SpeculativeModeOfInput(i) ==
-                 Instruction::kNotSpeculative);
           assembler()->LoadUnboxedDouble(
               fpu_reg, reg,
               compiler::target::Double::value_offset() - kHeapObjectTag);
           break;
         case kUnboxedFloat32x4:
         case kUnboxedFloat64x2:
           ASSERT(fpu_reg != kNoFpuRegister);
-          ASSERT(instr->SpeculativeModeOfInput(i) ==
-                 Instruction::kNotSpeculative);
           assembler()->LoadUnboxedSimd128(
               fpu_reg, reg,
               compiler::target::Float32x4::value_offset() - kHeapObjectTag);

runtime/vm/compiler/backend/flow_graph_test.cc

@@ -46,18 +46,18 @@ ISOLATE_UNIT_TEST_CASE(FlowGraph_UnboxInt64Phi) {
     BlockBuilder builder(H.flow_graph(), loop_header);
     loop_var = H.Phi(loop_header, {{normal_entry, v0}, {loop_body, &add1}});
     builder.AddPhi(loop_var);
-    builder.AddBranch(new RelationalOpInstr(
-                          InstructionSource(), Token::kLT, new Value(loop_var),
-                          new Value(H.IntConstant(50)), kMintCid,
-                          S.GetNextDeoptId(), Instruction::kNotSpeculative),
-                      loop_body, loop_exit);
+    builder.AddBranch(
+        new RelationalOpInstr(InstructionSource(), Token::kLT,
+                              new Value(loop_var), new Value(H.IntConstant(50)),
+                              kMintCid, S.GetNextDeoptId()),
+        loop_body, loop_exit);
   }
 
   {
     BlockBuilder builder(H.flow_graph(), loop_body);
     add1 = builder.AddDefinition(new BinaryInt64OpInstr(
         Token::kADD, new Value(loop_var), new Value(H.IntConstant(1)),
-        S.GetNextDeoptId(), Instruction::kNotSpeculative));
+        S.GetNextDeoptId()));
     builder.AddInstruction(new GotoInstr(loop_header, S.GetNextDeoptId()));
   }
 
@@ -109,18 +109,18 @@ ISOLATE_UNIT_TEST_CASE(FlowGraph_LateVariablePhiUnboxing) {
     late_var =
         H.Phi(loop_header, {{normal_entry, sentinel}, {loop_body, &add1}});
     builder.AddPhi(late_var);
-    builder.AddBranch(new RelationalOpInstr(
-                          InstructionSource(), Token::kLT, new Value(loop_var),
-                          new Value(H.IntConstant(10)), kMintCid,
-                          S.GetNextDeoptId(), Instruction::kNotSpeculative),
-                      loop_body, loop_exit);
+    builder.AddBranch(
+        new RelationalOpInstr(InstructionSource(), Token::kLT,
+                              new Value(loop_var), new Value(H.IntConstant(10)),
+                              kMintCid, S.GetNextDeoptId()),
+        loop_body, loop_exit);
   }
 
   {
     BlockBuilder builder(H.flow_graph(), loop_body);
     add1 = builder.AddDefinition(new BinaryInt64OpInstr(
         Token::kADD, new Value(loop_var), new Value(H.IntConstant(1)),
-        S.GetNextDeoptId(), Instruction::kNotSpeculative));
+        S.GetNextDeoptId()));
     builder.AddInstruction(new GotoInstr(loop_header, S.GetNextDeoptId()));
   }
 
@@ -168,16 +168,14 @@ ISOLATE_UNIT_TEST_CASE(FlowGraph_UnboxedFloatPhi) {
   {
     BlockBuilder builder(H.flow_graph(), then_body);
     double_to_float_1 = builder.AddDefinition(new DoubleToFloatInstr(
-        new Value(H.DoubleConstant(1)), S.GetNextDeoptId(),
-        Instruction::kNotSpeculative));
+        new Value(H.DoubleConstant(1)), S.GetNextDeoptId()));
     builder.AddInstruction(new GotoInstr(join_exit, S.GetNextDeoptId()));
   }
 
   {
     BlockBuilder builder(H.flow_graph(), else_body);
     double_to_float_2 = builder.AddDefinition(new DoubleToFloatInstr(
-        new Value(H.DoubleConstant(2)), S.GetNextDeoptId(),
-        Instruction::kNotSpeculative));
+        new Value(H.DoubleConstant(2)), S.GetNextDeoptId()));
     builder.AddInstruction(new GotoInstr(join_exit, S.GetNextDeoptId()));
   }