Preserve pointer identity for float NaNs - #1144
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFloat NaNs now use address identity and remain boxed or unspecialized where required. Complex values use pointer identity. Float-list matching uses direct equality. Tuple and list specializations exclude NaNs and float subclasses while finite exact floats retain specialized storage paths. ChangesIdentity semantics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PythonObject
participant FloatStrategy
participant JITTrace
participant BoxedStorage
PythonObject->>FloatStrategy: classify float value
FloatStrategy-->>JITTrace: accept finite exact float
JITTrace->>JITTrace: apply exact-class and non-NaN guards
JITTrace->>BoxedStorage: store NaN or subclass as original object
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 7cbda57). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cbda573ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if one.is_nan() || two.is_nan() { | ||
| return false; |
There was a problem hiding this comment.
Once NaNs fall back to pointer identity here, every path that erases the original W_FloatObject has to reject them, not just FloatListStrategy. makespecialisedtuple2 still builds W_SpecialisedTupleObject_ff for exact NaNs and mapdict still picks UnboxType::Float; both store only the raw f64 and rebox on read, so cases like n = float('nan'); t = (n, n); t[0] is n or a NaN instance attribute now become false / get fresh id() values even though Python attribute and tuple storage should retain the original object. Please apply the same NaN exclusion to those unboxed float paths before switching is_w/id to address identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/7cbda573ead44617d1086feec3ee2fdf5094292f/pyre-interpreter/src/baseobjspace.rs#L4149-L4150
When a hot loop creates an empty list and appends a NaN, this pointer-identity rule requires retaining the original float object, but try_walker_specialize_list_append in pyre-jit-trace/src/jitcode_dispatch/specialize.rs:9880-9913 still classifies every exact float as ListStrategy::Float and emits unboxed storage, while the concrete runtime classifier rejects NaNs. The compiled path can therefore rebox the value and make items[0] is nan false (or diverge from its Object-strategy shadow). Fresh evidence beyond the existing tuple/mapdict comment is this separate hot-append classifier, which also needs the !is_nan() gate.
AGENTS.md reference: AGENTS.md:L14-L20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7cbda57 to
19a0772
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a077278d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if unsafe { pyre_object::w_float_get_value(w_value) }.is_nan() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This runtime NaN guard is not mirrored in the STORE_ATTR JIT fold: try_walker_specialize_store_attr still accepts an existing unboxed-float slot with only is_float(concrete_value) before emitting jit_mapdict_unboxed_write_f, so a traced c.x = nan after c.x was initialized with a finite float writes raw NaN bits instead of taking plain_direct_write's conversion-to-boxed path. Fresh evidence is that the fold's value gate is still is_float, not this value_has_unbox_type predicate; future reads then rebox the slot, making c.x is nan and id(c.x) diverge under JIT.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
19a0772 to
aabf2a8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aabf2a8af9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// `item` must be null or point to a live object. | ||
| #[inline] | ||
| unsafe fn is_float_strategy_item(item: PyObjectRef) -> bool { | ||
| pub unsafe fn is_float_strategy_item(item: PyObjectRef) -> bool { |
There was a problem hiding this comment.
With this patch's NaN pointer identity, rejecting NaNs from Float storage becomes semantic, but the traced STORE_SUBSCR paths still do not share this exported predicate: trace_helpers/typed_trace.rs::detect_list_setitem_strategy and specialize.rs::try_walker_specialize_store_subscr both gate float-list writes with is_float before emitting a raw float-block store. In a traced lst = [1.0]; n = float('nan'); lst[0] = n, the interpreter converts the list to Object storage, while the compiled path leaves a Float list containing only the NaN bits, so the next read reboxes and lst[0] is n/id(lst[0]) == id(n) diverge. Please route those setitem gates through is_float_strategy_item as well.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-object/src/tupleobject.rs`:
- Around line 492-499: Validate the makespecialisedtuple2 JIT-visible change by
running cargo check --features dynasm and cargo test --features dynasm, then run
all eight benchmarks and investigate and explain any performance regressions
before committing.
- Around line 509-527: Add regression tests covering NaN operands through
makespecialisedtuple2 and w_tuple_new. Assert the resulting tuple does not use
the _ff specialization and that w_tuple_getitem returns the original NaN
PyObjectRef, while preserving the existing finite-pair test to verify the direct
_ff path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 584d9d2a-b13a-4c4e-80f4-ed91014b004e
📒 Files selected for processing (2)
pyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-object/src/tupleobject.rs
aabf2a8 to
6aa3d35
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
6aa3d35 to
8f4208a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Line 4020: Add a walker_guard_exact_w_class check for the canonical FLOAT_TYPE
immediately before walker_guard_float_not_nan in the relevant specialization
path. Ensure the guard’s side exit uses the generic mapdict write and performs
boxed-storage conversion for float subclasses, preserving is_unboxable_float
requirements.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a24cc04-bdd4-4cdd-a6e1-d98c92395d25
📒 Files selected for processing (7)
pyre/extra_tests/parity_tests/bool_text_signatures_python314.pypyre/extra_tests/parity_tests/float_subclass_unboxed_storage.pypyre/extra_tests/parity_tests/nan_unboxed_storage_identity.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/trace_helpers/typed_trace.rs
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/b8f948290dc3bd1590b992648bb7c6babf9c591f/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L4016-L4020
When a trace is recorded for an exact finite-float assignment to an existing unboxed mapdict slot, this path declines float subclasses only at record time. On replay, walker_unbox_float guards only ob_type == FLOAT_TYPE, and a float subclass shares that ob_type while carrying a different w_class; the new non-NaN guard still passes for finite subclass instances, so jit_mapdict_unboxed_write_f stores raw f64 instead of taking _direct_write's convert-to-boxed path. A later c.x reboxes as an exact float, making c.x is subclass_value and type(c.x) wrong; add the same w_class pin used by the list float-store paths before emitting the raw write.
AGENTS.md reference: AGENTS.md:L14-L19
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Assisted-by: codex-5.6-sol
Assisted-By: Claude Opus 5
b8f9482 to
483c426
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/483c426dd7c63256e01e8c594a873fbab9f836d2/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L4020
When an existing mapdict slot is unboxed-float, this added NaN guard still only protects the raw payload after walker_unbox_float, whose replay guard pins ob_type but not the Python w_class. In a traced loop like obj.x = v where the trace is recorded with exact finite floats and later v is a finite float subclass, the subclass shares FLOAT_TYPE, passes raw != raw as non-NaN, and jit_mapdict_unboxed_write_f stores only the f64; the interpreter path now says subclasses convert the slot to boxed storage, so a later obj.x is v/type(obj.x) diverges. Please add the same exact-w_class guard used by the list float paths before taking this helper.
AGENTS.md reference: AGENTS.md:L14-L19
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Cherry-picked from #1144, minus two hunks. The `pub(crate)` bump on `walker_exact_builtin_class` is dropped: `specialize` is a child module of `jitcode_dispatch`, so the private declaration is already in scope at every call site. The `trace_helpers/typed_trace.rs` hunk is dropped with the file, which #1318 deleted. Assisted-By: Claude Opus 5
Cherry-picked from #1144, minus two hunks. The `pub(crate)` bump on `walker_exact_builtin_class` is dropped: `specialize` is a child module of `jitcode_dispatch`, so the private declaration is already in scope at every call site. The `trace_helpers/typed_trace.rs` hunk is dropped with the file, which #1318 deleted. Assisted-By: Claude Opus 5
1 participant
Assisted-by: codex-5.6-sol
Summary
The NaN comparison cases in builtin_tuple, builtin_list, and builtin_slice exposed that pyre was still inheriting PyPy's value-identity behavior.
Align
is_w/ identity semantics for NaN floats and complex numbers with Python 3.14 pointer-identity behavior.is_w: NaN is never identical to a distinct object; finite floats retain bit-pattern identity (unboxed in FloatListStrategy, reboxed on read).is_w: dropped the complex branch entirely — nothing stores complex unboxed, so pointer identity is free and matches 3.14 forcomplex(1,2) is complex(1,2).immutable_unique_id: no value-derived id for NaN floats or any complex, preservingx is y <=> id(x) == id(y);hash()follows for free.Prevented
NaNs from entering identity-erasing unboxed storage inFloatListStrategy, specialized float tuples, andmapdictattributes, preserving the original object across reads.Self-review
Assisted-bySummary by CodeRabbit
Bug Fixes
floatsubclasses during optimized storage and JIT execution.Tests