[turbofan] Make OSR and stack slots compatible

Bug: chromium:1130844, v8:10973
Change-Id: I912f2cf6cedaf22dd50d456622880ea266b65dcd
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2445509
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70323}

src/compiler/effect-control-linearizer.cc

@@ -5170,6 +5170,11 @@ Node* EffectControlLinearizer::LowerFastApiCall(Node* node) {
   CHECK_EQ(FastApiCallNode::ArityForArgc(c_arg_count, js_arg_count),
            value_input_count);
 
+  // TODO(mslekova): There's an opportunity to optimize the allocation
+  // of the stack slot for all fast calls by reusing the same slot.
+  // If this is implemented, please update the FastApiStackSlot in
+  // cctest/test-api.cc, which will probably become meaningless, as it relies
+  // on two separate stack slots being allocated.
   // Add the { has_error } output parameter.
   int kAlign = 4;
   int kSize = 4;

src/compiler/pipeline.cc

@@ -486,6 +486,7 @@ class PipelineData {
           call_descriptor->CalculateFixedFrameSize(info()->code_kind());
     }
     frame_ = codegen_zone()->New<Frame>(fixed_frame_size);
+    if (osr_helper_.has_value()) osr_helper()->SetupFrame(frame());
   }
 
   void InitializeTopTierRegisterAllocationData(
@@ -2578,6 +2579,8 @@ bool PipelineImpl::OptimizeGraph(Linkage* linkage) {
 
   data->BeginPhaseKind("V8.TFBlockBuilding");
 
+  data->InitializeFrameData(linkage->GetIncomingDescriptor());
+
   // Run early optimization pass.
   Run<EarlyOptimizationPhase>();
   RunPrintAndVerify(EarlyOptimizationPhase::phase_name(), true);
@@ -2671,6 +2674,8 @@ bool PipelineImpl::OptimizeGraphForMidTier(Linkage* linkage) {
 
   data->BeginPhaseKind("V8.TFBlockBuilding");
 
+  data->InitializeFrameData(linkage->GetIncomingDescriptor());
+
   ComputeScheduledGraph();
 
   Run<ScheduledEffectControlLinearizationPhase>();
@@ -3310,7 +3315,11 @@ bool PipelineImpl::SelectInstructions(Linkage* linkage) {
 
   data->InitializeInstructionSequence(call_descriptor);
 
-  data->InitializeFrameData(call_descriptor);
+  // Depending on which code path led us to this function, the frame may or
+  // may not have been initialized. If it hasn't yet, initialize it now.
+  if (!data->frame()) {
+    data->InitializeFrameData(call_descriptor);
+  }
   // Select and schedule instructions covering the scheduled graph.
   Run<InstructionSelectionPhase>(linkage);
   if (data->compilation_failed()) {
@@ -3606,7 +3615,6 @@ void PipelineImpl::AllocateRegistersForTopTier(
     flags |= RegisterAllocationFlag::kTraceAllocation;
   }
   data->InitializeTopTierRegisterAllocationData(config, call_descriptor, flags);
-  if (info()->is_osr()) data->osr_helper()->SetupFrame(data->frame());
 
   Run<MeetRegisterConstraintsPhase>();
   Run<ResolvePhisPhase>();

test/BUILD.gn

@@ -152,6 +152,7 @@ v8_header_set("common_test_headers") {
 
   sources = [
     "common/assembler-tester.h",
+    "common/flag-utils.h",
     "common/types-fuzz.h",
     "common/wasm/flag-utils.h",
     "common/wasm/test-signatures.h",

test/cctest/BUILD.gn

@@ -72,6 +72,7 @@ v8_source_set("cctest_sources") {
   sources = [
     ### gcmole(all) ###
     "../common/assembler-tester.h",
+    "../common/flag-utils.h",
     "../common/wasm/flag-utils.h",
     "../common/wasm/test-signatures.h",
     "../common/wasm/wasm-macro-gen.h",

test/cctest/test-api.cc

@@ -70,6 +70,7 @@
 #include "test/cctest/heap/heap-tester.h"
 #include "test/cctest/heap/heap-utils.h"
 #include "test/cctest/wasm/wasm-run-utils.h"
+#include "test/common/flag-utils.h"
 #include "test/common/wasm/wasm-macro-gen.h"
 
 static const bool kLogThreading = false;
@@ -27557,11 +27558,26 @@ T* GetInternalField(v8::Object* wrapper) {
       wrapper->GetAlignedPointerFromInternalField(offset));
 }
 
+enum class Behavior {
+  kNoException,
+  kException,  // An exception should be thrown by the callback function.
+};
+
+enum class FallbackPolicy {
+  kDontRequestFallback,
+  kRequestFallback,  // The callback function should write a non-zero value
+                     // to the fallback variable.
+};
+
 template <typename T>
 struct ApiNumberChecker : BasicApiChecker<T, ApiNumberChecker<T>> {
-  explicit ApiNumberChecker(T value, bool raise_exception = false,
-                            int args_count = 1)
-      : raise_exception_(raise_exception), args_count_(args_count) {}
+  explicit ApiNumberChecker(
+      T value, Behavior raise_exception = Behavior::kNoException,
+      FallbackPolicy write_to_fallback = FallbackPolicy::kDontRequestFallback,
+      int args_count = 1)
+      : raise_exception_(raise_exception),
+        write_to_fallback_(write_to_fallback),
+        args_count_(args_count) {}
 
   static void FastCallback(v8::ApiObject receiver, T argument, int* fallback) {
     v8::Object* receiver_obj = reinterpret_cast<v8::Object*>(&receiver);
@@ -27574,7 +27590,11 @@ struct ApiNumberChecker : BasicApiChecker<T, ApiNumberChecker<T>> {
             receiver_obj);
     receiver_ptr->result_ |= ApiCheckerResult::kFastCalled;
     receiver_ptr->fast_value_ = argument;
-    if (receiver_ptr->raise_exception_) {
+    if (receiver_ptr->write_to_fallback_ == FallbackPolicy::kRequestFallback) {
+      // Anything != 0 has the same effect here, but we're writing 1 to match
+      // the default behavior expected from the embedder. The value is checked
+      // against after loading it from a stack slot, as defined in
+      // EffectControlLinearizer::LowerFastApiCall.
       *fallback = 1;
     }
   }
@@ -27594,14 +27614,16 @@ struct ApiNumberChecker : BasicApiChecker<T, ApiNumberChecker<T>> {
     LocalContext env;
     checker->slow_value_ = ConvertJSValue<T>::Get(info[0], env.local());
 
-    if (checker->raise_exception_) {
+    if (checker->raise_exception_ == Behavior::kException) {
+      CHECK(checker->write_to_fallback_ == FallbackPolicy::kRequestFallback);
       info.GetIsolate()->ThrowException(v8_str("Callback error"));
     }
   }
 
   T fast_value_ = T();
   Maybe<T> slow_value_ = v8::Nothing<T>();
-  bool raise_exception_ = false;
+  Behavior raise_exception_ = Behavior::kNoException;
+  FallbackPolicy write_to_fallback_ = FallbackPolicy::kDontRequestFallback;
   int args_count_ = 1;
 };
 
@@ -27634,11 +27656,6 @@ struct UnexpectedObjectChecker
   }
 };
 
-enum class Behavior {
-  kNoException,
-  kException,  // An exception should be thrown by the callback function.
-};
-
 template <typename Value, typename Impl>
 bool SetupTest(v8::Local<v8::Value> initial_value, LocalContext* env,
                BasicApiChecker<Value, Impl>* checker, const char* source_code,
@@ -27723,9 +27740,15 @@ template <typename T>
 void CallAndCheck(T expected_value, Behavior expected_behavior,
                   ApiCheckerResultFlags expected_path,
                   v8::Local<v8::Value> initial_value,
-                  bool raise_exception = false) {
+                  Behavior raise_exception = Behavior::kNoException) {
   LocalContext env;
-  ApiNumberChecker<T> checker(expected_value, raise_exception);
+  // The default embedder behaviour in case of exception should be
+  // to write to the fallback variable.
+  FallbackPolicy write_to_fallback = (raise_exception == Behavior::kException)
+                                         ? FallbackPolicy::kRequestFallback
+                                         : FallbackPolicy::kDontRequestFallback;
+  ApiNumberChecker<T> checker(expected_value, raise_exception,
+                              write_to_fallback);
 
   bool has_caught = SetupTest<T, ApiNumberChecker<T>>(
       initial_value, &env, &checker,
@@ -27783,7 +27806,7 @@ void CallAndDeopt() {
 void CallNoFallback(int32_t expected_value) {
   LocalContext env;
   v8::Local<v8::Value> initial_value(v8_num(42));
-  ApiNumberChecker<int32_t> checker(expected_value, false);
+  ApiNumberChecker<int32_t> checker(expected_value, Behavior::kNoException);
   SetupTest(initial_value, &env, &checker,
             "function func(arg) { return receiver.api_func(arg); }"
             "%PrepareFunctionForOptimization(func);"
@@ -27799,7 +27822,7 @@ void CallNoFallback(int32_t expected_value) {
 void CallNoConvertReceiver(int32_t expected_value) {
   LocalContext env;
   v8::Local<v8::Value> initial_value(v8_num(42));
-  ApiNumberChecker<int32_t> checker(expected_value, false);
+  ApiNumberChecker<int32_t> checker(expected_value, Behavior::kNoException);
   SetupTest(initial_value, &env, &checker,
             "function func(arg) { return receiver.api_func(arg); }"
             "%PrepareFunctionForOptimization(func);"
@@ -27815,7 +27838,8 @@ void CallNoConvertReceiver(int32_t expected_value) {
 void CallWithLessArguments() {
   LocalContext env;
   v8::Local<v8::Value> initial_value(v8_num(42));
-  ApiNumberChecker<int32_t> checker(42, false, 0);
+  ApiNumberChecker<int32_t> checker(42, Behavior::kNoException,
+                                    FallbackPolicy::kDontRequestFallback, 0);
   SetupTest(initial_value, &env, &checker,
             "function func() { return receiver.api_func(); }"
             "%PrepareFunctionForOptimization(func);"
@@ -27830,7 +27854,8 @@ void CallWithLessArguments() {
 void CallWithMoreArguments() {
   LocalContext env;
   v8::Local<v8::Value> initial_value(v8_num(42));
-  ApiNumberChecker<int32_t> checker(42, false, 2);
+  ApiNumberChecker<int32_t> checker(42, Behavior::kNoException,
+                                    FallbackPolicy::kDontRequestFallback, 2);
   SetupTest(initial_value, &env, &checker,
             "function func(arg) { receiver.api_func(arg, arg); }"
             "%PrepareFunctionForOptimization(func);"
@@ -27910,17 +27935,69 @@ void CheckDynamicTypeInfo() {
 }  // namespace
 #endif  // V8_LITE_MODE
 
+TEST(FastApiStackSlot) {
+#ifndef V8_LITE_MODE
+  if (i::FLAG_jitless) return;
+  if (i::FLAG_turboprop) return;
+
+  FLAG_SCOPE_EXTERNAL(opt);
+  FLAG_SCOPE_EXTERNAL(turbo_fast_api_calls);
+  FLAG_SCOPE_EXTERNAL(allow_natives_syntax);
+  // Disable --always_opt, otherwise we haven't generated the necessary
+  // feedback to go down the "best optimization" path for the fast call.
+  UNFLAG_SCOPE_EXTERNAL(always_opt);
+
+  v8::Isolate* isolate = CcTest::isolate();
+  i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  i_isolate->set_embedder_wrapper_type_index(kV8WrapperTypeIndex);
+  i_isolate->set_embedder_wrapper_object_index(kV8WrapperObjectIndex);
+
+  v8::HandleScope scope(isolate);
+  LocalContext env;
+
+  int test_value = 42;
+  ApiNumberChecker<int32_t> checker(test_value, Behavior::kNoException,
+                                    FallbackPolicy::kRequestFallback);
+
+  bool has_caught = SetupTest<int32_t, ApiNumberChecker<int32_t>>(
+      v8_num(test_value), &env, &checker,
+      "function func(arg) {"
+      " let foo = 128;"
+      " for (let i = 0; i < 100; ++i) {"
+      "  let bar = true;"
+      "  if (i == 10) %OptimizeOsr();"
+      "  try { receiver.api_func(arg) } catch(_) {};"
+      "  try { receiver.api_func(arg) } catch(_) {};"
+      " };"
+      " return foo;"
+      "};");
+  checker.result_ = ApiCheckerResult::kNotCalled;
+
+  v8::TryCatch try_catch(isolate);
+  v8::Local<v8::Value> foo =
+      CompileRun("%PrepareFunctionForOptimization(func); func(value);");
+  CHECK(foo->IsNumber());
+  CHECK_EQ(128, foo->ToInt32(env.local()).ToLocalChecked()->Value());
+
+  CHECK(checker.DidCallFast() && checker.DidCallSlow());
+  CHECK_EQ(false, has_caught);
+  int32_t slow_value_typed = checker.slow_value_.ToChecked();
+  CHECK_EQ(slow_value_typed, test_value);
+  CHECK_EQ(checker.fast_value_, test_value);
+#endif
+}
+
 TEST(FastApiCalls) {
 #ifndef V8_LITE_MODE
   if (i::FLAG_jitless) return;
   if (i::FLAG_turboprop) return;
 
-  i::FLAG_turbo_fast_api_calls = true;
-  i::FLAG_opt = true;
-  i::FLAG_allow_natives_syntax = true;
+  FLAG_SCOPE_EXTERNAL(opt);
+  FLAG_SCOPE_EXTERNAL(turbo_fast_api_calls);
+  FLAG_SCOPE_EXTERNAL(allow_natives_syntax);
   // Disable --always_opt, otherwise we haven't generated the necessary
   // feedback to go down the "best optimization" path for the fast call.
-  i::FLAG_always_opt = false;
+  UNFLAG_SCOPE_EXTERNAL(always_opt);
 
   v8::Isolate* isolate = CcTest::isolate();
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
@@ -28285,11 +28362,12 @@ TEST(FastApiCalls) {
   CallAndCheck<int32_t>(
       42, Behavior::kException,
       ApiCheckerResult::kFastCalled | ApiCheckerResult::kSlowCalled, v8_num(42),
-      true);
+      Behavior::kException);
 
   // Doesn't fallback to slow call
   CallAndCheck<int32_t>(42, Behavior::kNoException,
-                        ApiCheckerResult::kFastCalled, v8_num(42), false);
+                        ApiCheckerResult::kFastCalled, v8_num(42),
+                        Behavior::kNoException);
 
   // Wrong number of arguments
   CallWithLessArguments();

test/cctest/test-regexp.cc

@@ -50,7 +50,7 @@
 #include "src/utils/ostreams.h"
 #include "src/zone/zone-list-inl.h"
 #include "test/cctest/cctest.h"
-#include "test/common/wasm/flag-utils.h"
+#include "test/common/flag-utils.h"
 
 namespace v8 {
 namespace internal {

test/common/flag-utils.h

+// Copyright 2020 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.
+
+#ifndef V8_TEST_COMMON_FLAG_UTILS_H
+#define V8_TEST_COMMON_FLAG_UTILS_H
+
+namespace v8 {
+namespace internal {
+
+template <typename T>
+class FlagScope {
+ public:
+  FlagScope(T* flag, T new_value) : flag_(flag), previous_value_(*flag) {
+    *flag = new_value;
+  }
+  ~FlagScope() { *flag_ = previous_value_; }
+
+ private:
+  T* flag_;
+  T previous_value_;
+};
+
+#define FLAG_SCOPE(flag) \
+  FlagScope<bool> __scope_##flag##__LINE__(&FLAG_##flag, true)
+
+}  // namespace internal
+}  // namespace v8
+
+#define FLAG_SCOPE_EXTERNAL(flag)                         \
+  v8::internal::FlagScope<bool> __scope_##flag##__LINE__( \
+      &v8::internal::FLAG_##flag, true)
+
+#define UNFLAG_SCOPE_EXTERNAL(flag)                       \
+  v8::internal::FlagScope<bool> __scope_##flag##__LINE__( \
+      &v8::internal::FLAG_##flag, false)
+
+#endif  // V8_TEST_COMMON_FLAG_UTILS_H

test/common/wasm/flag-utils.h

@@ -2,30 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef V8_TEST_COMMON_FLAG_UTILS_H
-#define V8_TEST_COMMON_FLAG_UTILS_H
+#ifndef V8_TEST_COMMON_WASM_FLAG_UTILS_H
+#define V8_TEST_COMMON_WASM_FLAG_UTILS_H
 
 #include "src/wasm/wasm-features.h"
+#include "test/common/flag-utils.h"
 
 namespace v8 {
 namespace internal {
 
-template <typename T>
-class FlagScope {
- public:
-  FlagScope(T* flag, T new_value) : flag_(flag), previous_value_(*flag) {
-    *flag = new_value;
-  }
-  ~FlagScope() { *flag_ = previous_value_; }
-
- private:
-  T* flag_;
-  T previous_value_;
-};
-
-#define FLAG_SCOPE(flag) \
-  FlagScope<bool> __scope_##flag##__LINE__(&FLAG_##flag, true)
-
 #define EXPERIMENTAL_FLAG_SCOPE(flag) FLAG_SCOPE(experimental_wasm_##flag)
 
 namespace wasm {
@@ -65,4 +50,4 @@ class WasmFeatureScope {
 }  // namespace internal
 }  // namespace v8
 
-#endif  // V8_TEST_COMMON_FLAG_UTILS_H
+#endif  // V8_TEST_COMMON_WASM_FLAG_UTILS_H