Skip to content

majit: stop resolving a forced instance's w_class header in the optimizer (#920 review) #922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 2 commits into from
Jul 31, 2026
Merged

majit: stop resolving a forced instance's w_class header in the optimizer (#920 review) #922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
868f1ba
Revert "majit: exit the short-preamble force loop on a non-growing ar…
youknowone Jul 31, 2026
4dcad94
majit: stop resolving a forced instance's w_class header in the optim…
youknowone Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 0 additions & 89 deletions majit/majit-metainterp/src/optimizeopt/heap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,26 +861,6 @@ impl OptHeap {
}
}

/// Is a `w_class` store still pending under ANY descr spelling?
///
/// The field caches are keyed by `Arc::as_ptr` identity
/// (`cached_field_pos_for_descr`), and pyre mints more than one descr for
/// the `w_class` header (a walker singleton with no parent, and the size
/// descr's own `all_fielddescrs` entry). A miss on the descr being read
/// therefore does not prove the field is untouched — the store may sit in
/// the other spelling's `lazy_set`. Asking across every spelling costs a
/// fold, never correctness.
///
/// Only `lazy_set` is consulted: a store that has already been flushed
/// (`force_lazy_set` → `put_field_back_to_info`) is readable from the
/// instance's own field slot, and `cached_structs` also fills up from plain
/// reads, which say nothing about the field having been written.
fn w_class_set_pending(&self) -> bool {
self.cached_fields.iter().any(|(_, descr, cf)| {
descr.as_field_descr().is_some_and(|fd| fd.is_w_class()) && cf.lazy_set.is_some()
})
}

