Eliminate redundant UpdatePrevInstr stores in InsertUpdatePrevInstr pass

Summary:
The InsertUpdatePrevInstr pass inserts UpdatePrevInstr instructions before every hasArbitraryExecution instruction to keep the frame's prev_instr/instr_ptr field current for stack traces and instrumentation. The existing pass already avoids emitting duplicate stores when the source line hasn't changed, but it still emits redundant stores when consecutive arbitrary-execution instructions are on different lines with no observable instructions between them. Only the last store before each observation point matters.

This change adds intra-block dead store elimination directly into the existing single-pass insertion loop. When a new UpdatePrevInstr is about to be inserted and the previously inserted one hasn't been observed by any hasArbitraryExecution instruction, the previous one is dead and gets removed. This is tracked with two variables per block: a pointer to the last emitted UpdatePrevInstr (`last_emitted`) and a bool indicating whether an arbitrary-execution instruction has been seen since the last emit (`saw_arbitrary`).

State is conservatively reset at inline function boundaries (BeginInlinedFunction/EndInlinedFunction) since the UpdatePrevInstr stores target different frames across those boundaries.

This is safe because deoptimization does not read the frame's prev_instr field — it uses FrameState::cur_instr_offs from the HIR metadata instead (see deopt.cpp reifyFrameImpl). The only consumers of prev_instr are instructions with hasArbitraryExecution (calls into Python/C that may inspect the frame for tracebacks, sys._getframe(), profilers, instrumentation), and our optimization guarantees the store is live before each such instruction.

The optimization adds zero extra passes over the IR — it piggybacks on the existing insertion walk with only a pointer and bool of additional state per block.

Reviewed By: alexmalyshev

Differential Revision: D103112980

fbshipit-source-id: a14b3f6e97f4aac0302cbff0324b00bbfccaab73

Author: kddnewton

cinderx/Jit/hir/hir.h

@@ -3734,6 +3734,10 @@ class INSTR_CLASS(UpdatePrevInstr, (), Operands<0>) {
     return line_no_;
   }
 
