refactor(interp): unify plan-based JIT compilation#139
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 PR introduces plan-based static and trace JIT compilation, unified compiler orchestration, ARM64 lowering changes, atomic native function-entry publication, updated runtime tests, and documentation describing the revised JIT architecture. ChangesPlan-based unified JIT
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Sequence Diagram(s)sequenceDiagram
participant Interpreter
participant compiler as compiler.Compile
participant planner as staticPlanner or tracePlanner
participant lowerer as arm64Lowerer
participant Callable
participant natives as Interpreter.natives
Interpreter->>compiler: request compilation
compiler->>planner: create plan
planner-->>compiler: validated plan
compiler->>lowerer: lower plan
lowerer-->>compiler: assembler output
compiler-->>Interpreter: compiled Callable
Interpreter->>Callable: Addr()
Interpreter->>natives: atomic.StorePointer function entry
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
interp/cfg.go (2)
46-50: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
funcsmap is rebuilt from scratch on everycompileCFGcall.This scans all of
i.instrsand callsi.functionfor each entry every timecompileCFGis invoked. If baseline CFG tiering retries this for functions that repeatedly failblockHeights/blockKindsstructural checks, this becomes redundant work on a per-attempt basis. Consider caching this address→function map once (e.g., on theInterpreterorcompiler) and reusing it across compile attempts.🤖 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 `@interp/cfg.go` around lines 46 - 50, Cache the address-to-function map built around the compileCFG flow instead of reconstructing it on every compileCFG call. Add storage on the Interpreter or compiler, populate it once by scanning instrs and resolving with function, then reuse the cached map across baseline CFG retries while preserving the existing function lookup behavior.
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnalysis-pass error is unconditionally swallowed as a clean rejection.
Any error from
pass.GetResult— not just structural CFG issues — is treated identically to "can't compile this shape" and returnsnilerror. If the pass manager ever fails for a non-structural reason (e.g., pass wiring bug), this would be silently indistinguishable from a normal fallback, per thenilerrstatic-analysis hint.🤖 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 `@interp/cfg.go` around lines 24 - 29, Update the error handling after pass.GetResult in the CFG construction flow to propagate unexpected analysis-pass failures instead of returning a clean rejection. Preserve the existing fallback only for the specific structural CFG failure case, using the available error type or classification to distinguish it from other errors.Source: Linters/SAST tools
🤖 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 `@asm/arch.go`:
- Around line 86-87: Remove Addr() from the asm.Callable interface to preserve
compatibility with existing downstream implementations, or define it in a
separate optional interface and update callers to use that interface when
needed. If retaining it on Callable is required, mark this change for a breaking
release.
In `@docs/benchmarks.md`:
- Around line 199-202: Add the missing BenchmarkJITIssue101 provenance reference
in the benchmark section, using the exact command or a direct link to the
benchmark source. Keep the existing performance results and explanatory text
unchanged.
In `@interp/interp_test.go`:
- Around line 4091-4093: Update the negative-id early-return guards in the
relevant interpreter tests to use a CFG-success signal rather than
vm_jit_cfg_attempts_total. Apply the same correction at all three reported
cases, or make those cases explicitly trace-only, so rejected CFG attempts still
allow trace compilation and missing learned continuations are not hidden.
In `@interp/jit_arm64_cfg.go`:
- Around line 290-298: Audit the immediate-range handling in the JIT code around
ctx.sp(), params, and localKinds, confirming whether arm64.ADDI, arm64.SUBI, and
arm64.STR validate immediates and expose errors through Build(). If they do not,
add explicit bounds checks before every int16/uint16 narrowing and return the
established fallback/error path, matching the op.callee guard; if they do,
ensure Build() propagates the distinct assembler error alongside existing errors
such as ErrNoRegistersAvailable and ErrBranchOutOfRange.
- Around line 425-444: Update cfgIf to preserve cond.reg before calling flush,
matching cfgTable’s fresh-register handling, or emit the conditional branch
before flushing. Ensure subsequent register reuse cannot change the value used
by CBNZLabel to select the correct edge.
---
Nitpick comments:
In `@interp/cfg.go`:
- Around line 46-50: Cache the address-to-function map built around the
compileCFG flow instead of reconstructing it on every compileCFG call. Add
storage on the Interpreter or compiler, populate it once by scanning instrs and
resolving with function, then reuse the cached map across baseline CFG retries
while preserving the existing function lookup behavior.
- Around line 24-29: Update the error handling after pass.GetResult in the CFG
construction flow to propagate unexpected analysis-pass failures instead of
returning a clean rejection. Preserve the existing fallback only for the
specific structural CFG failure case, using the available error type or
classification to distinguish it from other errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90aed18f-76cf-47b9-86de-512167674151
📒 Files selected for processing (15)
asm/arch.goasm/arm64/abi.godocs/architecture.mddocs/benchmarks.mddocs/jit-internals.mddocs/profile.mdinterp/blockflow.gointerp/blockflow_test.gointerp/cfg.gointerp/cfgflow.gointerp/interp.gointerp/interp_test.gointerp/jit.gointerp/jit_arm64_cfg.gointerp/jit_arm64_cfg_test.go
| // Addr returns the immutable executable entry address. | ||
| Addr() unsafe.Pointer |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and find interface implementations/usages.
git ls-files asm/arch.go asm | sed -n '1,120p'
printf '\n--- outline asm/arch.go ---\n'
ast-grep outline asm/arch.go --view expanded || true
printf '\n--- search for Callable implementations/usages ---\n'
rg -n "type .*Callable|interface .*Callable|Addr\(\) unsafe.Pointer|Callable" asm -g '*.go' || trueRepository: siyul-park/minivm
Length of output: 4207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant section of asm/arch.go with line numbers.
wc -l asm/arch.go
sed -n '1,180p' asm/arch.go | cat -nRepository: siyul-park/minivm
Length of output: 4684
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the added method is used by any concrete types in this repo.
rg -n "Addr\(\) unsafe.Pointer" -g '*.go' .
printf '\n---\n'
rg -n "implements|satisfy|Callable" -g '*.go' .Repository: siyul-park/minivm
Length of output: 3971
Adding Addr() to asm.Callable is a breaking change. Existing downstream implementations will stop compiling until they add the new method. If compatibility matters, move it to a separate optional interface or ship this as a breaking release.
🤖 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 `@asm/arch.go` around lines 86 - 87, Remove Addr() from the asm.Callable
interface to preserve compatibility with existing downstream implementations, or
define it in a separate optional interface and update callers to use that
interface when needed. If retaining it on Callable is required, mark this change
for a breaking release.
| | `branchy_batch_tree_evaluation` | threaded | 1,568 | 0 | 0 | | ||
| | `branchy_batch_tree_evaluation` | baseline CFG tier | 1,386 | 0 | 0 | | ||
|
|
||
| The JIT row stays faster by keeping warm-entry fallback checks dense and skipping no-op safepoints when no context cancellation, fuel, hook, profiler, or shared cache coordination is active. | ||
| The whole-CFG tier is about 12% faster on this workload while preserving zero steady-state allocations. The remaining gap to native tree evaluators is dominated by per-function call and canonical stack traffic rather than path-dependent trace warmup. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section and any nearby reproducibility/provenance guidance.
sed -n '170,230p' docs/benchmarks.md
printf '\n--- SEARCH ---\n'
rg -n "ARM64|Go version|BenchmarkJITIssue101|provenance|reproduc|benchmark" docs/benchmarks.mdRepository: siyul-park/minivm
Length of output: 6481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the doc structure to find if provenance is documented elsewhere.
wc -l docs/benchmarks.md
printf '\n--- OUTLINE ---\n'
ast-grep outline docs/benchmarks.md --view expanded || trueRepository: siyul-park/minivm
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,60p' docs/benchmarks.mdRepository: siyul-park/minivm
Length of output: 2343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "BenchmarkJITIssue101|branchy_batch_tree_evaluation|Issue101" -S .Repository: siyul-park/minivm
Length of output: 875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the benchmark declaration and any command snippets in the benchmark module docs/tests.
git ls-files | rg '(^|/)(.*bench.*|.*test.*|docs/benchmarks\.md)$'Repository: siyul-park/minivm
Length of output: 1725
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the benchmark implementation for any embedded invocation/provenance details.
sed -n '1,140p' benchmarks/jit_issue101_test.goRepository: siyul-park/minivm
Length of output: 3366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any exact command examples for BenchmarkJITIssue101 or nearby provenance notes.
rg -n 'go test .*BenchmarkJITIssue101|BenchmarkJITIssue101|benchmem|benchtime=2s' docs benchmarks -SRepository: siyul-park/minivm
Length of output: 1605
Add the missing BenchmarkJITIssue101 provenance link
The ARM64 platform and Go version are already documented in the Environment section; add the exact BenchmarkJITIssue101 command here, or link directly to the benchmark source/provenance.
🤖 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 `@docs/benchmarks.md` around lines 199 - 202, Add the missing
BenchmarkJITIssue101 provenance reference in the benchmark section, using the
exact command or a direct link to the benchmark source. Keep the existing
performance results and explanatory text unchanged.
| a := ctx.assembler | ||
| vCtrl := ctx.pin(scratchCtrl) | ||
| natives := a.Reg(asm.RegTypeInt, asm.Width64) | ||
| a.Emit(arm64.LDR(natives, vCtrl, int16(journalNatives*8))) | ||
| callee := a.Reg(asm.RegTypeInt, asm.Width64) | ||
| if op.callee > 4095 { | ||
| return false | ||
| } | ||
| a.Emit(arm64.LDR(callee, natives, int16(op.callee*8))) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded int16/uint16 narrowing of runtime-derived offsets (ctx.sp(), params, len(localKinds)).
Line 295-297 explicitly guards op.callee > 4095 before narrowing op.callee*8 to int16, but the same class of narrowing at lines 327 (uint16(ctx.sp())), 336 (uint16(params)), 350 (int16((ctx.sp()-params+idx)*8)), and 356 (uint16(len(localKinds))) has no equivalent bound check. For a function with deep stack usage or many locals, these could silently wrap/truncate into an incorrect immediate, producing miscompiled native code (wrong stack offsets) rather than a clean fallback.
Please confirm whether arm64.ADDI/arm64.SUBI/arm64.STR validate their immediate range internally and surface a distinct asm error (which Build() would then need to handle, similar to ErrNoRegistersAvailable/ErrBranchOutOfRange), or whether callers are expected to self-guard as done for op.callee.
Also applies to: 325-356
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 292-292: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int16(journalNatives*8)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 297-297: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int16(op.callee*8)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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 `@interp/jit_arm64_cfg.go` around lines 290 - 298, Audit the immediate-range
handling in the JIT code around ctx.sp(), params, and localKinds, confirming
whether arm64.ADDI, arm64.SUBI, and arm64.STR validate immediates and expose
errors through Build(). If they do not, add explicit bounds checks before every
int16/uint16 narrowing and return the established fallback/error path, matching
the op.callee guard; if they do, ensure Build() propagates the distinct
assembler error alongside existing errors such as ErrNoRegistersAvailable and
ErrBranchOutOfRange.
| func (l arm64Lowerer) cfgIf(ctx *lowering, op step, from, nextIP int, labels []asm.Label, index map[int]int) bool { | ||
| if ctx.count() < 1 || !l.kinds(ctx, types.KindI32, 1) { | ||
| return false | ||
| } | ||
| cond := ctx.pop() | ||
| if !l.flush(ctx, false) { | ||
| return false | ||
| } | ||
| targets := instr.Targets(ctx.frame().code, op.ip) | ||
| if len(targets) != 1 { | ||
| return false | ||
| } | ||
| taken := ctx.assembler.Label() | ||
| ctx.assembler.Emit(arm64.CBNZLabel(l.narrow32(cond.reg), taken)) | ||
| if !l.cfgEdge(ctx, nextIP, from, labels, index) { | ||
| return false | ||
| } | ||
| ctx.assembler.Bind(taken) | ||
| return l.cfgEdge(ctx, targets[0], from, labels, index) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo files of interest:\n'
git ls-files 'interp/*.go' 'interp/**/*.go' | sed -n '1,120p'
printf '\nOutline interp/jit_arm64.go:\n'
ast-grep outline interp/jit_arm64.go --view expanded || true
printf '\nOutline interp/jit_arm64_cfg.go:\n'
ast-grep outline interp/jit_arm64_cfg.go --view expanded || true
printf '\nSearch for flush definition and register allocation helpers:\n'
rg -n "func .*flush|type lowering|ctx\.pop\(|ctx\.values|assembler\.Reg\(|Label\(|Bind\(" interp/jit_arm64.go interp/jit_arm64_cfg.go interp -g '!**/*_test.go'Repository: siyul-park/minivm
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant slices once file sizes are known.
wc -l interp/jit_arm64.go interp/jit_arm64_cfg.go || true
printf '\nRelevant excerpts from interp/jit_arm64_cfg.go:\n'
sed -n '240,380p' interp/jit_arm64_cfg.go
printf '\nRelevant excerpts around cfgIf/cfgTable:\n'
sed -n '420,490p' interp/jit_arm64_cfg.go
printf '\nFlush-related definitions in interp/jit_arm64.go:\n'
rg -n "func .*flush|type lowering|func .*narrow32|func .*kinds|func .*cfgEdge|func .*pop|func .*sp\(" interp/jit_arm64.go interp -g '!**/*_test.go'Repository: siyul-park/minivm
Length of output: 8288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate any register reuse / stack allocation logic in the JIT lowering.
rg -n "free.*reg|reuse.*reg|allocate.*reg|spill|clobber|values\s*\[\]|ctx\.values|RegTypeInt|Width64" interp/jit_arm64.go interp/jit_arm64_cfg.go interp -g '!**/*_test.go'Repository: siyul-park/minivm
Length of output: 48740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact regions with line numbers.
cat -n interp/jit_arm64_cfg.go | sed -n '250,380p'
printf '\n---\n'
cat -n interp/jit_arm64_cfg.go | sed -n '420,490p'
printf '\n---\n'
cat -n interp/jit_arm64.go | sed -n '1,260p'Repository: siyul-park/minivm
Length of output: 17425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the lowering and flush implementation.
sed -n '1,260p' interp/jit_arm64.go
printf '\n---\n'
sed -n '260,420p' interp/jit_arm64.go
printf '\n---\n'
sed -n '420,520p' interp/jit_arm64_cfg.goRepository: siyul-park/minivm
Length of output: 15756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate cfgTable and flush precisely.
rg -n -A 30 -B 10 "func \(l arm64Lowerer\) cfgTable|func \(l arm64Lowerer\) cfgIf|func \(l arm64Lowerer\) flush" interp/jit_arm64_cfg.go interp/jit_arm64.goRepository: siyul-park/minivm
Length of output: 5699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the boxing helper and the lowering value/register model.
sed -n '3440,3715p' interp/jit_arm64.go
printf '\n---\n'
sed -n '48,180p' interp/jit.go
printf '\n---\n'
sed -n '400,490p' interp/jit.goRepository: siyul-park/minivm
Length of output: 14715
Preserve cond.reg before flush cfgTable copies the branch value into a fresh register first; cfgIf should do the same or emit the branch before flush, otherwise a reused temp can drive the wrong edge.
🤖 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 `@interp/jit_arm64_cfg.go` around lines 425 - 444, Update cfgIf to preserve
cond.reg before calling flush, matching cfgTable’s fresh-register handling, or
emit the conditional branch before flushing. Ensure subsequent register reuse
cannot change the value used by CBNZLabel to select the correct edge.
|
Follow-up bottleneck analysis and improvement:
Apple M4 Pro, darwin/arm64:
Verification:
Commit: b429f4d |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@interp/interp_test.go`:
- Around line 3229-3234: Update the five assertion branches in interp_test.go
around the relevant JIT emission checks to determine CFG success using
vm_jit_cfg_attempts_total > 0 together with vm_jit_cfg_rejected_total == 0,
matching the existing top-level typed-array loop test. Replace attempts-only
branching while preserving each location’s current expected emission thresholds
and fallback behavior.
In `@interp/jit_arm64_cfg.go`:
- Around line 261-282: Update cfgConstGet to treat heap index 0 as the valid
Null sentinel by rejecting only negative ref values. Preserve the existing
bounds check and allow ref == 0 to reach the heap type switch and fallback to
l.constGet, matching the behavior of constGet in the ARM64 interpreter.
- Around line 253-259: Update cfgArrayGet in jit_arm64_cfg.go to handle
types.TypedArray[int64] consistently with cfgflow.go’s types.KindI64 behavior.
Add the appropriate int64 array lowering case, or route it through the exact-IP
fallback, so valid ARRAY_GET operations do not reach the default failure path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63e463d7-5f03-4fc8-847a-a3996821ad4e
📒 Files selected for processing (10)
docs/benchmarks.mddocs/jit-internals.mdinterp/cfg.gointerp/cfgflow.gointerp/interp.gointerp/interp_test.gointerp/jit.gointerp/jit_arm64.gointerp/jit_arm64_cfg.gointerp/pool_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/benchmarks.md
🚧 Files skipped from review as they are similar to previous changes (5)
- interp/jit.go
- interp/cfg.go
- interp/interp.go
- docs/jit-internals.md
- interp/cfgflow.go
| emits := i.samples.Value("vm_jit_emits_total") | ||
| if i.samples.Value("vm_jit_cfg_attempts_total") > 0 { | ||
| require.GreaterOrEqual(t, emits, float64(1)) | ||
| } else { | ||
| require.GreaterOrEqual(t, emits, float64(2)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Gate on CFG success, not vm_jit_cfg_attempts_total, across all five spots.
vm_jit_cfg_attempts_total increments before compileCFG decides acceptance, and rejected attempts still fall through to trace compilation — so branching/skipping assertions on attempts_total > 0/== 0 alone can mask a real regression in the trace fallback path whenever CFG was attempted but rejected. The new "jits top-level typed-array loop as cfg" test (5448-5449) already uses the correct pattern — vm_jit_cfg_attempts_total > 0 && vm_jit_cfg_rejected_total == 0 — to prove genuine CFG success; the other five spots should use the same combined signal.
🎯 Proposed fix (applies analogously to all five locations)
- if id < 0 && i.samples.Value("vm_jit_cfg_attempts_total") > 0 {
+ if id < 0 && i.samples.Value("vm_jit_cfg_attempts_total") > 0 &&
+ i.samples.Value("vm_jit_cfg_rejected_total") == 0 {
return
}Also applies to: 3391-3393, 4098-4100, 5291-5293, 5397-5399
🤖 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 `@interp/interp_test.go` around lines 3229 - 3234, Update the five assertion
branches in interp_test.go around the relevant JIT emission checks to determine
CFG success using vm_jit_cfg_attempts_total > 0 together with
vm_jit_cfg_rejected_total == 0, matching the existing top-level typed-array loop
test. Replace attempts-only branching while preserving each location’s current
expected emission thresholds and fallback behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
docs/superpowers/specs/2026-07-12-unified-jit-region-design.md (1)
115-116: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMake the performance gate reproducible.
The 5% regression threshold needs a defined benchmark command, hardware/toolchain, warmup and repetition policy, workload sizes, and comparison artifact. Without those details, benchmark noise can produce inconsistent pass/fail decisions.
🤖 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 `@docs/superpowers/specs/2026-07-12-unified-jit-region-design.md` around lines 115 - 116, Expand the performance gate in the unified JIT region design specification to define the exact tl2g benchmark command, hardware and toolchain, warmup and repetition policy, single-row and 200-row workloads, and the recorded comparison artifact used for the 5% threshold. Keep the existing correctness and test requirements unchanged.interp/jit_structure_test.go (2)
15-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
requirefor assertions.Both new tests use raw
t.Fatalf/t.Fatal. As per coding guidelines, tests should "Userequire, notassert, in tests." Convert the conditionalt.Fatal*checks torequireassertions for consistency with the project's test conventions.🤖 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 `@interp/jit_structure_test.go` around lines 15 - 24, Update TestMergeCFGSlot to replace the conditional t.Fatalf and t.Fatal checks with require assertions, preserving the existing validation of changed, ok, dst.refKnown, and kind-mismatch behavior.Source: Coding guidelines
26-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest asserts private implementation shape, not behavior.
TestJITOpcodeDispatchIsSharedscans source text to countemitStepdefinitions and reject obsolete helper substrings. This couples the test to internal file layout/naming rather than observable dispatch behavior, so a benign rename or file split breaks it while a real dispatch regression that keeps the shape passes. Prefer a behavioral assertion (e.g., exercising representative opcodes through the single dispatch entrypoint).As per coding guidelines, tests must "assert behavior rather than private implementation shape."
🤖 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 `@interp/jit_structure_test.go` around lines 26 - 49, Replace the source-scanning logic in TestJITOpcodeDispatchIsShared with a behavioral test that invokes the shared emitStep dispatch entrypoint using representative opcodes and verifies their emitted results or effects. Remove assertions based on file names, function counts, and obsolete helper strings, while covering enough opcode categories to detect separate or incorrect dispatch implementations.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 `@interp/jit_arm64_step.go`:
- Around line 178-181: Update the reference validation in the CONST_GET handling
around boxed.Ref() to allow heap index 0, since it represents Null and must
reach l.constGet’s fallback handling. Reject only negative refs or refs beyond
ctx.heap, preserving the existing false return for invalid out-of-range
references.
- Around line 182-218: Update both constGetKnown and arrayGetKnown to handle
types.TypedArray[int64] consistently with cfgflow’s KindI64 inference. Add the
corresponding KindI64 handling and correct type-table metadata in each known
typed-array switch, or route int64 through the same exact-IP fallback used for
unsupported arrays.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-07-12-unified-jit-region-design.md`:
- Around line 115-116: Expand the performance gate in the unified JIT region
design specification to define the exact tl2g benchmark command, hardware and
toolchain, warmup and repetition policy, single-row and 200-row workloads, and
the recorded comparison artifact used for the 5% threshold. Keep the existing
correctness and test requirements unchanged.
In `@interp/jit_structure_test.go`:
- Around line 15-24: Update TestMergeCFGSlot to replace the conditional t.Fatalf
and t.Fatal checks with require assertions, preserving the existing validation
of changed, ok, dst.refKnown, and kind-mismatch behavior.
- Around line 26-49: Replace the source-scanning logic in
TestJITOpcodeDispatchIsShared with a behavioral test that invokes the shared
emitStep dispatch entrypoint using representative opcodes and verifies their
emitted results or effects. Remove assertions based on file names, function
counts, and obsolete helper strings, while covering enough opcode categories to
detect separate or incorrect dispatch implementations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65182a0f-3a7a-4eb7-9a50-292097eb2c67
📒 Files selected for processing (10)
docs/jit-internals.mddocs/superpowers/plans/2026-07-12-unified-jit-region.mddocs/superpowers/specs/2026-07-12-unified-jit-region-design.mdinterp/cfg.gointerp/cfgflow.gointerp/jit.gointerp/jit_arm64.gointerp/jit_arm64_cfg.gointerp/jit_arm64_step.gointerp/jit_structure_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/superpowers/plans/2026-07-12-unified-jit-region.md
🚧 Files skipped from review as they are similar to previous changes (2)
- interp/cfgflow.go
- docs/jit-internals.md
| ref := boxed.Ref() | ||
| if ref <= 0 || ref >= len(ctx.heap) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ref <= 0 hard-fails CONST_GET of a boxed-null constant.
Heap index 0 is permanently Null, and l.constGet handles boxed-null refs gracefully via the type-switch default. Here ref <= 0 rejects ref == 0 with return false before reaching that fallback; since emitStep reports this as (handled=true, ok=false), cfgBlock treats it as a hard failure and abandons whole-CFG compilation for any function containing a CONST_GET of a null-ref constant. Allow ref == 0 through so it delegates to l.constGet.
🐛 Proposed fix
ref := boxed.Ref()
- if ref <= 0 || ref >= len(ctx.heap) {
+ if ref < 0 || ref >= len(ctx.heap) {
return false
}As per coding guidelines, "Heap index 0 is permanently Null."
🤖 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 `@interp/jit_arm64_step.go` around lines 178 - 181, Update the reference
validation in the CONST_GET handling around boxed.Ref() to allow heap index 0,
since it represents Null and must reach l.constGet’s fallback handling. Reject
only negative refs or refs beyond ctx.heap, preserving the existing false return
for invalid out-of-range references.
Source: Coding guidelines
| switch ctx.heap[ref].(type) { | ||
| case types.TypedArray[bool], types.TypedArray[int8], types.TypedArray[int32], | ||
| types.TypedArray[float32], types.TypedArray[float64]: | ||
| ctx.push(value{kind: types.KindRef, raw: true, ref: ref}) | ||
| return true | ||
| default: | ||
| return l.constGet(ctx, op) | ||
| } | ||
| } | ||
|
|
||
| func (l arm64Lowerer) arrayGetKnown(ctx *lowering, op step) bool { | ||
| if ctx.count() < 2 || ctx.values[len(ctx.values)-1].kind != types.KindI32 { | ||
| return false | ||
| } | ||
| marker := ctx.values[len(ctx.values)-2] | ||
| constant := marker.ref | ||
| if !marker.raw || constant <= 0 || constant >= len(ctx.heap) { | ||
| return false | ||
| } | ||
|
|
||
| var kind types.Kind | ||
| var want uintptr | ||
| var scale uint8 | ||
| switch value := ctx.heap[constant].(type) { | ||
| case types.TypedArray[bool]: | ||
| kind, want = types.KindI1, itab(value) | ||
| case types.TypedArray[int8]: | ||
| kind, want = types.KindI8, itab(value) | ||
| case types.TypedArray[int32]: | ||
| kind, want, scale = types.KindI32, itab(value), 2 | ||
| case types.TypedArray[float32]: | ||
| kind, want, scale = types.KindF32, itab(value), 2 | ||
| case types.TypedArray[float64]: | ||
| kind, want, scale = types.KindF64, itab(value), 3 | ||
| default: | ||
| return false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing TypedArray[int64] case in the known typed-array switches.
Both constGetKnown (Lines 182-189) and arrayGetKnown (Lines 205-218) omit types.TypedArray[int64]. If the CFG dataflow in interp/cfgflow.go classifies int64 typed-array ARRAY_GET results as KindI64, then a valid int64 array read reaches the default: return false path here, which propagates as a hard failure and abandons CFG lowering for those functions. Add the int64/KindI64 case or route it through the exact-IP fallback consistently.
#!/bin/bash
# Confirm whether cfgflow infers KindI64 for int64 typed arrays (mismatch vs lowering).
rg -nP 'TypedArray\[int64\]|KindI64' interp/cfgflow.go interp/blockflow.go🤖 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 `@interp/jit_arm64_step.go` around lines 182 - 218, Update both constGetKnown
and arrayGetKnown to handle types.TypedArray[int64] consistently with cfgflow’s
KindI64 inference. Add the corresponding KindI64 handling and correct type-table
metadata in each known typed-array switch, or route int64 through the same
exact-IP fallback used for unsupported arrays.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
interp/jit_plan_test.go (1)
71-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
mergeSlotandplanSpillinto their own top-level tests.These subtests exercise separate functions unrelated to
plan.valid(), so they belong in dedicatedTestMergeSlotandTestPlanSpilltop-level tests rather than nested underTestPlan.As per coding guidelines: "Use one top-level test per public symbol, named
Test<Func>orTest<Type>_<Method>."🤖 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 `@interp/jit_plan_test.go` around lines 71 - 89, Move the “slot merge” subtest into a dedicated top-level TestMergeSlot test and move the “spill policy” subtest into a dedicated top-level TestPlanSpill test. Keep each test’s existing assertions and fixtures unchanged, and remove these unrelated subtests from TestPlan.Source: Coding guidelines
interp/jit.go (1)
264-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
okhandling flagged by staticcheck (SA4006).The value returned into
okis only consumed byif !ok { continue }, which sits at the end of the loop body and is therefore a no-op — every plan is tried regardless, and the real gate ismod.emits > 0after the loop. This is functionally harmless but the[error]-level lint will fail CI. Simplify by discarding the boolean.♻️ Proposed simplification
- ok, err := c.compile(input, plan, mod) - if err != nil { - return nil, err - } - if !ok { - continue - } + if _, err := c.compile(input, plan, mod); err != nil { + return nil, err + }🤖 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 `@interp/jit.go` around lines 264 - 275, In the plan iteration around c.compile, discard the unused boolean result and retain only the error handling; remove the trailing if !ok continue because it cannot affect loop behavior. Preserve the existing plan validity checks, compile error propagation, and mod.emits gate.Source: Linters/SAST tools
interp/interp.go (1)
858-864: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold the entry-kind dispatch into a tagged switch (QF1003).
entryFunctionpublishes the native slot above and then still falls through this if/else chain intoi.call, which is intended. Aswitch entry.entryreads cleaner and clears the lint hint; behavior is unchanged.🤖 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 `@interp/interp.go` around lines 858 - 864, Replace the entry-kind if/else chain in the dispatch block with a tagged switch on entry.entry, preserving the existing i.loop handling for entryLoop, i.start handling for entryModule, and default i.call handling for entryFunction and other kinds.Source: Linters/SAST tools
🤖 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 `@interp/interp.go`:
- Around line 858-864: Replace the entry-kind if/else chain in the dispatch
block with a tagged switch on entry.entry, preserving the existing i.loop
handling for entryLoop, i.start handling for entryModule, and default i.call
handling for entryFunction and other kinds.
In `@interp/jit_plan_test.go`:
- Around line 71-89: Move the “slot merge” subtest into a dedicated top-level
TestMergeSlot test and move the “spill policy” subtest into a dedicated
top-level TestPlanSpill test. Keep each test’s existing assertions and fixtures
unchanged, and remove these unrelated subtests from TestPlan.
In `@interp/jit.go`:
- Around line 264-275: In the plan iteration around c.compile, discard the
unused boolean result and retain only the error handling; remove the trailing if
!ok continue because it cannot affect loop behavior. Preserve the existing plan
validity checks, compile error propagation, and mod.emits gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a4c4251-b9d4-4ec6-9f3b-c746bde1180c
📒 Files selected for processing (14)
docs/architecture.mddocs/jit-internals.mddocs/superpowers/plans/2026-07-12-plan-compiler.mddocs/superpowers/specs/2026-07-12-plan-compiler-design.mdinterp/interp.gointerp/interp_test.gointerp/jit.gointerp/jit_arm64.gointerp/jit_arm64_test.gointerp/jit_plan.gointerp/jit_plan_test.gointerp/jit_test.gointerp/pool_test.gointerp/trace.go
💤 Files with no reviewable changes (1)
- interp/jit_test.go
✅ Files skipped from review due to trivial changes (4)
- interp/pool_test.go
- docs/superpowers/plans/2026-07-12-plan-compiler.md
- docs/architecture.md
- docs/superpowers/specs/2026-07-12-plan-compiler-design.md
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
===========================================
- Coverage 59.39% 27.79% -31.60%
===========================================
Files 82 83 +1
Lines 54215 60471 +6256
===========================================
- Hits 32200 16807 -15393
- Misses 13029 42469 +29440
+ Partials 8986 1195 -7791 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
blockHeights computes each basic block entry operand-stack height via a forward fixpoint, resolving CALL/RETURN_CALL arity from statically known function signatures (locals, upvals, function constants). Indeterminate shapes (handlers, MAP_NEW, CLOSURE_NEW, STRUCT_NEW, dynamic callees, height merge disagreement, unreachable blocks) reject so callers can fall back to the trace JIT.
Summary
(function, IP)block -> step -> term -> edgepipelineinterp/jit_arm64.go; numeric, call/frame, heap, and control-flow helpers are not split across filesTracerto oneprogram.Programso exact code, trees, loops, and hit state cannot cross program boundariesthreaded.goimport grouping deterministic andgoimports-clean without an external generator dependencyArchitecture
Every internal edge carries a block ID. An unresolved edge carries only its threaded fallback anchor. Caller continuations are ordinary blocks in the same flat pool and are referenced by block ID. The ARM64 backend does not inspect bytecode, trace trees, CFG nodes, or frontend identity.
Correctness fixes
WithTracerreuse from sharing cached code and traces across different programsVerification
make checkgit diff --checkgo test -race -count=1 ./...GOARCH=amd64 go test -count=1 ./...go run ./internal/cmd/geninterp -checkgo test -race -count=1 ./...tl2g benchmark
Apple M4 Pro, darwin/arm64, median of three runs:
Compared with the earlier PR result (2,891 / 137,494 ns/op), single-row performance is effectively unchanged while batch/200 improves by about 3.0%.
Structure
Production JIT/tracer code is 5,773 lines. ARM64 lowering remains in the single
interp/jit_arm64.gofile, while architecture-neutral planning stays ininterp/jit.goandinterp/jit_plan.go.