Remove JIT implementation of RAISE_VARARGS

Summary:
There's no need to implement this in the JIT as we just deopt after executing it. Much neater to just deopt immediately and let the interpreter run its implementation of `RAISE_VARARGS`.

The motivation for doing this now is something is broken in 3.14 on deopt after `RAISE_VARARGS`. Something to do with the instruction pointer location is a problem in this situation where it wasn't before. Seems easier to just remove this whole situation than trying to work around this difference for 3.14+ only.

Reviewed By: alexmalyshev

Differential Revision: D80860343

fbshipit-source-id: ef1efbc31dda4c9344017c4256444fff39d7daa9

Author: jbower-fb

cinderx/Jit/codegen/gen_asm.cpp

@@ -219,6 +219,7 @@ prepareForDeopt(const uint64_t* regs, Runtime* runtime, std::size_t deopt_idx) {
         runtime->guardFailed(deopt_meta);
         break;
       }
+      case DeoptReason::kRaise:
       case DeoptReason::kYieldFrom: {
         break;
       }
@@ -233,23 +234,8 @@ prepareForDeopt(const uint64_t* regs, Runtime* runtime, std::size_t deopt_idx) {
         break;
       case DeoptReason::kUnhandledException:
         JIT_ABORT("unhandled exception without error set");
-      case DeoptReason::kRaise:
-        // This code mirrors what happens in _PyEval_EvalFrameDefault although
-        // I'm not sure how to test it. Not clear it can happen with JIT.
-#ifdef NDEBUG
-        if (!PyErr_Occurred()) {
-          PyErr_SetString(
-              PyExc_SystemError, "error return without exception set");
-        }
-#else
-        JIT_CHECK(PyErr_Occurred(), "Error return without exception set");
-#endif
-        break;
       case DeoptReason::kRaiseStatic:
         JIT_ABORT("Lost exception when raising static exception");
-      case DeoptReason::kReraise:
-        PyErr_SetString(PyExc_RuntimeError, "No active exception to reraise");
-        break;
     }
   }
   return frame;
