[maglev] Conservatively mark nodes with builtins calls as

.. Throw|LazyDeopt. Whether a builtin can Throw|LazyDeopt depends
on the implementation, so to be safe all builtin calls should be
marked as such - UNLESS we know for certain that one or the other
doesn't happen.

Drive-by: For calls with two result registers, properly consider
the second register in a few spots.

Bug: v8:7700
Change-Id: Icbcffb51e9760761a2f4e32d79af33abccb8f1cb
Fixed: chromium:1361245
Fixed: chromium:1360800
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3879617Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Auto-Submit: Jakob Linke <jgruber@chromium.org>
Commit-Queue: Jakob Linke <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83152}

src/maglev/maglev-compiler.cc

@@ -222,7 +222,7 @@ class UseMarkingProcessor {
     register_frame->ForEachValue(
         deopt_info->unit, [&](ValueNode* node, interpreter::Register reg) {
           // Skip over the result location.
-          if (reg == deopt_info->result_location) return;
+          if (deopt_info->IsResultRegister(reg)) return;
           MarkUse(node, use_id, &deopt_info->input_locations[index++],
                   loop_used_nodes);
         });

src/maglev/maglev-graph-builder.cc

@@ -2008,7 +2008,8 @@ void MaglevGraphBuilder::VisitCallRuntimeForPair() {
   for (int i = 0; i < args.register_count(); ++i) {
     call_runtime->set_arg(i, GetTaggedValue(args[i]));
   }
-  StoreRegisterPair(iterator_.GetRegisterOperand(3), call_runtime);
+  auto result = iterator_.GetRegisterPairOperand(3);
+  StoreRegisterPair(result, call_runtime);
 }
 
 void MaglevGraphBuilder::VisitInvokeIntrinsic() {
@@ -2324,21 +2325,19 @@ void MaglevGraphBuilder::VisitCreateArrayLiteral() {
   int bytecode_flags = GetFlag8Operand(2);
   int literal_flags =
       interpreter::CreateArrayLiteralFlags::FlagsBits::decode(bytecode_flags);
-  ValueNode* result;
   if (interpreter::CreateArrayLiteralFlags::FastCloneSupportedBit::decode(
           bytecode_flags)) {
     // TODO(victorgomes): CreateShallowArrayLiteral should not need the
     // boilerplate descriptor. However the current builtin checks that the
     // feedback exists and fallsback to CreateArrayLiteral if it doesn't.
-    result = AddNewNode<CreateShallowArrayLiteral>(
+    SetAccumulator(AddNewNode<CreateShallowArrayLiteral>(
         {}, constant_elements, compiler::FeedbackSource{feedback(), slot_index},
-        literal_flags);
+        literal_flags));
   } else {
-    result = AddNewNode<CreateArrayLiteral>(
+    SetAccumulator(AddNewNode<CreateArrayLiteral>(
         {}, constant_elements, compiler::FeedbackSource{feedback(), slot_index},
-        literal_flags);
+        literal_flags));
   }
-  SetAccumulator(result);
 }
 
 void MaglevGraphBuilder::VisitCreateArrayFromIterable() {
@@ -2361,21 +2360,19 @@ void MaglevGraphBuilder::VisitCreateObjectLiteral() {
   int bytecode_flags = GetFlag8Operand(2);
   int literal_flags =
       interpreter::CreateObjectLiteralFlags::FlagsBits::decode(bytecode_flags);
-  ValueNode* result;
   if (interpreter::CreateObjectLiteralFlags::FastCloneSupportedBit::decode(
           bytecode_flags)) {
     // TODO(victorgomes): CreateShallowObjectLiteral should not need the
     // boilerplate descriptor. However the current builtin checks that the
     // feedback exists and fallsback to CreateObjectLiteral if it doesn't.
-    result = AddNewNode<CreateShallowObjectLiteral>(
+    SetAccumulator(AddNewNode<CreateShallowObjectLiteral>(
         {}, boilerplate_desc, compiler::FeedbackSource{feedback(), slot_index},
-        literal_flags);
+        literal_flags));
   } else {
-    result = AddNewNode<CreateObjectLiteral>(
+    SetAccumulator(AddNewNode<CreateObjectLiteral>(
         {}, boilerplate_desc, compiler::FeedbackSource{feedback(), slot_index},
-        literal_flags);
+        literal_flags));
   }
-  SetAccumulator(result);
 }
 
 void MaglevGraphBuilder::VisitCreateEmptyObjectLiteral() {
@@ -2751,10 +2748,10 @@ void MaglevGraphBuilder::VisitForInPrepare() {
   // |cache_info_triple + 2|, with the registers holding cache_type,
   // cache_array, and cache_length respectively.
   interpreter::Register first = iterator_.GetRegisterOperand(0);
-  interpreter::Register second(first.index() + 1);
-  interpreter::Register third(first.index() + 2);
-  StoreRegister(second, result);
-  StoreRegister(third, GetSecondValue(result));
+  auto array_and_length =
+      std::make_pair(interpreter::Register{first.index() + 1},
+                     interpreter::Register{first.index() + 2});
+  StoreRegisterPair(array_and_length, result);
 }
 
 void MaglevGraphBuilder::VisitForInContinue() {

src/maglev/maglev-graph-builder.h

@@ -787,21 +787,27 @@ class MaglevGraphBuilder {
     if (!IsConstantNode(value->opcode())) {
       DCHECK_NE(0, new_nodes_.count(value));
     }
-    MarkAsLazyDeoptResult(value, target);
+    MarkAsLazyDeoptResult(value, target, 1);
     current_interpreter_frame_.set(target, value);
   }
 
-  void StoreRegisterPair(interpreter::Register target, CallRuntime* value) {
+  template <typename NodeT>
+  void StoreRegisterPair(
+      std::pair<interpreter::Register, interpreter::Register> target,
+      NodeT* value) {
+    const interpreter::Register target0 = target.first;
+    const interpreter::Register target1 = target.second;
+
+    DCHECK_EQ(interpreter::Register(target0.index() + 1), target1);
     DCHECK_EQ(value->ReturnCount(), 2);
 
     DCHECK_NE(0, new_nodes_.count(value));
-    MarkAsLazyDeoptResult(value, target, value->ReturnCount());
-    current_interpreter_frame_.set(target, value);
+    MarkAsLazyDeoptResult(value, target0, value->ReturnCount());
+    current_interpreter_frame_.set(target0, value);
 
     ValueNode* second_value = GetSecondValue(value);
     DCHECK_NE(0, new_nodes_.count(second_value));
-    current_interpreter_frame_.set(interpreter::Register(target.index() + 1),
-                                   second_value);
+    current_interpreter_frame_.set(target1, second_value);
   }
 
   CheckpointedInterpreterState GetLatestCheckpointedState() {
@@ -832,7 +838,7 @@ class MaglevGraphBuilder {
   template <typename NodeT>
   void MarkAsLazyDeoptResult(NodeT* value,
                              interpreter::Register result_location,
-                             int result_size = 1) {
+                             int result_size) {
     DCHECK_EQ(NodeT::kProperties.can_lazy_deopt(),
               value->properties().can_lazy_deopt());
     if constexpr (NodeT::kProperties.can_lazy_deopt()) {

src/maglev/maglev-graph-printer.cc

@@ -430,7 +430,7 @@ void PrintLazyDeopt(std::ostream& os, std::vector<BasicBlock*> targets,
           os << ", ";
         }
         os << reg.ToString() << ":";
-        if (reg == deopt_info->result_location) {
+        if (deopt_info->IsResultRegister(reg)) {
           os << "<result>";
         } else {
           os << PrintNodeLabel(graph_labeller, node) << ":"

src/maglev/maglev-ir.cc

@@ -379,6 +379,15 @@ DeoptInfo::DeoptInfo(Zone* zone, const MaglevCompilationUnit& compilation_unit,
   }
 }
 
+bool LazyDeoptInfo::IsResultRegister(interpreter::Register reg) const {
+  if (V8_LIKELY(result_size == 1)) {
+    return reg == result_location;
+  }
+  DCHECK_EQ(result_size, 2);
+  return reg == result_location ||
+         reg == interpreter::Register(result_location.index() + 1);
+}
+
 // ---
 // Nodes
 // ---
@@ -707,6 +716,7 @@ void ForInPrepare::GenerateCode(MaglevAssembler* masm,
           TaggedIndex::FromIntptr(feedback().index()));
   __ Move(D::GetRegisterParameter(D::kFeedbackVector), feedback().vector);
   __ CallBuiltin(Builtin::kForInPrepare);
+  masm->DefineExceptionHandlerAndLazyDeoptPoint(this);
 }
 
 void ForInNext::AllocateVreg(MaglevVregAllocationState* vreg_state) {
@@ -752,6 +762,7 @@ void GetIterator::GenerateCode(MaglevAssembler* masm,
           TaggedIndex::FromIntptr(call_slot()));
   __ Move(D::GetRegisterParameter(D::kMaybeFeedbackVector), feedback());
   __ CallBuiltin(Builtin::kGetIteratorWithFeedback);
+  masm->DefineExceptionHandlerAndLazyDeoptPoint(this);
 }
 
 void GetSecondReturnedValue::AllocateVreg(
@@ -905,6 +916,7 @@ void CreateEmptyArrayLiteral::GenerateCode(MaglevAssembler* masm,
   __ Move(D::GetRegisterParameter(D::kSlot), Smi::FromInt(feedback().index()));
   __ Move(D::GetRegisterParameter(D::kFeedbackVector), feedback().vector);
   __ CallBuiltin(Builtin::kCreateEmptyArrayLiteral);
+  masm->DefineExceptionHandlerAndLazyDeoptPoint(this);
 }
 
 void CreateArrayLiteral::AllocateVreg(MaglevVregAllocationState* vreg_state) {
@@ -935,6 +947,7 @@ void CreateShallowArrayLiteral::GenerateCode(MaglevAssembler* masm,
           constant_elements().object());
   __ Move(D::GetRegisterParameter(D::kFlags), Smi::FromInt(flags()));
   __ CallBuiltin(Builtin::kCreateShallowArrayLiteral);
+  masm->DefineExceptionHandlerAndLazyDeoptPoint(this);
 }
 
 void CreateObjectLiteral::AllocateVreg(MaglevVregAllocationState* vreg_state) {
@@ -988,6 +1001,7 @@ void CreateShallowObjectLiteral::GenerateCode(MaglevAssembler* masm,
   __ Move(D::GetRegisterParameter(D::kDesc), boilerplate_descriptor().object());
   __ Move(D::GetRegisterParameter(D::kFlags), Smi::FromInt(flags()));
   __ CallBuiltin(Builtin::kCreateShallowObjectLiteral);
+  masm->DefineExceptionHandlerAndLazyDeoptPoint(this);
 }
 
 void CreateFunctionContext::AllocateVreg(
@@ -1028,6 +1042,7 @@ void CreateFunctionContext::GenerateCode(MaglevAssembler* masm,
     __ Move(D::GetRegisterParameter(D::kSlots), Immediate(slot_count()));
     __ CallBuiltin(Builtin::kFastNewFunctionContextEval);
   }
+  masm->DefineExceptionHandlerAndLazyDeoptPoint(this);
 }
 void CreateFunctionContext::PrintParams(
     std::ostream& os, MaglevGraphLabeller* graph_labeller) const {
@@ -1049,6 +1064,7 @@ void FastCreateClosure::GenerateCode(MaglevAssembler* masm,
           shared_function_info().object());
   __ Move(D::GetRegisterParameter(D::kFeedbackCell), feedback_cell().object());
   __ CallBuiltin(Builtin::kFastNewClosure);
+  masm->DefineExceptionHandlerAndLazyDeoptPoint(this);
 }
 void FastCreateClosure::PrintParams(std::ostream& os,
                                     MaglevGraphLabeller* graph_labeller) const {
@@ -1107,6 +1123,7 @@ void GetTemplateObject::GenerateCode(MaglevAssembler* masm,
   __ Move(D::GetRegisterParameter(D::kSlot), feedback().slot.ToInt());
   __ Move(D::GetRegisterParameter(D::kShared), shared_function_info_.object());
   __ CallBuiltin(Builtin::kGetTemplateObject);
+  masm->DefineExceptionHandlerAndLazyDeoptPoint(this);
 }
 
 void Abort::AllocateVreg(MaglevVregAllocationState* vreg_state) {}

src/maglev/maglev-ir.h

@@ -525,6 +525,14 @@ class OpProperties {
   static constexpr OpProperties NeedsRegisterSnapshot() {
     return OpProperties(kNeedsRegisterSnapshotBit::encode(true));
   }
+  // Without auditing the call target, we must assume it can cause a lazy deopt
+  // and throw. Use this when codegen calls runtime or a builtin, unless
+  // certain that the target either doesn't throw or cannot deopt.
+  // TODO(jgruber): Go through all nodes marked with this property and decide
+  // whether to keep it (or remove either the lazy-deopt or throw flag).
+  static constexpr OpProperties GenericRuntimeOrBuiltinCall() {
+    return Call() | NonMemorySideEffects() | LazyDeopt() | Throw();
+  }
   static constexpr OpProperties JSCall() {
     return Call() | NonMemorySideEffects() | LazyDeopt() | Throw();
   }
@@ -680,6 +688,8 @@ class LazyDeoptInfo : public DeoptInfo {
                 CheckpointedInterpreterState checkpoint)
       : DeoptInfo(zone, compilation_unit, checkpoint) {}
 
+  bool IsResultRegister(interpreter::Register reg) const;
+
   int deopting_call_return_pc = -1;
   interpreter::Register result_location =
       interpreter::Register::invalid_value();
@@ -1983,13 +1993,16 @@ class ForInPrepare : public FixedInputValueNodeT<2, ForInPrepare> {
   explicit ForInPrepare(uint64_t bitfield, compiler::FeedbackSource& feedback)
       : Base(bitfield), feedback_(feedback) {}
 
-  static constexpr OpProperties kProperties = OpProperties::Call();
+  static constexpr OpProperties kProperties =
+      OpProperties::GenericRuntimeOrBuiltinCall();
 
   compiler::FeedbackSource feedback() const { return feedback_; }
 
   Input& context() { return Node::input(0); }
   Input& enumerator() { return Node::input(1); }
 
+  int ReturnCount() const { return 2; }
+
   DECL_NODE_INTERFACE_WITH_EMPTY_PRINT_PARAMS()
 
  private:
@@ -2221,7 +2234,8 @@ class CreateEmptyArrayLiteral
   compiler::FeedbackSource feedback() const { return feedback_; }
 
   // The implementation currently calls runtime.
-  static constexpr OpProperties kProperties = OpProperties::Call();
+  static constexpr OpProperties kProperties =
+      OpProperties::GenericRuntimeOrBuiltinCall();
 
   DECL_NODE_INTERFACE_WITH_EMPTY_PRINT_PARAMS()
 
@@ -2275,7 +2289,8 @@ class CreateShallowArrayLiteral
   int flags() const { return flags_; }
 
   // The implementation currently calls runtime.
-  static constexpr OpProperties kProperties = OpProperties::Call();
+  static constexpr OpProperties kProperties =
+      OpProperties::GenericRuntimeOrBuiltinCall();
 
   DECL_NODE_INTERFACE_WITH_EMPTY_PRINT_PARAMS()
 
@@ -2357,7 +2372,8 @@ class CreateShallowObjectLiteral
   int flags() const { return flags_; }
 
   // The implementation currently calls runtime.
-  static constexpr OpProperties kProperties = OpProperties::Call();
+  static constexpr OpProperties kProperties =
+      OpProperties::GenericRuntimeOrBuiltinCall();
 
   DECL_NODE_INTERFACE_WITH_EMPTY_PRINT_PARAMS()
 
@@ -2387,7 +2403,8 @@ class CreateFunctionContext
   Input& context() { return input(0); }
 
   // The implementation currently calls runtime.
-  static constexpr OpProperties kProperties = OpProperties::Call();
+  static constexpr OpProperties kProperties =
+      OpProperties::GenericRuntimeOrBuiltinCall();
 
   DECL_NODE_INTERFACE()
 
@@ -2416,7 +2433,8 @@ class FastCreateClosure : public FixedInputValueNodeT<1, FastCreateClosure> {
   Input& context() { return input(0); }
 
   // The implementation currently calls runtime.
-  static constexpr OpProperties kProperties = OpProperties::Call();
+  static constexpr OpProperties kProperties =
+      OpProperties::GenericRuntimeOrBuiltinCall();
 
   DECL_NODE_INTERFACE()
 
@@ -2682,7 +2700,8 @@ class GetTemplateObject : public FixedInputValueNodeT<1, GetTemplateObject> {
         feedback_(feedback) {}
 
   // The implementation currently calls runtime.
-  static constexpr OpProperties kProperties = OpProperties::Call();
+  static constexpr OpProperties kProperties =
+      OpProperties::GenericRuntimeOrBuiltinCall();
 
   Input& description() { return input(0); }
 
@@ -3328,7 +3347,7 @@ class CallRuntime : public ValueNodeT<CallRuntime> {
     set_input(i + kFixedInputCount, node);
   }
 
-  int ReturnCount() {
+  int ReturnCount() const {
     return Runtime::FunctionForId(function_id())->result_size;
   }
 

src/maglev/maglev-regalloc.cc

@@ -521,8 +521,9 @@ void StraightForwardRegisterAllocator::UpdateUse(
   // See also: UpdateUse(EagerDeoptInfo&).
   checkpoint_state->ForEachValue(
       deopt_info.unit, [&](ValueNode* node, interpreter::Register reg) {
-        // Skip over the result location.
-        if (reg == deopt_info.result_location) return;
+        // Skip over the result location since it is irrelevant for lazy deopts
+        // (unoptimized code will recreate the result).
+        if (deopt_info.IsResultRegister(reg)) return;
         if (FLAG_trace_maglev_regalloc) {
           printing_visitor_->os()
               << "- using " << PrintNodeLabel(graph_labeller(), node) << "\n";