jit: read a deref-last projection as a value, and put fast2locals on the virtualizable protocol

majit/majit-translate/src/front/mir.rs

@@ -4664,7 +4664,7 @@ impl<'a> Lowering<'a> {
             // itself. Aliasing the dest local to the referent Variable
             // keeps the IR small, treating `&x` as a same-Variable copy.
             Rvalue::Ref { place, .. } => {
-                let projection = matches!(&place.kind, PlaceKind::Projection(..));
+                let projection = Self::place_ref_is_address_of(&place);
                 let before = self.graph.block(self.block_id[mir_bb]).operations.len();
                 let v = self.resolve_place(mir_bb, place)?;
                 self.mark_place_address_of(mir_bb, projection, before, &v);
@@ -4675,7 +4675,7 @@ impl<'a> Lowering<'a> {
             // and references identically at the IR level (lifetime
             // tracking lives outside the JIT).
             Rvalue::RawPtr { place, .. } => {
-                let projection = matches!(&place.kind, PlaceKind::Projection(..));
+                let projection = Self::place_ref_is_address_of(&place);
                 let before = self.graph.block(self.block_id[mir_bb]).operations.len();
                 let v = self.resolve_place(mir_bb, place)?;
                 self.mark_place_address_of(mir_bb, projection, before, &v);
@@ -5375,6 +5375,24 @@ impl<'a> Lowering<'a> {
         Ok(var)
     }
 
+    /// Whether `&<place>` / `&raw [mut] <place>` takes the address of a
+    /// place, as opposed to reading the value one holds.
+    ///
+    /// `&*(*p).f` — a projection whose outermost element is `Deref` — yields
+    /// the value the pointer in `(*p).f` holds, so the underlying read stays
+    /// a value read and keeps its lowering.  `&(*p).f`, Field-last, is the
+    /// real place-address and stays marked.
+    ///
+    /// The `Deref` test is spelled as `resolve_place` and
+    /// `emit_projection_write` already spell it, applied one level out.
+    fn place_ref_is_address_of(place: &Place) -> bool {
+        match &place.kind {
+            PlaceKind::Projection(_, ProjectionElem::Atom(s)) if s == "Deref" => false,
+            PlaceKind::Projection(..) => true,
+            _ => false,
+        }
+    }
+
     /// Record that the `Variable` just resolved for a place is the
     /// **address** of a field, not its value.
     ///
@@ -21726,6 +21744,67 @@ mod tests {
         assert_eq!(resolve_to_producer_op(&graph, &Variable::new()), None);
     }
 
+    /// `&*(*p).f` reads the value the field holds; `&(*p).f` names the
+    /// field's address.
+    ///
+    /// Both reach `build_rvalue`'s `Ref` / `RawPtr` arms as a
+    /// `PlaceKind::Projection`, so a bare `matches!(.., Projection(..))`
+    /// calls them both an address-of.  On `locals_cells_stack_w` the false
+    /// mark is not inert: `rewrite_op_getfield` reads
+    /// `suppresses_virtualizable` and skips the `vable_array_vars`
+    /// registration, so the read leaves the protocol and becomes a plain
+    /// `getarrayitem_gc` against a heap array that is only synchronised at
+    /// the three `sync_virtualizable_*` points — a stale read, not a slow
+    /// equivalent.  `locals_w!` expands to the deref-then-ref spelling, so
+    /// every one of its readers took that path.
+    #[test]
+    fn a_deref_last_projection_reads_a_value_rather_than_naming_an_address() {
+        use super::Lowering;
+        use majit_charon_reader::ullbc::{Place, PlaceKind, ProjectionElem};
+
+        fn ty() -> TyRef {
+            TyRef::Other(serde_json::Value::Null)
+        }
+        fn place(kind: PlaceKind) -> Place {
+            Place { kind, ty: ty() }
+        }
+        fn field(inner: Place, name: &str) -> Place {
+            place(PlaceKind::Projection(
+                Box::new(inner),
+                ProjectionElem::Tagged(serde_json::json!({ "Field": name })),
+            ))
+        }
+        fn deref(inner: Place) -> Place {
+            place(PlaceKind::Projection(
+                Box::new(inner),
+                ProjectionElem::Atom("Deref".to_string()),
+            ))
+        }
+
+        let local = || place(PlaceKind::Local(0));
+        let is_addr = Lowering::place_ref_is_address_of;
+
+        // `&(*p).f` — the outermost step names a field, so the reference
+        // stands for that field's address and the mark is owed.
+        assert!(is_addr(&field(deref(local()), "locals_cells_stack_w")));
+
+        // `&*(*p).f` — the outermost step dereferences what the field
+        // holds, so the reference stands for the pointee.  The field read
+        // underneath is an ordinary value read.
+        assert!(!is_addr(&deref(field(
+            deref(local()),
+            "locals_cells_stack_w"
+        ))));
+
+        // `&*p`, the same shape one level in, and a bare local: neither
+        // names a field address either.
+        assert!(!is_addr(&deref(local())));
+        assert!(!is_addr(&local()));
+
+        // A nested field is still an address-of at its outermost step.
+        assert!(is_addr(&field(field(deref(local()), "a"), "b")));
+    }
+
     #[test]
     fn items_block_base_accessor_gate_excludes_deref_in_place() {
         use super::graph_is_items_block_base_accessor;

pyre/pyre-interpreter/src/eval.rs

@@ -3485,9 +3485,13 @@ impl OpcodeStepExecutor for PyFrame {
                 "WITH_EXCEPT_START requires five stack values",
             ));
         }
-        let val = locals_w!(self)[depth - 1];
-        let exit_self = locals_w!(self)[depth - 4];
-        let exit_func = locals_w!(self)[depth - 5];
+        // Indices first: a subscript evaluates its receiver before its index
+        // expression, so arithmetic inside the brackets puts the subtraction's
+        // overflow check between the `locals_cells_stack_w` read and its use.
+        let (i_val, i_self, i_func) = (depth - 1, depth - 4, depth - 5);
+        let val = locals_w!(self)[i_val];
+        let exit_self = locals_w!(self)[i_self];
+        let exit_func = locals_w!(self)[i_func];
         let res = with_except_start_values(exit_func, exit_self, val);
         if res.is_null() {
             return Err(crate::call::take_call_error()

pyre/pyre-interpreter/src/pyframe.rs

@@ -3025,13 +3025,23 @@ impl PyFrame {
 
     #[inline]
     pub fn peek(&self) -> PyObjectRef {
-        locals_w!(self)[self.valuestackdepth - 1]
+        // The index is computed into a local first, as
+        // `pyframe.py:479-484 peekvalue_maybe_none` computes `index` before
+        // its subscript.  A subscript evaluates its receiver before its index
+        // expression, so spelling the arithmetic inside the brackets emits the
+        // `locals_cells_stack_w` read ahead of the subtraction's overflow
+        // check, and that check's branch then carries the array out of the
+        // block on a link — which `_check_no_vable_array` rejects.
+        let index = self.valuestackdepth - 1;
+        locals_w!(self)[index]
     }
 
     #[inline]
     #[allow(dead_code)]
     pub fn peek_at(&self, depth: usize) -> PyObjectRef {
-        locals_w!(self)[self.valuestackdepth - 1 - depth]
+        // Hoisted for the reason given on [`Self::peek`].
+        let index = self.valuestackdepth - 1 - depth;
+        locals_w!(self)[index]
     }
 
     /// Null the locals_cells_stack slots at and above `depth`, the
@@ -3143,7 +3153,8 @@ impl PyFrame {
         let mut idx = n;
         while idx > 0 {
             idx -= 1;
-            values_w[idx] = locals_w!(self)[base + idx];
+            let slot = base + idx;
+            values_w[idx] = locals_w!(self)[slot];
         }
         values_w
     }
@@ -4144,6 +4155,13 @@ impl PyFrame {
     /// silently dropped).  A function frame with no locals bound yet lazily
     /// allocates a fresh dict (pyframe.py:557 `self.space.newdict(instance=True)`)
     /// and caches it, so `locals() is locals()` holds.  Errors propagate.
+    ///
+    /// `@jit.unroll_safe` (`pyframe.py:572`) cancels `contains_loop` in the
+    /// policy (`codewriter/policy.rs look_inside_graph`), so the slot loop
+    /// below does not keep the codewriter out.  Without it the whole function
+    /// is one residual call, and the `f_locals` read behind it forces the
+    /// virtualizable for the length of that call.
+    #[majit_macros::unroll_safe]
     pub fn fast2locals(&mut self) -> Result<(), crate::PyError> {
         // `space.setitem_str` / `space.delitem` allocate one key per slot and
         // can collect.  RPython's GC transform keeps both the frame and its