@@ -272,7 +258,9 @@ PyObject* resumeInInterpreter(
   // Resume all of the inlined frames and the caller
   const DeoptMetadata& deopt_meta = runtime->getDeoptMetadata(deopt_idx);
   int inline_depth = deopt_meta.inline_depth();
-  int err_occurred = (deopt_meta.reason != DeoptReason::kGuardFailure);
+  int err_occurred =
+      (deopt_meta.reason != DeoptReason::kGuardFailure &&
+       deopt_meta.reason != DeoptReason::kRaise);
   while (inline_depth >= 0) {
     // Consider skipping resuming frames that do not have try/catch. Will
     // require re-adding _PyShadowFrame_Pop back for non-generators and
@@ -322,7 +310,9 @@ PyObject* resumeInInterpreter(
   PyThreadState* tstate = PyThreadState_Get();
 
   const DeoptMetadata& deopt_meta = runtime->getDeoptMetadata(deopt_idx);
-  int err_occurred = (deopt_meta.reason != DeoptReason::kGuardFailure);
+  int err_occurred =
+      (deopt_meta.reason != DeoptReason::kGuardFailure &&
+       deopt_meta.reason != DeoptReason::kRaise);
 
   PyObject* result = nullptr;
   // Resume all of the inlined frames and the caller
@@ -340,6 +330,15 @@ PyObject* resumeInInterpreter(
     JIT_CHECK(
         currentFrame(tstate) == frame, "unexpected frame at top of stack");
     setCurrentFrame(tstate, frame->previous);
+    // The interpreter calls _Py_Instrument() on the initial RESUME opcode in a
+    // function, but we don't do this in the JIT. Calling it now is a bit
+    // dubious, because we are splitting execution of the same function between
+    // instrumented and not. However, if we don't do this and instrumentation
+    // is enabled we might hit assertions in the deopted execution because the
+    // code's instrumentation version doesn't match the interpreter's.
+    JIT_CHECK(
+        _Py_Instrument(frameCode(frame), tstate->interp) == 0,
+        "Failed to instrument code on deopt");
     result = _PyEval_EvalFrameDefault(tstate, frame, err_occurred);
 
     frame = prev_frame;

cinderx/Jit/deopt.cpp

@@ -242,7 +242,9 @@ static BCIndex getDeoptResumeIndex(
   // function's callers then these will be resumed by the interpreter in
   // future and will never be a JIT guard failure.
   bool is_innermost = &frame == &meta.innermostFrame();
-  if ((is_innermost && meta.reason == DeoptReason::kGuardFailure) ||
+  if ((is_innermost &&
+       (meta.reason == DeoptReason::kGuardFailure ||
+        meta.reason == DeoptReason::kRaise)) ||
       forced_deopt) {
     return frame.cause_instr_idx;
   }
@@ -390,15 +392,7 @@ static DeoptReason getDeoptReason(const jit::hir::DeoptBase& instr) {
       return DeoptReason::kYieldFrom;
     }
     case jit::hir::Opcode::kRaise: {
-      auto& raise = static_cast<const hir::Raise&>(instr);
-      switch (raise.kind()) {
-        case hir::Raise::Kind::kReraise:
-          return DeoptReason::kReraise;
-        case hir::Raise::Kind::kRaiseWithExc:
-        case hir::Raise::Kind::kRaiseWithExcAndCause:
-          return DeoptReason::kRaise;
-      }
-      JIT_ABORT("invalid raise kind");
+      return DeoptReason::kRaise;
     }
     case jit::hir::Opcode::kRaiseStatic: {
       return DeoptReason::kRaiseStatic;

cinderx/Jit/deopt.h

@@ -76,7 +76,6 @@ struct LiveValue {
   X(YieldFrom)               \
   X(Raise)                   \
   X(RaiseStatic)             \
-  X(Reraise)                 \
   X(UnhandledException)      \
   X(UnhandledUnboundLocal)   \
   X(UnhandledUnboundFreevar) \

cinderx/Jit/hir/builder.cpp

@@ -1356,7 +1356,7 @@ void HIRBuilder::translate(
           break;
         }
         case RAISE_VARARGS: {
-          emitRaiseVarargs(tc, bc_instr);
+          emitRaiseVarargs(tc);
           break;
         }
         case YIELD_VALUE: {
@@ -4271,26 +4271,8 @@ void HIRBuilder::emitImportName(
   stack.push(res);
 }
 
-void HIRBuilder::emitRaiseVarargs(
-    TranslationContext& tc,
-    const jit::BytecodeInstruction& bc_instr) {
-  auto& stack = tc.frame.stack;
-  switch (bc_instr.oparg()) {
-    case 2: {
-      auto cause = stack.pop();
-      auto exc = stack.pop();
-      tc.emit<Raise>(2, tc.frame, exc, cause);
-      break;
-    }
-    case 1:
-      tc.emit<Raise>(1, tc.frame, stack.pop());
-      break;
-    case 0:
-      tc.emit<Raise>(0, tc.frame);
-      break;
-    default:
-      JIT_ABORT("Unsupported RAISE_VARARGS op: {}", bc_instr.oparg());
-  }
+void HIRBuilder::emitRaiseVarargs(TranslationContext& tc) {
+  tc.emit<Raise>(tc.frame);
 }
 
 void HIRBuilder::emitYieldFrom(TranslationContext& tc, Register* out) {

cinderx/Jit/hir/builder.h

@@ -343,9 +343,7 @@ class HIRBuilder {
       CFG& cfg,
       TranslationContext& tc,
       const jit::BytecodeInstruction& bc_instr);
-  void emitRaiseVarargs(
-      TranslationContext& tc,
-      const jit::BytecodeInstruction& bc_instr);
+  void emitRaiseVarargs(TranslationContext& tc);
   void emitRefineType(
       TranslationContext& tc,
       const jit::BytecodeInstruction& bc_instr);

cinderx/Jit/hir/hir.h

@@ -3697,39 +3697,7 @@ class INSTR_CLASS(
   }
 };
 
-// (Re)raises an exception with optional cause.
-class INSTR_CLASS(Raise, (TObject, TObject), Operands<>, DeoptBase) {
- public:
-  enum class Kind {
-    kReraise,
-    kRaiseWithExc,
-    kRaiseWithExcAndCause,
-  };
-
- private:
-  Raise(Kind kind, const FrameState& frame) : InstrT(frame), kind_(kind) {}
-
- public:
-  explicit Raise(const FrameState& frame) : Raise(Kind::kReraise, frame) {}
-
-  Raise(const FrameState& frame, Register* exc)
-      : Raise(Raise::Kind::kRaiseWithExc, frame) {
-    SetOperand(0, exc);
-  }
-
-  Raise(const FrameState& frame, Register* exc, Register* cause)
-      : Raise(Raise::Kind::kRaiseWithExcAndCause, frame) {
-    SetOperand(0, exc);
-    SetOperand(1, cause);
-  }
-
-  Kind kind() const {
-    return kind_;
-  }
-
- private:
-  const Kind kind_;
-};
+DEFINE_SIMPLE_INSTR(Raise, (), Operands<0>, DeoptBase);
 
 // Set an error by calling PyErr_Format() and then raising. This is typically
 // used when a runtime assertion implemented as part of a Python opcode is hit.

cinderx/Jit/jit_rt.cpp

@@ -1455,34 +1455,6 @@ PyObject* JITRT_ImportName(
       import_func, name, globals, locals, fromlist, level, nullptr);
 }
 
-void JITRT_DoRaise(PyThreadState* tstate, PyObject* exc, PyObject* cause) {
-  // If we re-raise with no error set, deliberately do nothing and let
-  // prepareForDeopt() handle this. We can't let do_raise() handle this by
-  // raising a RuntimeError as this would mean prepareForDeopt() does not call
-  // PyTraceBack_Here().
-  if (exc == nullptr) {
-    _PyErr_StackItem* exc_info = _PyErr_GetTopmostException(tstate);
-    PyObject* type_or_value =
-#if PY_VERSION_HEX < 0x030C0000
-        exc_info->exc_type;
-#else
-        exc_info->exc_value;
-#endif
-    if (Py_IsNone(type_or_value) || type_or_value == nullptr) {
-      return;
-    }
-  }
-  // We deliberately discard the return value here. In the interpreter a return
-  // value of 1 indicates a _valid_ re-raise which skips:
-  // (1) Calling PyTraceBack_Here().
-  // (2) Raising a SystemError if no exception is set (no need, do_raise
-  //     already handles this).
-  // (3) Calling tstate->c_tracefunc.
-  // We don't support (3) and handle (1) + (2) between the check above and in
-  // prepareForDeopt().
-  Cix_do_raise(tstate, exc, cause);
-}
-
 #if PY_VERSION_HEX < 0x030C0000
 enum class MakeGenObjectMode {
   kAsyncGenerator,

cinderx/Jit/jit_rt.h

@@ -359,12 +359,6 @@ PyObject* JITRT_ImportName(
     PyObject* fromlist,
     PyObject* level);
 
-/*
- * Wrapper around _Py_DoRaise() which handles the case where we re-raise but no
- * active exception is set.
- */
-void JITRT_DoRaise(PyThreadState* tstate, PyObject* exc, PyObject* cause);
-
 /*
  * Formats a f-string value
  */

cinderx/Jit/lir/generator.cpp

@@ -3082,25 +3082,6 @@ LIRGenerator::TranslatedBlock LIRGenerator::TranslateOneBasicBlock(
       }
       case Opcode::kRaise: {
         const auto& instr = static_cast<const Raise&>(i);
-        hir::Register* exc = nullptr;
-        hir::Register* cause = nullptr;
-
-        switch (instr.kind()) {
-          case Raise::Kind::kReraise:
-            break;
-          case Raise::Kind::kRaiseWithExcAndCause:
-            cause = instr.GetOperand(1);
-            [[fallthrough]];
-          case Raise::Kind::kRaiseWithExc:
-            exc = instr.GetOperand(0);
-            break;
-        }
-        bbb.appendCallInstruction(
-            OutVReg{OperandBase::k32bit},
-            Cix_do_raise,
-            env_->asm_tstate,
-            exc,
-            cause);
         appendGuardAlwaysFail(bbb, instr);
         break;
       }

cinderx/RuntimeTests/hir_test.cpp

@@ -1108,31 +1108,49 @@ TEST_F(HIRCloneTest, CanCloneBorrwedRefFields) {
 
 TEST_F(HIRCloneTest, CanCloneVariadicOpInstr) {
   Environment env;
+  auto out = env.AllocateRegister();
   auto v0 = env.AllocateRegister();
-  FrameState raise_fs{BCOffset{10}};
-  std::unique_ptr<Instr> raise_exc(Raise::create(1, raise_fs, v0));
-  std::unique_ptr<Instr> new_raise_exc(raise_exc->clone());
-  ASSERT_NE(raise_exc.get(), new_raise_exc.get());
-  ASSERT_TRUE(new_raise_exc->IsRaise());
-
-  Raise* orig_raise = static_cast<Raise*>(raise_exc.get());
-  Raise* dup_raise = static_cast<Raise*>(new_raise_exc.get());
-  EXPECT_EQ(orig_raise->kind(), dup_raise->kind());
-  EXPECT_EQ(orig_raise->GetOperand(0), dup_raise->GetOperand(0));
-  FrameState* orig_raise_fs = orig_raise->frameState();
-  EXPECT_EQ(orig_raise_fs->cur_instr_offs, 10);
-  EXPECT_NE(orig_raise_fs, dup_raise->frameState());
-
-  std::unique_ptr<Instr> raise_exc_cause(Raise::create(2, raise_fs, v0, v0));
-  std::unique_ptr<Instr> new_raise_exc_cause(raise_exc_cause->clone());
-  ASSERT_NE(raise_exc_cause.get(), new_raise_exc_cause.get());
-  ASSERT_TRUE(new_raise_exc_cause->IsRaise());
-
-  orig_raise = static_cast<Raise*>(raise_exc_cause.get());
-  dup_raise = static_cast<Raise*>(new_raise_exc_cause.get());
-  EXPECT_EQ(orig_raise->kind(), dup_raise->kind());
-  EXPECT_EQ(orig_raise->GetOperand(0), dup_raise->GetOperand(0));
-  EXPECT_EQ(orig_raise->GetOperand(1), dup_raise->GetOperand(1));
+
+  // Create a CallStatic with no arguments
+  std::unique_ptr<Instr> call_static_no_args(
+      CallStatic::create(0, out, nullptr, Type::fromObject(Py_None)));
+  std::unique_ptr<Instr> new_call_static_no_args(call_static_no_args->clone());
+  ASSERT_NE(call_static_no_args.get(), new_call_static_no_args.get());
+  ASSERT_TRUE(new_call_static_no_args->IsCallStatic());
+
+  CallStatic* orig_call = static_cast<CallStatic*>(call_static_no_args.get());
+  CallStatic* dup_call =
+      static_cast<CallStatic*>(new_call_static_no_args.get());
+  EXPECT_EQ(orig_call->addr(), dup_call->addr());
+  EXPECT_EQ(orig_call->ret_type(), dup_call->ret_type());
+
+  // Create a CallStatic with one argument
+  std::unique_ptr<Instr> call_static_one_arg(
+      CallStatic::create(1, out, nullptr, Type::fromObject(Py_None), v0));
+  std::unique_ptr<Instr> new_call_static_one_arg(call_static_one_arg->clone());
+  ASSERT_NE(call_static_one_arg.get(), new_call_static_one_arg.get());
+  ASSERT_TRUE(new_call_static_one_arg->IsCallStatic());
+
+  orig_call = static_cast<CallStatic*>(call_static_one_arg.get());
+  dup_call = static_cast<CallStatic*>(new_call_static_one_arg.get());
+  EXPECT_EQ(orig_call->addr(), dup_call->addr());
+  EXPECT_EQ(orig_call->ret_type(), dup_call->ret_type());
+  EXPECT_EQ(orig_call->GetOperand(0), dup_call->GetOperand(0));
+
+  // Create a CallStatic with two arguments
+  std::unique_ptr<Instr> call_static_two_args(
+      CallStatic::create(2, out, nullptr, Type::fromObject(Py_None), v0, v0));
+  std::unique_ptr<Instr> new_call_static_two_args(
+      call_static_two_args->clone());
+  ASSERT_NE(call_static_two_args.get(), new_call_static_two_args.get());
+  ASSERT_TRUE(new_call_static_two_args->IsCallStatic());
+
+  orig_call = static_cast<CallStatic*>(call_static_two_args.get());
+  dup_call = static_cast<CallStatic*>(new_call_static_two_args.get());
+  EXPECT_EQ(orig_call->addr(), dup_call->addr());
+  EXPECT_EQ(orig_call->ret_type(), dup_call->ret_type());
+  EXPECT_EQ(orig_call->GetOperand(0), dup_call->GetOperand(0));
+  EXPECT_EQ(orig_call->GetOperand(1), dup_call->GetOperand(1));
 }
 
 TEST_F(HIRCloneTest, CanCloneDeoptBase) {