-
Notifications
You must be signed in to change notification settings - Fork 19
jit-trace: bridge the codewriter's PyObject.w_class read to the walker descr (append loop 40 → 33 ops) #931
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3628,6 +3628,70 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| /// A `PyObject.w_class` `BhDescr` describing the same access as the | ||
| /// canonical header descr, for both owner spellings the codewriter emits. | ||
| fn w_class_bh(owner: &str, field_size: usize) -> majit_translate::jitcode::BhDescr { | ||
| use majit_ir::descr::ArrayFlag; | ||
| use majit_translate::jitcode::BhDescr; | ||
|
|
||
| BhDescr::Field { | ||
| offset: pyre_object::pyobject::W_CLASS_OFFSET, | ||
| field_size, | ||
| field_type: Type::Ref, | ||
| field_flag: ArrayFlag::Signed, | ||
| is_field_signed: false, | ||
| is_immutable: false, | ||
| is_quasi_immutable: false, | ||
| // slot 0 is `ob_type`; `w_class` is slot 1 of the header. | ||
| index_in_parent: 1, | ||
| parent: None, | ||
| name: "w_class".into(), | ||
| owner: owner.into(), | ||
| } | ||
| } | ||
|
|
||
| /// The shared `PyObject` header's `w_class` bridges to the same descr the | ||
| /// walker pins a value's class through, so a codewriter-lowered subclass | ||
| /// test (`is_plain_int1`) reads the header the walker already guarded | ||
| /// instead of emitting a second, uncacheable read of the same offset. | ||
| #[test] | ||
| fn make_descr_from_bh_bridges_pyobject_w_class_to_the_walker_descr() { | ||
| let canonical = w_class_descr(); | ||
| let width = W_CLASS_FIELD_DESCR.field_size(); | ||
|
|
||
| for owner in ["PyObject", "pyre_object::pyobject::PyObject"] { | ||
| let descr = make_descr_from_bh(&w_class_bh(owner, width)); | ||
| assert!( | ||
| std::sync::Arc::ptr_eq(&descr, &canonical), | ||
| "{owner}.w_class must bridge to the walker's w_class descr Arc", | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// …but only when the two spellings describe the same access. The canonical | ||
| /// descr hardcodes an 8-byte width while the codewriter sizes a pointer by | ||
| /// `target_word_size()`, so on a 32-bit target the incoming descr is a | ||
| /// narrower load at the same offset. Bridging there would widen the read | ||
| /// over the adjacent payload, so the mismatch declines instead. | ||
| #[test] | ||
| fn make_descr_from_bh_declines_w_class_bridge_on_a_width_mismatch() { | ||
| let canonical = w_class_descr(); | ||
| let narrower = W_CLASS_FIELD_DESCR.field_size() / 2; | ||
|
|
||
| for owner in ["PyObject", "pyre_object::pyobject::PyObject"] { | ||
| let descr = make_descr_from_bh(&w_class_bh(owner, narrower)); | ||
| assert!( | ||
| !std::sync::Arc::ptr_eq(&descr, &canonical), | ||
| "{owner}.w_class must not bridge to a descr of a different width", | ||
| ); | ||
| assert_eq!( | ||
| descr.as_field_descr().map(|f| f.field_size()), | ||
| Some(narrower), | ||
| "the declined descr must keep the width the codewriter asked for", | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn make_descr_from_bh_struct_array_preserves_type_and_interior_fields() { | ||
| use majit_ir::descr::ArrayFlag; | ||
|
|
@@ -4308,6 +4372,43 @@ pub fn make_descr_from_bh(bh: &majit_translate::jitcode::BhDescr) -> DescrRef { | |
| // (one skipped `list.pop(0)` per compiled loop entry). One field is | ||
| // one descr — `metainterp_sd.all_descrs` has no second entry for a | ||
| // field just because a different interpreter reached it. | ||
| // Same split, one struct up: the shared `PyObject` header's | ||
| // `w_class`. The walker pins a value's Python-level class by | ||
| // reading offset 8 through `w_class_descr()`; a codewriter-lowered | ||
| // body testing the same header — `is_plain_int1` reading | ||
| // `value.w_class` (listobject.rs) — reaches it through the modelled | ||
| // `PyObject` parent, whose group entry is a different identity for | ||
| // the same field. The pinned constant then never reached the | ||
| // second read, so the strict subclass test stayed symbolic and | ||
| // re-emitted the load plus its null and equality tests. | ||
| // | ||
| // Like the leaves below this runs BEFORE the parent-group lookup, | ||
| // which would otherwise answer with the parent's own entry. | ||
| // | ||
| // Only when the two spellings describe the same memory access. | ||
| // `new_w_class_field_descr` hardcodes `field_size: 8` while the | ||
| // codewriter sizes a pointer field by `layout::target_word_size()` | ||
| // (`call.rs get_type_flag`), so on wasm32 the incoming descr is a | ||
| // 4-byte load and the canonical one an 8-byte load at the same | ||
| // offset. Merging them there would widen the read over four bytes | ||
| // of the adjacent payload. That size split is deliberate and | ||
| // documented at `new_w_class_field_descr`; until it is resolved the | ||
| // bridge declines rather than papering over it, leaving those | ||
| // targets exactly as they were before the bridge existed. | ||
| if name.as_str() == "w_class" | ||
| && matches!( | ||
| owner.as_str(), | ||
| "PyObject" | "pyre_object::pyobject::PyObject" | ||
| ) | ||
| { | ||
| let canonical = &*W_CLASS_FIELD_DESCR; | ||
| if canonical.offset() == *offset | ||
| && canonical.field_size() == *field_size | ||
| && canonical.field_type() == *field_type | ||
| { | ||
| return w_class_descr(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a codewriter-lowered body stores AGENTS.md reference: AGENTS.md:L205-L207 Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
| if owner.as_str() == "W_ListObject" { | ||
| match name.as_str() { | ||
| "int_items.len" => return list_int_items_len_descr(), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover every bridge rejection guard.
The tests only reject a mismatched
field_size. Add rejection cases for a mismatchedoffsetandfield_type. Both cases must return a noncanonical descriptor. This protects the access-compatibility contract at Lines 4405-4407.🤖 Prompt for AI Agents