Tuple Support and WIP Object Support in JIT#8188
Tuple Support and WIP Object Support in JIT#8188LuigiTheJokesterAlvarez wants to merge 12 commits into
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:
📝 WalkthroughWalkthroughThe JIT now treats tuples as heap-backed object values, threads a tuple allocator through compilation, and updates ABI, local-variable handling, tuple construction, and unpacking to use the new object shape. JIT tuple annotations and snippet coverage were also extended. ChangesHeap-backed JIT Tuple Objects
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Jit
participant build_function
participant FunctionCompiler
participant jit_alloc_tuple
participant AbiValue
participant libffi
Jit->>Jit: import jit_alloc_tuple
build_function->>Jit: declare alloc_func_ref
build_function->>FunctionCompiler: new(..., alloc_func_ref)
FunctionCompiler->>jit_alloc_tuple: call(len)
FunctionCompiler->>AbiValue: create Object(usize)
AbiValue->>libffi: marshal pointer argument
libffi->>AbiValue: return object payload
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/jit/src/instructions.rs (1)
55-60: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve tuple shape for object arguments before supporting tuple unpacking there.
JitType::Objectarguments are materialized asObjectKind::Opaque, butUnpackSequenceonly acceptsObjectKind::Tuple. A JIT function taking a tuple argument and unpacking it will reject its own object ABI value despite the new tuple support.Also applies to: 878-884
🤖 Prompt for 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. In `@crates/jit/src/instructions.rs` around lines 55 - 60, In from_type_and_value, JitType::Object is currently always wrapped as ObjectKind::Opaque, which breaks tuple arguments that later flow into UnpackSequence. Update the object-argument materialization path in from_type_and_value so tuple-shaped values are preserved as ObjectKind::Tuple when the ABI value represents a tuple, and keep UnpackSequence aligned to accept that preserved shape instead of rejecting the function’s own argument value.
🤖 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 `@crates/jit/src/instructions.rs`:
- Around line 155-161: The changed Rust code needs rustfmt cleanup, with several
signatures and calls in functions like local_to_value and other touched Rust
sections needing standard formatting and trailing whitespace removed. Run cargo
fmt on the affected code so the updated blocks follow the default Rust style
consistently across the modified instructions and nearby changed lines.
- Around line 429-435: The return ABI handling in build_function/instructions.rs
is being mutated too late and can diverge across branches, so make the return
signature fixed before any return emission and enforce that all subsequent
returns match the first inferred ret_ty. Update the logic around self.sig.ret
and builder.func.signature.returns so the signature is established once, then
validate or reject mixed return types in the return-building path rather than
appending/changing ABI params later.
In `@crates/jit/src/lib.rs`:
- Around line 41-50: Add a matching deallocation path for the heap tuples
allocated by jit_alloc_tuple, since BuildTuple and tuple constants currently
return raw allocations with no owner or free. Introduce a clear ownership
strategy in crates/jit/src/lib.rs, and ensure the JIT runtime exposes a
corresponding free/release function or otherwise transfers ownership so tuple
allocations are reclaimed after use.
---
Outside diff comments:
In `@crates/jit/src/instructions.rs`:
- Around line 55-60: In from_type_and_value, JitType::Object is currently always
wrapped as ObjectKind::Opaque, which breaks tuple arguments that later flow into
UnpackSequence. Update the object-argument materialization path in
from_type_and_value so tuple-shaped values are preserved as ObjectKind::Tuple
when the ABI value represents a tuple, and keep UnpackSequence aligned to accept
that preserved shape instead of rejecting the function’s own argument value.
🪄 Autofix (Beta)
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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e9eeb398-7e6d-4522-bc07-630d242533be
📒 Files selected for processing (3)
crates/jit/src/instructions.rscrates/jit/src/lib.rscrates/vm/src/builtins/function.rs
| extern "C" fn jit_alloc_tuple(len: i64) -> i64 { | ||
| let layout = Layout::array::<i64>(len as usize + 1).unwrap(); | ||
| let ptr = unsafe { alloc(layout) }; | ||
| if ptr.is_null() { | ||
| handle_alloc_error(layout); | ||
| } | ||
| unsafe { | ||
| *(ptr as *mut i64) = len; | ||
| } | ||
| ptr as i64 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add an ownership/freeing strategy for heap tuples.
jit_alloc_tuple allocates with the global allocator and returns a raw pointer, but this PR adds no matching deallocation path. Since BuildTuple and tuple constants call this allocator at runtime, repeated JIT invocations leak every tuple allocation.
🤖 Prompt for 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.
In `@crates/jit/src/lib.rs` around lines 41 - 50, Add a matching deallocation path
for the heap tuples allocated by jit_alloc_tuple, since BuildTuple and tuple
constants currently return raw allocations with no owner or free. Introduce a
clear ownership strategy in crates/jit/src/lib.rs, and ensure the JIT runtime
exposes a corresponding free/release function or otherwise transfers ownership
so tuple allocations are reclaimed after use.
ShaharNaveh
left a comment
ShaharNaveh
left a comment
There was a problem hiding this comment.
Hi, thanks for contributing to RustPython:)
Can you please check the AI policy? (#8188 (comment))
| @@ -1,6 +1,8 @@ | |||
| // instructions.rs | |||
There was a problem hiding this comment.
What's the reasoning for this comment?
There was a problem hiding this comment.
when i forget what im doing i comment it out to make myself know again what im doing
yeah it's weird
There was a problem hiding this comment.
ill delete unnecessary comments and if i can right now, document the functions i did, and making the changes coderabbit requested
There was a problem hiding this comment.
i did documentation yesterday lol
| @@ -1,7 +1,9 @@ | |||
| // lib.rs | |||
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/jit/src/instructions.rs (1)
382-406: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftHeap tuple allocations need a reclamation path
crates/jit/src/instructions.rs:382-406
build_heap_tupleallocates raw memory viajit_alloc_tuple, but nothing in the JIT frees those buffers.CompiledCode::droponly releases the compiled module, not tuple storage. Since bothLoadConsttuples andBuildTuplehit this path, repeated execution will leak one allocation per tuple unless ownership is transferred to the VM or a matching free path is added.🤖 Prompt for 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. In `@crates/jit/src/instructions.rs` around lines 382 - 406, The heap tuple allocation in build_heap_tuple currently has no matching reclamation path, so tuple buffers allocated through alloc_func will leak across repeated execution. Update the JIT tuple lifecycle around build_heap_tuple, TupleShape, and CompiledCode::drop so ownership of these allocations is clearly transferred to the VM or a corresponding free is registered and invoked when the compiled result is released. Make sure both LoadConst tuples and BuildTuple allocations are covered by the same cleanup path.
🧹 Nitpick comments (2)
crates/vm/src/builtins/function/jit.rs (1)
55-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winError message is now stale — tuple is accepted but not mentioned.
The error message still says "Jit requires argument to be either int, float or bool"-style text, but the branch above now also accepts tuples. Update the message so it doesn't mislead users about supported argument types.
📝 Proposed fix
Err(new_jit_error( - "Jit requires argument to be either int, float or bool".to_owned(), + "Jit requires argument to be either int, float, bool or tuple".to_owned(), vm, ))🤖 Prompt for 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. In `@crates/vm/src/builtins/function/jit.rs` around lines 55 - 63, The fallback error in JIT argument validation is stale because the `jit` type check now also accepts tuples via `value.is(vm.ctx.types.tuple_type)`. Update the `new_jit_error` message in `jit.rs` to include tuple among the supported argument types, keeping the message aligned with the branches in the JIT type-detection logic.crates/jit/src/instructions.rs (1)
616-622: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeduplicate the tuple
JitValue::Objectconstruction. TheJitValue::Object(ptr, Rc::new(ObjectKind::Tuple(shape)))wrapping is identical here and inprepare_const(Lines 374-376). Consider havingbuild_heap_tuplereturn the finishedJitValue(or a small helper) so the wrapping lives in one place. As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."🤖 Prompt for 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. In `@crates/jit/src/instructions.rs` around lines 616 - 622, The tuple JitValue::Object construction is duplicated in Instruction::BuildTuple and prepare_const, so move the wrapping into one shared place. Update build_heap_tuple to return the finished JitValue, or add a small helper used by both build_heap_tuple and prepare_const, and have Instruction::BuildTuple and prepare_const call that shared logic instead of rebuilding JitValue::Object(ptr, Rc::new(ObjectKind::Tuple(shape))) in multiple places.Source: Coding guidelines
🤖 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 `@crates/vm/src/builtins/function/jit.rs`:
- Around line 57-58: The conditional in the JIT function implementation is
formatted incorrectly, with the closing brace and else split across separate
lines. Update the `jit.rs` block around the `Function`/`jit` logic so it follows
rustfmt style and uses a single-line `} else {` form consistent with `cargo
fmt`.
---
Outside diff comments:
In `@crates/jit/src/instructions.rs`:
- Around line 382-406: The heap tuple allocation in build_heap_tuple currently
has no matching reclamation path, so tuple buffers allocated through alloc_func
will leak across repeated execution. Update the JIT tuple lifecycle around
build_heap_tuple, TupleShape, and CompiledCode::drop so ownership of these
allocations is clearly transferred to the VM or a corresponding free is
registered and invoked when the compiled result is released. Make sure both
LoadConst tuples and BuildTuple allocations are covered by the same cleanup
path.
---
Nitpick comments:
In `@crates/jit/src/instructions.rs`:
- Around line 616-622: The tuple JitValue::Object construction is duplicated in
Instruction::BuildTuple and prepare_const, so move the wrapping into one shared
place. Update build_heap_tuple to return the finished JitValue, or add a small
helper used by both build_heap_tuple and prepare_const, and have
Instruction::BuildTuple and prepare_const call that shared logic instead of
rebuilding JitValue::Object(ptr, Rc::new(ObjectKind::Tuple(shape))) in multiple
places.
In `@crates/vm/src/builtins/function/jit.rs`:
- Around line 55-63: The fallback error in JIT argument validation is stale
because the `jit` type check now also accepts tuples via
`value.is(vm.ctx.types.tuple_type)`. Update the `new_jit_error` message in
`jit.rs` to include tuple among the supported argument types, keeping the
message aligned with the branches in the JIT type-detection logic.
🪄 Autofix (Beta)
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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e4d2b7ab-8b30-431a-b98b-975a72432d63
📒 Files selected for processing (3)
crates/jit/src/instructions.rscrates/jit/src/lib.rscrates/vm/src/builtins/function/jit.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/jit/src/lib.rs
|
I will fix the leak with tuple alloc later |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extra_tests/snippets/jit.py (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVariable name violates PEP 8 (snake_case).
tupleIdshould betuple_idper PEP 8 naming conventions for local variables.As per coding guidelines,
**/*.py: "Follow PEP 8 style for custom Python code".🎨 Proposed fix
- tupleId = (1, 2) - assert tuple_identity(tupleId) == tupleId + tuple_id = (1, 2) + assert tuple_identity(tuple_id) == tuple_id🤖 Prompt for 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. In `@extra_tests/snippets/jit.py` at line 24, Rename the local variable tupleId to tuple_id in jit.py to comply with PEP 8 snake_case naming; update any nearby references in the same snippet or function so the new identifier is used consistently.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@extra_tests/snippets/jit.py`:
- Line 24: Rename the local variable tupleId to tuple_id in jit.py to comply
with PEP 8 snake_case naming; update any nearby references in the same snippet
or function so the new identifier is used consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 01ba0ae0-098c-406f-9653-2d60610b3f24
📒 Files selected for processing (2)
crates/vm/src/builtins/function/jit.rsextra_tests/snippets/jit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/builtins/function/jit.rs
|
I have temporarily disabled the while-loop JIT test due to incorrect loop lowering causing infinite execution, i will re-enable it once SSA φ-node based loop construction is implemented |
2 participants
Summary
This changes the JIT to support tuples and possibly, objects
Summary by CodeRabbit
New Features
tuple, mapping it to the new representation.Bug Fixes
Tests
tuple_identityand expanded JIT snippet coverage with explicit tuple-related type annotations.