jit: fix a lost conditional store in an inlined callee, widen the method-form inline

majit/majit-metainterp/src/trace_ctx.rs

@@ -3414,6 +3414,38 @@ impl TraceCtx {
         self.box_value(elem)
     }
 
+    /// Resolve the array-base `OpRef` (`frame.locals_cells_stack_w`) for a
+    /// NONSTANDARD virtualizable the way `opimpl_getfield_gc_r` does
+    /// (`_opimpl_getfield_gc_any_pureornot`, pyjitpl.py): forward the cached
+    /// field box on a hit, otherwise record the `GetfieldGcR` once and publish
+    /// it with `getfield_now_known`.
+    ///
+    /// Every `getarrayitem_vable` / `setarrayitem_vable` on such a frame has to
+    /// come through here, because the per-array element cache is keyed by this
+    /// base `OpRef`.  Recording a fresh base per access instead keys each store
+    /// under an `OpRef` no read ever looks up, so a store neither updates nor
+    /// invalidates the entry the reads share — a local written after the read
+    /// that seeded the cache then keeps reading its pre-store value for as long
+    /// as the base stays cached.
+    fn nonstandard_vable_array_base(&mut self, vable_opref: OpRef, fdescr: &DescrRef) -> OpRef {
+        let record_descr = self.vable_array_record_descr(fdescr);
+        let field_index = record_descr.index();
+        if let Some(cached) = self.heapcache_getfield_cached(vable_opref, field_index) {
+            self.profiler().count_ops(
+                OpCode::GetfieldGcR,
+                crate::pyjitpl::counters::HEAPCACHED_OPS,
+            );
+            return cached;
+        }
+        self.profiler()
+            .count_ops(OpCode::GetfieldGcR, crate::counters::OPS);
+        self.profiler()
+            .count_ops(OpCode::GetfieldGcR, crate::counters::RECORDED_OPS);
+        let op = self.record_op_with_descr(OpCode::GetfieldGcR, &[vable_opref], record_descr);
+        self.heapcache_getfield_now_known(vable_opref, field_index, op);
+        op
+    }
+
     /// pyjitpl.py:1167-1172 `opimpl_getfield_vable_i(box, fielddescr, pc)`.
     ///
     /// ```text
