[turbofan] Don't overwrite jump target serialization environment

A given target offset may already have an environment associated with
it (there can be multiple jumps to the same target). In that case we
used to throw away the previous environment. With this CL we merge the
environments instead.

Bug: v8:7790
Change-Id: I0c22182436fc48e29675e49627729a33cbeaaf4d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1631603
Commit-Queue: Georg Neis <neis@chromium.org>
Auto-Submit: Georg Neis <neis@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61905}

src/compiler/serializer-for-background-compilation.cc

@@ -106,10 +106,7 @@ class SerializerForBackgroundCompilation::Environment : public ZoneObject {
     DCHECK(!IsDead());
   }
 
-  // When control flow bytecodes are encountered, e.g. a conditional jump,
-  // the current environment needs to be stashed together with the target jump
-  // address. Later, when this target bytecode is handled, the stashed
-  // environment will be merged into the current one.
+  // Merge {other} into {this} environment (leaving {other} unmodified).
   void Merge(Environment* other);
 
   FunctionBlueprint function() const { return function_; }
@@ -297,7 +294,7 @@ SerializerForBackgroundCompilation::SerializerForBackgroundCompilation(
       dependencies_(dependencies),
       zone_(zone),
       environment_(new (zone) Environment(zone, {closure, broker_->isolate()})),
-      stashed_environments_(zone),
+      jump_target_environments_(zone),
       flags_(flags) {
   JSFunctionRef(broker, closure).Serialize();
 }
@@ -311,7 +308,7 @@ SerializerForBackgroundCompilation::SerializerForBackgroundCompilation(
       zone_(zone),
       environment_(new (zone) Environment(zone, broker_->isolate(), function,
                                           new_target, arguments)),
-      stashed_environments_(zone),
+      jump_target_environments_(zone),
       flags_(flags) {
   DCHECK(!(flags_ & SerializerForBackgroundCompilationFlag::kOsr));
   TraceScope tracer(
@@ -415,7 +412,7 @@ void SerializerForBackgroundCompilation::TraverseBytecode() {
   ExceptionHandlerMatcher handler_matcher(iterator);
 
   for (; !iterator.done(); iterator.Advance()) {
-    MergeAfterJump(&iterator);
+    IncorporateJumpTargetEnvironment(iterator.current_offset());
 
     if (environment()->IsDead()) {
       if (iterator.current_bytecode() ==
@@ -773,22 +770,31 @@ void SerializerForBackgroundCompilation::ProcessCallVarArgs(
   ProcessCallOrConstruct(callee, base::nullopt, arguments, slot);
 }
 
-void SerializerForBackgroundCompilation::ProcessJump(
-    interpreter::BytecodeArrayIterator* iterator) {
-  int jump_target = iterator->GetJumpTargetOffset();
-  int current_offset = iterator->current_offset();
-  if (current_offset >= jump_target) return;
+void SerializerForBackgroundCompilation::ContributeToJumpTargetEnvironment(
+    int target_offset) {
+  auto it = jump_target_environments_.find(target_offset);
+  if (it == jump_target_environments_.end()) {
+    jump_target_environments_[target_offset] =
+        new (zone()) Environment(*environment());
+  } else {
+    it->second->Merge(environment());
+  }
+}
 
-  stashed_environments_[jump_target] = new (zone()) Environment(*environment());
+void SerializerForBackgroundCompilation::IncorporateJumpTargetEnvironment(
+    int target_offset) {
+  auto it = jump_target_environments_.find(target_offset);
+  if (it != jump_target_environments_.end()) {
+    environment()->Merge(it->second);
+    jump_target_environments_.erase(it);
+  }
 }
 
-void SerializerForBackgroundCompilation::MergeAfterJump(
+void SerializerForBackgroundCompilation::ProcessJump(
     interpreter::BytecodeArrayIterator* iterator) {
-  int current_offset = iterator->current_offset();
-  auto stash = stashed_environments_.find(current_offset);
-  if (stash != stashed_environments_.end()) {
-    environment()->Merge(stash->second);
-    stashed_environments_.erase(stash);
+  int jump_target = iterator->GetJumpTargetOffset();
+  if (iterator->current_offset() < jump_target) {
+    ContributeToJumpTargetEnvironment(jump_target);
   }
 }
 
@@ -803,9 +809,7 @@ void SerializerForBackgroundCompilation::VisitSwitchOnSmiNoFeedback(
   interpreter::JumpTableTargetOffsets targets =
       iterator->GetJumpTableTargetOffsets();
   for (const auto& target : targets) {
-    // TODO(neis): Here and in ProcessJump, don't overwrite stashed environment.
-    stashed_environments_[target.target_offset] =
-        new (zone()) Environment(*environment());
+    ContributeToJumpTargetEnvironment(target.target_offset);
   }
 }
 

src/compiler/serializer-for-background-compilation.h

@@ -317,7 +317,6 @@ class SerializerForBackgroundCompilation {
                           bool with_spread = false);
 
   void ProcessJump(interpreter::BytecodeArrayIterator* iterator);
-  void MergeAfterJump(interpreter::BytecodeArrayIterator* iterator);
 
   void ProcessKeyedPropertyAccess(Hints const& receiver, Hints const& key,
                                   FeedbackSlot slot, AccessMode mode);
@@ -339,6 +338,16 @@ class SerializerForBackgroundCompilation {
                            base::Optional<Hints> new_target,
                            const HintsVector& arguments, bool with_spread);
 
+  // When (forward-)branching bytecodes are encountered, e.g. a conditional
+  // jump, we call ContributeToJumpTargetEnvironment to "remember" the current
+  // environment, associated with the jump target offset. When serialization
+  // eventually reaches that offset, we call IncorporateJumpTargetEnvironment to
+  // merge that environment back into whatever is the current environment then.
+  // Note: Since there may be multiple jumps to the same target,
+  // ContributeToJumpTargetEnvironment may actually do a merge as well.
+  void ContributeToJumpTargetEnvironment(int target_offset);
+  void IncorporateJumpTargetEnvironment(int target_offset);
+
   JSHeapBroker* broker() const { return broker_; }
   CompilationDependencies* dependencies() const { return dependencies_; }
   Zone* zone() const { return zone_; }
@@ -349,7 +358,7 @@ class SerializerForBackgroundCompilation {
   CompilationDependencies* const dependencies_;
   Zone* const zone_;
   Environment* const environment_;
-  ZoneUnorderedMap<int, Environment*> stashed_environments_;
+  ZoneUnorderedMap<int, Environment*> jump_target_environments_;
   SerializerForBackgroundCompilationFlags const flags_;
 };
 

test/cctest/compiler/serializer-tester.cc

@@ -274,6 +274,20 @@ TEST(SerializeUnconditionalJump) {
       "f(); return f;");
 }
 
+TEST(MergeJumpTargetEnvironment) {
+  CheckForSerializedInlinee(
+      "function f() {"
+      "  let g;"
+      "  while (true) {"
+      "    if (g === undefined) {g = ()=>1; break;} else {g = ()=>2; break};"
+      "  };"
+      "  g(); return g;"
+      "};"
+      "%EnsureFeedbackVectorForFunction(f);"
+      "%EnsureFeedbackVectorForFunction(f());"
+      "f(); return f;");  // Two calls to f to make g() megamorhpic.
+}
+
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8