fn cached_field_pos_for_descr(&self, descr: &DescrRef) -> Option<usize> {
let identity = descr_identity(descr);
self.cached_fields
Expand Down Expand Up @@ -2227,75 +2207,6 @@ impl OptHeap {
// Cache miss — fall through to emit the getfield
}

// Pyre object-model: `w_class` (PyObject header) carries Python-level
// class identity. `virtualize.rs` resolves it from a virtual's own
// class; once the allocation is forced the read arrives here instead.
// `info.py:152 force_box` keeps the class (`info.py:324
// get_known_class` still answers), so a forced instance still names its
// layout and the header value is that layout's canonical class.
//
// A store that reached the instance's own slot wins over the layout's
// canonical class; only when the header was never written does the
// layout answer, and then only if no store is still pending. A retag to
// a user subclass writes this header, and folding past that write would
// answer with the base class and silently defeat a class check. Reads
// of an object whose layout is unknown (`descr: None`, what
// `ensure_ptr_info_arg0` builds for a plain heap object) resolve to
// nothing and stay in the trace.
//
// The slot is looked up through the LAYOUT's own `w_class` fielddescr,
// never the read descr's index: a parent-bearing `w_class` spelling has
// `index_in_parent == 0`, which is the first VALUE field, and folding
// through it forwards `Ref <- Int` (`virtualize.rs:850-856`).
if descr.as_field_descr().is_some_and(|fd| fd.is_w_class()) {
let obj_box = ctx.get_box_replacement_operand(obj);
if let Some(crate::optimizeopt::info::PtrInfo::Instance(iinfo)) =
ctx.peek_ptr_info(&obj_box)
{
let size_descr = iinfo.descr.as_ref().and_then(|d| d.as_size_descr());
let stored = size_descr
.and_then(|sd| {
sd.all_fielddescrs()
.iter()
.find(|fd| fd.is_w_class())
.map(|fd| fd.index_in_parent() as u32)
})
.and_then(|widx| {
iinfo
.fields
.iter()
.find(|(idx, _)| *idx == widx)
.map(|(_, e)| e.clone())
});
match stored {
Some(crate::optimizeopt::info::FieldEntry::Value(b_val)) => {
let b_old = Operand::from_bound_op(op_rc);
let b_val = ctx.get_box_replacement_operand(b_val.to_opref());
ctx.make_equal_to(&b_old, &b_val);
return OptimizationResult::Remove;
}
// Not a value yet — leave the read rather than folding past
// the pending preamble op.
Some(crate::optimizeopt::info::FieldEntry::Preamble(_)) => {}
None => {
if !self.w_class_set_pending() {
if let Some(w_class) = size_descr
.and_then(|sd| sd.w_class_obj())
.filter(|&w| w != 0)
{
let b = ctx.materialize_operand_at(op.pos.get());
ctx.make_constant_box(
&b,
majit_ir::Value::Ref(majit_ir::GcRef(w_class as usize)),
);
return OptimizationResult::Remove;
}
}
}
}
}
}

// Virtualizable fields are loop-variant; skip caching/import.
let is_vable_field = descr.is_virtualizable();

Expand Down
5 changes: 1 addition & 4 deletions majit/majit-metainterp/src/optimizeopt/unroll.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4040,10 +4040,7 @@ impl OptUnroll {
// producer first, since the top-of-loop _map_args above would
// otherwise read an unmapped arg.
let grown = current_short_jump_args(short_preamble, ctx);
// `<=`, not `==`: the list only grows here, but the slice below
// indexes from `num_short_jump_args`, so a shrink would panic
// rather than fall into the "did not grow" exit.
if grown.len() <= num_short_jump_args {
if grown.len() == num_short_jump_args {
break;
}
let all_new_mapped = grown[num_short_jump_args..]
Expand Down
33 changes: 21 additions & 12 deletions majit/majit-metainterp/src/optimizeopt/virtualize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,18 +893,27 @@ impl OptVirtualize {
// rather than mis-indexing a value field.
return OptimizationResult::PassOn;
}
// Once the allocation is forced the read is NOT resolved here.
// `virtualize.py:184-195 optimize_GETFIELD_GC_*` folds only
// under `opinfo.is_virtual()` and otherwise emits, leaving a
// non-virtual's field read to `heap.py`. That layering is
// load-bearing for this field: `force_box` empties the forced
// instance's field list (info.rs:1113-1118, so heap's
// `do_setfield` cannot MUST_ALIAS-elide the materializing
// SETFIELD_GC) and routes the write into `OptHeap`, where it
// sits in `CachedField::lazy_set`. Folding here would run
// BEFORE that pass and answer from the layout's canonical
// class, discarding a pending retag to a user subclass.
// The canonical-class fallback lives in `optimize_getfield`.
// Once the allocation is forced the read is NOT resolved — not
// here and not in `optimize_getfield`. `virtualize.py:184-195
// optimize_GETFIELD_GC_*` folds only under
// `opinfo.is_virtual()` and otherwise emits; upstream has no
// counterpart to resolve afterwards because
// `jtransform.py:1004-1009 handle_getfield_typeptr` deletes the
// read at codewriter time, so no typeptr getfield ever reaches
// the optimizer.
//
// Answering from the layout's canonical class here is unsound:
// `force_box` empties the forced instance's field list
// (info.rs:1113-1118, so heap's `do_setfield` cannot
// MUST_ALIAS-elide the materializing SETFIELD_GC) and routes
// the header write into `OptHeap`, where it sits in
// `CachedField::lazy_set`. A retag to a user subclass would be
// discarded and the base class answered instead. Resolving it
// in `OptHeap` does not close the hole either: reads and writes
// of this header carry different descr spellings, and the
Comment on lines +909 to +913

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Flush retag stores before passing on forced-header reads

When a virtual with an overridden w_class is forced by a side-effect-free operation before a later class read, its materializing SETFIELD_GC is cached here under the layout descriptor while the subsequent GETFIELD_GC uses the standalone header descriptor. The read therefore misses the pending store, and because GETFIELD_GC is side-effect-free, OptHeap::emitting_operation does not flush lazy sets before emitting it; the generated order is the class load followed by the retag store, so the load observes the allocation's original base class and can make type/class guards silently take the wrong path. Merely removing the fold does not fix this case—the pending cross-spelling store must be forced before the read, or the descriptors must first be unified.

AGENTS.md reference: AGENTS.md:L14-L19

Useful? React with 👍 / 👎.

// caches are keyed by `Arc::as_ptr` while `structinfo_setfield`
// slots by `field_slot_index`, so the two do not meet. Removing
// that split is the prerequisite for folding this read at all.
}
let field_val = match &info {
PtrInfo::Virtual(vinfo) => get_field(&vinfo.fields, field_idx),
Expand Down
Loading