feat: JIT expansion — logical ops, 8-arg dispatch, tests#13
Conversation
- Fix And/Or from bitwise to logical semantics in Cranelift IR (2 && 3 was 2, now correctly 1) - Extend JIT dispatch from max 3 to 8 arguments for both i64 and f64 - Extract jit_call_i64/jit_call_f64 helpers for clean dispatch - Add 24 new JIT tests: logical ops, multi-arg, float arithmetic, recursive algorithms (fib30, GCD, Collatz), comparisons
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcd20e1010
ℹ️ 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".
| _ => panic!( | ||
| "JIT dispatch supports up to 8 arguments, got {}", | ||
| args.len() | ||
| ), |
There was a problem hiding this comment.
Avoid panicking on JIT calls with more than 8 args
This introduces a hard runtime panic for any JIT-dispatched call where args.len() > 8. In profiling/JIT mode, once a function is cached, call_value forwards the full argument vector into jit_call_i64/jit_call_f64, so valid Forge programs that call hot functions with 9+ arguments will crash instead of continuing in the interpreter path. Since function arity in bytecode is u8 and not capped at 8, this is a production-facing regression rather than just a test limitation.
Useful? React with 👍 / 👎.
humancto
left a comment
There was a problem hiding this comment.
Review: JIT expansion — logical ops, 8-arg dispatch, tests
Good PR overall. The And/Or fix is correct and important, the dispatch refactor is a clear improvement, and the test coverage is solid. A few items to address before merge.
What works well
- And/Or fix is correct. The old code did
band(l, r)/bor(l, r)on raw i64 values, so2 && 3returned2(bitwise AND). The new code normalizes both operands to 0/1 viaicmp(NotEqual, v, zero)first, which matches the VM bytecode interpreter's behavior (Value::Bool(left && right)atmachine.rs:958). Float path was already correct; this brings the integer path into alignment. - Dispatch refactor is clean. Extracting
jit_call_i64/jit_call_f64removes ~56 lines of duplicated inlinematchand extends support from 3 to 8 args. The old code silently dropped args beyond 3 viaraw_args.get(2).copied().unwrap_or(0)— that was a real silent-data-corruption bug. - Tests are well-structured. Good coverage of edge cases (falsy operands, boolean chains, float and/or, recursive algorithms). The recursive tests (fib(30), GCD, Collatz) are particularly good for exercising JIT correctness under non-trivial control flow.
Issue: panic! on >8 args with no compile-time guard
The IR builder (ir_builder.rs:26-29) creates Cranelift function signatures for any arity (from chunk.arity, a u8). But the dispatch functions panic at >8 args. This creates a gap: a function with 9+ args will be JIT-compiled successfully, then crash at call time.
The fix is straightforward — add an arity check in the auto-JIT gate at machine.rs:1556:
if !type_info.has_unsupported_ops && chunk.arity <= 8 {This way functions with >8 args simply won't be JIT-compiled and will fall through to the normal VM interpreter path. No panic, no silent failure.
CI: 2 of 9 checks failing
- Test (windows-latest): Failed. Worth checking if this is a pre-existing flake or related to this PR.
- Security Audit: Failed. Likely a pre-existing
cargo auditadvisory unrelated to this PR, but should be confirmed.
Minor observations (non-blocking)
-
No short-circuit evaluation: Both
&&and||evaluate both operands eagerly in the JIT, matching the VM bytecode interpreter. This is fine for the JIT's numeric-only context (no side effects), but worth noting in case this becomes relevant when string/object support is added later. -
Macro opportunity:
jit_call_i64andjit_call_f64are structurally identical (only the type parameter differs). A macro likedefine_jit_dispatch!(jit_call_i64, i64)could deduplicate. Non-blocking — the current form is readable. -
ROADMAP.md marking 3.6 as DONE: The roadmap originally listed "String operations, function calls" under 3.6. The PR description correctly notes string ops are deferred to Milestone 2, but the strikethrough on the heading could mislead someone into thinking the full scope is complete. Consider adding a
(partial)qualifier or keeping the heading un-struck with a status note.
Verdict
One real issue (the >8 arg panic path). The rest is solid work. Fix the arity guard, confirm the Windows CI failure isn't from this PR, and this is good to merge.
Generated by Claude Code
The JIT dispatch helpers (jit_call_i64/jit_call_f64) support up to 8 arguments but the IR builder places no arity limit, so a function with 9+ args would be JIT-compiled then panic at call time. Skip JIT for those functions so they fall through to the VM interpreter. https://claude.ai/code/session_01FejarfK8nU63ZKrcJiYURB
Summary
band/borto logical semantics in Cranelift IR —2 && 3now correctly returns1instead of2jit_call_i64/jit_call_f64helpersTest plan
cargo test)2 && 3 = 1,0 && 3 = 0,2 || 0 = 1sum4(1,2,3,4) = 10,sum6(1..6) = 21🤖 Generated with Claude Code