+  void setLineNo(int line_no) {
+    line_no_ = line_no;
+  }
+
   // The inlined function which this update belongs to or nullptr if not in an
   // inlined function.
   BeginInlinedFunction* parent() const {

cinderx/Jit/hir/insert_update_prev_instr.cpp

@@ -88,12 +88,20 @@ void InsertUpdatePrevInstr::Run([[maybe_unused]] Function& func) {
     worklist.pop();
 
     int prev_emitted_lno_or_bc = INT_MAX;
+    Instr* last_emitted = nullptr;
     for (Instr& instr : *block) {
       auto update_one = [&]() {
         auto add_update_prev_instr = [&](int line_no) {
-          Instr* update_instr = UpdatePrevInstr::create(line_no, parent);
-          update_instr->copyBytecodeOffset(instr);
-          update_instr->InsertBefore(instr);
+          if (last_emitted != nullptr) {
+            last_emitted->unlink();
+            static_cast<UpdatePrevInstr*>(last_emitted)->setLineNo(line_no);
+            last_emitted->copyBytecodeOffset(instr);
+            last_emitted->InsertBefore(instr);
+          } else {
+            last_emitted = UpdatePrevInstr::create(line_no, parent);
+            last_emitted->copyBytecodeOffset(instr);
+            last_emitted->InsertBefore(instr);
+          }
         };
         // If we don't have a valid line table to optimize with, update after
         // every bytecode.
@@ -135,12 +143,16 @@ void InsertUpdatePrevInstr::Run([[maybe_unused]] Function& func) {
         }
         parents[begin] = parent;
         parent = begin;
+        last_emitted = nullptr;
+        prev_emitted_lno_or_bc = INT_MAX;
         if (getConfig().frame_mode == FrameMode::kLightweight) {
           inited_once = false;
         }
       } else if (instr.IsEndInlinedFunction()) {
         parent =
             parents[static_cast<EndInlinedFunction&>(instr).matchingBegin()];
+        last_emitted = nullptr;
+        prev_emitted_lno_or_bc = INT_MAX;
       }
 
       if (getConfig().frame_mode == FrameMode::kLightweight) {
@@ -157,11 +169,13 @@ void InsertUpdatePrevInstr::Run([[maybe_unused]] Function& func) {
           update_instr->setBytecodeOffset(
               BCIndex(target_code->_co_firsttraceable));
           update_instr->InsertBefore(instr);
+          last_emitted = update_instr;
 
           inited_once = true;
         }
       } else if (hasArbitraryExecution(instr)) {
         update_one();
+        last_emitted = nullptr;
       }
     }
 

cinderx/PythonLib/test_cinderx/test_jit_frame.py

@@ -110,6 +110,29 @@ def f3():
         self.assert_code_and_lineno(gen1_frame, f1, 2)
         self.assert_code_and_lineno(gen2_frame, f2, 2)
 
+    def test_line_numbers_consecutive_calls(self) -> None:
+        """Verify line numbers are correct across consecutive calls on different
+        lines. This exercises the UpdatePrevInstr dead store elimination: the
+        JIT eliminates redundant prev_instr stores between consecutive calls,
+        but each call must still see the correct line number."""
+        stacks = []
+
+        def capture():
+            stacks.append(traceback.extract_stack())
+
+        @cinder_support.failUnlessJITCompiled
+        def f():
+            capture()
+            capture()
+            capture()
+            capture()
+
+        f()
+        self.assertEqual(len(stacks), 4)
+        base = firstlineno(f)
+        for i, stack in enumerate(stacks):
+            self.assertEqual(stack[-2].lineno, base + 2 + i)
+
     def test_line_numbers_from_finalizers(self) -> None:
         """Make sure we can get accurate line numbers from finalizers"""
         stack = []

cinderx/RuntimeTests/hir_insert_update_prev_instr_test.cpp

@@ -0,0 +1,90 @@
+// Copyright (c) Meta Platforms, Inc. and affiliates.
+
+#include <gtest/gtest.h>
+
+#include "cinderx/Jit/hir/hir.h"
+#include "cinderx/Jit/hir/insert_update_prev_instr.h"
+#include "cinderx/Jit/hir/instr_effects.h"
+#include "cinderx/Jit/hir/phi_elimination.h"
+#include "cinderx/Jit/hir/simplify.h"
+#include "cinderx/Jit/hir/ssa.h"
+#include "cinderx/RuntimeTests/fixtures.h"
+
+using namespace jit::hir;
+
+class InsertUpdatePrevInstrTest : public RuntimeTest {};
+
+namespace {
+
+int countIf(const Function& func, auto pred) {
+  int count = 0;
+  for (const auto& block : func.cfg.blocks) {
+    for (const auto& instr : block) {
+      if (pred(instr)) {
+        count++;
+      }
+    }
+  }
+  return count;
+}
+
+std::unique_ptr<Function> compileAndRunPass(
+    RuntimeTest* test,
+    const char* src) {
+  std::unique_ptr<Function> irfunc;
+  test->CompileToHIR(src, "test", irfunc);
+  if (irfunc == nullptr) {
+    return nullptr;
+  }
+  SSAify{}.Run(*irfunc);
+  Simplify{}.Run(*irfunc);
+  PhiElimination{}.Run(*irfunc);
+  InsertUpdatePrevInstr{}.Run(*irfunc);
+  return irfunc;
+}
+
+} // namespace
+
+TEST_F(InsertUpdatePrevInstrTest, RedundantStoresEliminated) {
+  // Four len() calls on consecutive lines produce four arbitrary-execution
+  // points on different source lines. The additions produce more. Without
+  // dead store elimination, each line change emits its own UpdatePrevInstr.
+  // With the optimization, consecutive UpdatePrevInstr stores separated only
+  // by non-arbitrary-execution instructions are collapsed.
+  const char* src = R"(
+def test(a):
+  w = len(a)
+  x = len(a)
+  y = len(a)
+  z = len(a)
+  return w + x + y + z
+)";
+  auto irfunc = compileAndRunPass(this, src);
+  ASSERT_NE(irfunc, nullptr);
+
+  int update_count =
+      countIf(*irfunc, [](const Instr& i) { return i.IsUpdatePrevInstr(); });
+  int arbitrary_count = countIf(*irfunc, hasArbitraryExecution);
+
+  // There must be at least one UpdatePrevInstr.
+  ASSERT_GT(update_count, 0);
+  // There must be multiple arbitrary-execution points (len calls + additions).
+  ASSERT_GE(arbitrary_count, 7);
+  // The optimization must eliminate at least one redundant store: each
+  // consecutive pair of arbitrary-execution instructions on different lines
+  // with nothing observable between them has its first store removed.
+  EXPECT_LT(update_count, arbitrary_count);
+}
+
+TEST_F(InsertUpdatePrevInstrTest, SingleCallPreservesStore) {
+  const char* src = R"(
+def test(a):
+  return len(a)
+)";
+  auto irfunc = compileAndRunPass(this, src);
+  ASSERT_NE(irfunc, nullptr);
+
+  int update_count =
+      countIf(*irfunc, [](const Instr& i) { return i.IsUpdatePrevInstr(); });
+  EXPECT_GT(update_count, 0);
+}