Tuple Support and WIP Object Support in JIT#8188
Tuple Support and WIP Object Support in JIT#8188LuigiTheJokesterAlvarez wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThe JIT adds heap-backed object values for tuples, with new shape metadata, allocator wiring, ABI support, and updated tuple build/unpack and variable conversion paths. ChangesHeap-backed JIT Tuple Objects
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 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
| // single abi type | ||
| if self.sig.ret.is_none() { | ||
| self.sig.ret = Some(ret_ty.clone()); | ||
| self.builder.func.signature | ||
| .returns | ||
| .push(AbiParam::new(ret_ty.to_cranelift())); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Keep return signatures consistent before emitting returns.
build_function declares the Cranelift function before compilation, but this block mutates builder.func.signature.returns later and also allows later returns with a different ret_ty. For inferred returns, this can leave the declared function/call refs with the wrong ABI; for mixed branches, callers may treat an i64 int as an object pointer.
🤖 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 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.
| 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 | |||
2 participants
Summary
This changes the JIT to support tuples and possibly, objects
Summary by CodeRabbit
New Features
Bug Fixes