[turbofan] Fix a deopt loop

... by disallowing checkpoint elimination across function boundaries.
See the comment in checkpoint-elimination.cc and the tests for details.

Bug: v8:9945
Change-Id: Ibf4ab6f0e4e709e26d3c4428a082ef45dcbeb8b0
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1906208
Auto-Submit: Georg Neis <neis@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65027}

src/compiler/checkpoint-elimination.cc

@@ -4,6 +4,7 @@
 
 #include "src/compiler/checkpoint-elimination.h"
 
+#include "src/compiler/common-operator.h"
 #include "src/compiler/node-properties.h"
 
 namespace v8 {
@@ -15,14 +16,31 @@ CheckpointElimination::CheckpointElimination(Editor* editor)
 
 namespace {
 
+FrameStateFunctionInfo const* GetFunctionInfo(Node* checkpoint) {
+  DCHECK_EQ(IrOpcode::kCheckpoint, checkpoint->opcode());
+  Node* frame_state = NodeProperties::GetFrameStateInput(checkpoint);
+  return FrameStateInfoOf(frame_state->op()).function_info();
+}
+
 // The given checkpoint is redundant if it is effect-wise dominated by another
-// checkpoint and there is no observable write in between. For now we consider
-// a linear effect chain only instead of true effect-wise dominance.
+// checkpoint of the same origin (*) and there is no observable write in
+// between. For now we consider a linear effect chain only instead of true
+// effect-wise dominance.
+// "Same origin" here refers to the same graph building pass and is expressed as
+// the identity of the checkpoint's FrameStateFunctionInfo pointer. This
+// restriction ensures that an eager deopt from an inlined function will resume
+// the inlined function's bytecode (rather than, say, the call in the caller's
+// bytecode), which in turn is necessary to ensure that we learn something from
+// the deopt in the case where an optimized code object for the inlined function
+// exists. See regress-9945-*.js and v8:9945.
 bool IsRedundantCheckpoint(Node* node) {
+  FrameStateFunctionInfo const* function_info = GetFunctionInfo(node);
   Node* effect = NodeProperties::GetEffectInput(node);
   while (effect->op()->HasProperty(Operator::kNoWrite) &&
          effect->op()->EffectInputCount() == 1) {
-    if (effect->opcode() == IrOpcode::kCheckpoint) return true;
+    if (effect->opcode() == IrOpcode::kCheckpoint) {
+      return GetFunctionInfo(effect) == function_info;
+    }
     effect = NodeProperties::GetEffectInput(effect);
   }
   return false;

test/mjsunit/compiler/number-comparison-truncations.js

@@ -31,13 +31,8 @@
 // kIdentifyZeros truncation.
 (function() {
   // Produce a SpeculativeNumberEqual with Number feedback.
-  function bar(x, y) { return x === y; }
-  %EnsureFeedbackVectorForFunction(bar);
-  bar(0.1, 0.5);
-  bar(-0, 100);
-
   function foo(x, y) {
-    if (bar(x * y, 0)) return 0;
+    if (x * y === -0) return 0;
     return 1;
   }
 
@@ -83,13 +78,8 @@
 // kIdentifyZeros truncation.
 (function() {
   // Produce a SpeculativeNumberLessThan with Number feedback.
-  function bar(x, y) { return x < y; }
-  %EnsureFeedbackVectorForFunction(bar);
-  bar(0.1, 0.5);
-  bar(-0, 100);
-
   function foo(x, y) {
-    if (bar(x * y, 0)) return 0;
+    if (x * y < -0) return 0;
     return 1;
   }
 
@@ -135,13 +125,8 @@
 // kIdentifyZeros truncation.
 (function() {
   // Produce a SpeculativeNumberLessThanOrEqual with Number feedback.
-  function bar(x, y) { return x <= y; }
-  %EnsureFeedbackVectorForFunction(bar);
-  bar(0.1, 0.5);
-  bar(-0, 100);
-
   function foo(x, y) {
-    if (bar(x * y, 0)) return 0;
+    if (x * y <= -0) return 0;
     return 1;
   }
 

test/mjsunit/compiler/regress-9945-1.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax --opt --no-always-opt
+// Flags: --concurrent-recompilation --block-concurrent-recompilation
+
+function foo(x) { bar(x) }
+function bar(x) { x.p }
+
+%PrepareFunctionForOptimization(foo);
+%PrepareFunctionForOptimization(bar);
+
+// Create map transitions such that a's final map is not stable.
+var dummy = [];
+dummy.p = 0;
+dummy.q = 0;
+
+var a = [];
+a.p = 42;
+
+var b = [];
+b.p = 42;
+
+// Warm-up.
+foo(a);
+foo(a);
+
+// Trigger optimization of bar but don't yet complete it.
+%OptimizeFunctionOnNextCall(bar, "concurrent");
+foo(a);
+%PrepareFunctionForOptimization(bar);
+
+// Change a's map from PACKED_SMI_ELEMENTS to PACKED_ELEMENTS and run bar in the
+// interpreter (via foo) s.t. bar's load feedback changes accordingly.
+a[0] = {};
+foo(a);
+assertUnoptimized(bar, "no sync");
+
+// Now finish the optimization of bar, which was based on the old
+// PACKED_SMI_ELEMENTS feedback.
+%UnblockConcurrentRecompilation();
+assertOptimized(bar);
+// If we were to call the optimized bar now, it would deopt.
+
+// Instead we trigger optimization of foo, which will inline bar (this time
+// based on the new PACKED_ELEMENTS map.
+%OptimizeFunctionOnNextCall(foo);
+foo(a);
+assertOptimized(foo);
+%PrepareFunctionForOptimization(foo);
+assertOptimized(bar);
+
+// Now call the optimized foo on an object that has the old PACKED_SMI_ELEMENTS
+// map. This will lead to an eager deopt of foo when the inlined bar sees that
+// old map.
+foo(b);
+assertUnoptimized(foo);
+assertOptimized(bar);
+
+// Now ensure there is no deopt-loop. There used to be a deopt-loop because, as
+// a result of over-eager checkpoint elimination, we used to deopt into foo
+// (right before the call to bar) rather than into bar (right before the load).
+%OptimizeFunctionOnNextCall(foo);
+foo(b);
+assertOptimized(foo);

test/mjsunit/compiler/regress-9945-2.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Flags: --allow-natives-syntax --opt --no-always-opt
+
+////////////////////////////////////////////////////////////////////////////////
+// This is a variant of regress-99540-1.js that does not rely on concurrent
+// recompilation.
+////////////////////////////////////////////////////////////////////////////////
+
+function mkbar() { return function(x) { x.p } }
+var bar = mkbar();
+function foo(x) { bar(x) }
+
+%PrepareFunctionForOptimization(foo);
+%PrepareFunctionForOptimization(bar);
+
+// Create map transitions such that a's final map is not stable.
+var dummy = [];
+dummy.p = 0;
+dummy.q = 0;
+
+var a = [];
+a.p = 42;
+
+var b = [];
+b.p = 42;
+
+// Warm-up.
+foo(a);
+foo(a);
+
+// Trigger optimization of bar, based on PACKED_SMI_ELEMENTS feedback.
+%OptimizeFunctionOnNextCall(bar);
+bar(a);
+assertOptimized(bar);
+%PrepareFunctionForOptimization(bar);
+
+// Change a's map from PACKED_SMI_ELEMENTS to PACKED_ELEMENTS and run new
+// instance of mkbar() in the interpreter s.t. bar's load feedback changes
+// accordingly (thanks to feedback vector sharing).
+a[0] = {};
+mkbar()(a);
+assertOptimized(bar);
+// If we were to call the optimized bar now, it would deopt.
+
+// Instead we trigger optimization of foo, which will inline bar (this time
+// based on the new PACKED_ELEMENTS map.
+assertOptimized(bar);
+%OptimizeFunctionOnNextCall(foo);
+assertOptimized(bar);
+foo(a);
+assertOptimized(bar);
+assertOptimized(foo);
+%PrepareFunctionForOptimization(foo);
+
+// Now call the optimized foo on an object that has the old PACKED_SMI_ELEMENTS
+// map. This will lead to an eager deopt of foo when the inlined bar sees that
+// old map.
+foo(b);
+assertUnoptimized(foo);
+assertOptimized(bar);
+
+// Now ensure there is no deopt-loop. There used to be a deopt-loop because, as
+// a result of over-eager checkpoint elimination, we used to deopt into foo
+// (right before the call to bar) rather than into bar (right before the load).
+%OptimizeFunctionOnNextCall(foo);
+foo(b);
+assertOptimized(foo);

test/mjsunit/mjsunit.status

@@ -1070,6 +1070,7 @@
   'compiler/native-context-specialization-hole-check': [SKIP],
   'compiler/number-comparison-truncations': [SKIP],
   'compiler/redundancy-elimination': [SKIP],
+  'compiler/regress-9945-*': [SKIP],
 
   # Static asserts for optimizations don't hold due to removed optimization
   # phases.