@@ -4025,13 +4057,7 @@ impl TraceCtx {
         if self.is_nonstandard_virtualizable(pc, vable_opref, &fdescr, concrete) {
             // arraybox = self.opimpl_getfield_gc_r(box, fdescr)
             // return self.opimpl_getarrayitem_gc_i(arraybox, indexbox, adescr)
-            let record_descr = self.vable_array_record_descr(&fdescr);
-            self.profiler()
-                .count_ops(OpCode::GetfieldGcR, crate::counters::OPS);
-            self.profiler()
-                .count_ops(OpCode::GetfieldGcR, crate::counters::RECORDED_OPS);
-            let array_opref =
-                self.record_op_with_descr(OpCode::GetfieldGcR, &[vable_opref], record_descr);
+            let array_opref = self.nonstandard_vable_array_base(vable_opref, &fdescr);
             return (
                 self.vable_getarrayitem_int_descr(array_opref, index, adescr),
                 None,
@@ -4103,13 +4129,7 @@ impl TraceCtx {
                 index,
                 adescr.index(),
             );
-            let record_descr = self.vable_array_record_descr(&fdescr);
-            self.profiler()
-                .count_ops(OpCode::GetfieldGcR, crate::counters::OPS);
-            self.profiler()
-                .count_ops(OpCode::GetfieldGcR, crate::counters::RECORDED_OPS);
-            let array_opref =
-                self.record_op_with_descr(OpCode::GetfieldGcR, &[vable_opref], record_descr);
+            let array_opref = self.nonstandard_vable_array_base(vable_opref, &fdescr);
             let item = self.vable_getarrayitem_ref_descr(array_opref, index, adescr);
             if let Some(v) = fwd {
                 self.set_opref_concrete(item, v);
@@ -4174,13 +4194,7 @@ impl TraceCtx {
     ) -> (OpRef, Option<Value>) {
         let concrete = self.concrete_of_opref(vable_opref);
         if self.is_nonstandard_virtualizable(pc, vable_opref, &fdescr, concrete) {
-            let record_descr = self.vable_array_record_descr(&fdescr);
-            self.profiler()
-                .count_ops(OpCode::GetfieldGcR, crate::counters::OPS);
-            self.profiler()
-                .count_ops(OpCode::GetfieldGcR, crate::counters::RECORDED_OPS);
-            let array_opref =
-                self.record_op_with_descr(OpCode::GetfieldGcR, &[vable_opref], record_descr);
+            let array_opref = self.nonstandard_vable_array_base(vable_opref, &fdescr);
             return (
                 self.vable_getarrayitem_float_descr(array_opref, index, adescr),
                 None,
@@ -4244,18 +4258,12 @@ impl TraceCtx {
     ) -> bool {
         let vable_concrete = self.concrete_of_opref(vable_opref);
         if self.is_nonstandard_virtualizable(pc, vable_opref, &fdescr, vable_concrete) {
-            let record_descr = self.vable_array_record_descr(&fdescr);
-            self.profiler()
-                .count_ops(OpCode::GetfieldGcR, crate::counters::OPS);
-            self.profiler()
-                .count_ops(OpCode::GetfieldGcR, crate::counters::RECORDED_OPS);
-            let array_opref =
-                self.record_op_with_descr(OpCode::GetfieldGcR, &[vable_opref], record_descr);
+            let array_opref = self.nonstandard_vable_array_base(vable_opref, &fdescr);
             self.profiler()
                 .count_ops(OpCode::SetarrayitemGc, crate::counters::OPS);
             self.profiler()
                 .count_ops(OpCode::SetarrayitemGc, crate::counters::RECORDED_OPS);
-            self.vable_setarrayitem_descr(array_opref, index, value, adescr);
+            self.execute_setarrayitem_gc(array_opref, index, value, adescr);
             return true;
         }
         // index = self._get_arrayitem_vable_index(pc, fdescr, indexbox)
@@ -4488,6 +4496,29 @@ impl TraceCtx {
     ) {
         self.record_op_with_descr(OpCode::SetarrayitemGc, &[array_opref, index, value], descr);
     }
+
+    /// `execute_setarrayitem_gc(arraydescr, arraybox, indexbox, itembox)`
+    /// (pyjitpl.py): record the `SETARRAYITEM_GC`, then publish the stored item
+    /// to the heapcache.  `gen_store_back_in_vable` deliberately does NOT come
+    /// through here — it records the op directly — so the raw recording form
+    /// stays available as `vable_setarrayitem_descr`.
+    ///
+    /// The heapcache write is what keeps a later read of the same slot from
+    /// forwarding the value the cache was seeded with: the element cache is the
+    /// only thing a nonstandard virtualizable's reads consult for the stored
+    /// box, so a store that skips it leaves every subsequent read answering the
+    /// pre-store value.
+    fn execute_setarrayitem_gc(
+        &mut self,
+        array_opref: OpRef,
+        index: OpRef,
+        value: OpRef,
+        descr: DescrRef,
+    ) {
+        let descr_index = descr.index();
+        self.vable_setarrayitem_descr(array_opref, index, value, descr);
+        self.heapcache_setarrayitem(array_opref, index, descr_index, value);
+    }
 }
 
 #[cfg(test)]

pyre/bench/synth/inline_subwalk_mutating_residual.py

@@ -1,4 +1,4 @@
-# pyre-check: max-pypy-ratio=200
+# pyre-check: max-pypy-ratio=40
 # gh#495 guard: an inlined subwalk whose callee makes an unjournaled mutation
 # through a nested residual CALL, in the two miss-handling shapes.
 #

pyre/bench/synth/inline_subwalk_property_mutates.py

@@ -1,4 +1,4 @@
-# pyre-check: max-pypy-ratio=80
+# pyre-check: max-pypy-ratio=50
 # gh#495 guard: inlined property residual mutates before branch and caught miss.
 # @property value-returning mutating + try/except-inside-callee raising branch
 N = 60000

pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs

@@ -413,6 +413,13 @@ pub(crate) fn callee_body_contains_raise(body_code: &[u8]) -> bool {
     false
 }
 
+/// Whether a method-form callee body is free of `LoadAttr` residuals.
+///
+/// Consulted only by the entries that pass `allow_method_load_attr = false`.
+/// A `self.attr` read in the body is what makes it answer `false`, which is the
+/// common shape (`def at(self, i): return self.v + i`), so an entry that opts
+/// out of the check trades a narrower inline surface for the ability to inline
+/// ordinary accessor methods.
 pub(crate) fn method_form_callee_body_supported(
     body_code: &[u8],
     callee_descr_refs: &[DescrRef],
@@ -1742,7 +1749,7 @@ pub(crate) fn try_walker_inline_user_call<Sym: WalkSym>(
         has_closure,
         None,
         None,
-        false,
+        true,
         false,
         None,
     )
@@ -2689,6 +2696,12 @@ pub(crate) fn try_walker_inline_resolved_user_call<Sym: WalkSym>(
     if !bridge_rec_root_selfrec && fbw_hazardous_inline_denied(callee_code_key) {
         return Ok(None);
     }
+    // True when only the widened method-form surface reaches this callee: an
+    // unbound `self.attr` accessor body, which the narrow surface declines.
+    let widened_method_form = method_form
+        && bound_method.is_none()
+        && allow_method_load_attr
+        && !method_form_callee_body_supported(body.code, callee_descr_refs);
     // A legacy, unseeded inline sub-walk inside a FOR_ITER body resumes a guard
     // at the caller's CALL boundary, so deopt re-executes the whole callee.
     // Replaying a live-heap mutation would double it, so a Dirty body stays on
@@ -2725,8 +2738,16 @@ pub(crate) fn try_walker_inline_resolved_user_call<Sym: WalkSym>(
                 // iteration's contribution is dropped, silently.  A `Clean`
                 // body is still admitted from there — it has nothing that can
                 // abort.
-                foriter_deferred_admit =
-                    arg_class_guard.is_none() && !fbw_foriter_deferred_call_denied(callee_code_key);
+                //
+                // The widened method-form surface stays out: its deferred call
+                // is the `self.attr` read's own dispatch, which the lever
+                // resolves to a builtin rather than a body it can inline, so the
+                // admission spends the abort and then denies the callee anyway.
+                // Declining here reaches the same residual call without retiring
+                // the enclosing loop.
+                foriter_deferred_admit = arg_class_guard.is_none()
+                    && !widened_method_form
+                    && !fbw_foriter_deferred_call_denied(callee_code_key);
                 foriter_deferred_admit
             }
             CalleeReplaySafety::Dirty => {
@@ -2758,12 +2779,30 @@ pub(crate) fn try_walker_inline_resolved_user_call<Sym: WalkSym>(
             return Ok(None);
         }
     }
-    if method_form
-        && bound_method.is_none()
-        && !allow_method_load_attr
-        && !method_form_callee_body_supported(body.code, callee_descr_refs)
-    {
-        return Ok(None);
+    if method_form && bound_method.is_none() {
+        // Two surfaces for an unbound method-form callee.  The narrow one
+        // declines any `self.attr` read in the body.  The wide one admits it,
+        // and pays for the reach with a body that raises: the sub-walk records
+        // into the handler region, and a guard whose resume coordinate lands on
+        // the `Reraise` needs ref registers the recorded path never wrote
+        // (`collect_callee_active_boxes`).  That decline arrives mid-recording
+        // on a non-effect-free opcode, so it has no mid-body carrier and the
+        // whole enclosing loop is discarded.  Decline such a body here instead,
+        // where the call simply stays residual and the loop still compiles.
+        let declined = if allow_method_load_attr {
+            callee_body_contains_raise(body.code)
+        } else {
+            !method_form_callee_body_supported(body.code, callee_descr_refs)
+        };
+        if declined {
+            if std::env::var_os("PYRE_FBW_INLINE_DIAG").is_some() {
+                eprintln!(
+                    "[inline-method-form] decline pc={} allow_load_attr={allow_method_load_attr}",
+                    op.pc
+                );
+            }
+            return Ok(None);
+        }
     }
     if std::env::var("PYRE_FBW_INLINE_DIAG").is_ok() {
         let mut pc = 0usize;