fix: sum and avg now work on VM and Cranelift, not just tree#295
Merged
Conversation
sum and avg fell through to the named-function lookup on the VM and errored as 'undefined function', though both were implemented in the tree-walking interpreter. Pattern-match the median wiring: new OP_SUM (165) and OP_AVG (166), compile arms in build_call, dispatch handlers, vm_sum/vm_avg helpers, and jit_sum/jit_avg for the Cranelift path. Semantics match the tree-walker: sum [] = 0, avg [] errors, non-list input errors, non-number elements error. Un-ignore vm_sum_basic, vm_sum_empty, and vm_avg_basic now that the VM backs them.
Two changes, same hazard: 1. Add OP_SUM/OP_AVG codegen to both the in-process JIT (jit_cranelift) and the AOT compiler (compile_cranelift). New helper FuncIds, helper declarations, symbol registrations, and matching codegen blocks that mirror OP_MEDIAN. 2. Refresh the per-register F64 shadow after every stats helper call (median, quantile, stdev, variance, sum, avg). The JIT keeps an F64 shadow alongside each I64 NanVal so OP_*_NN fast paths can skip the bitcast; helper-call opcodes were only writing the I64 slot, leaving the shadow stale at zero. That made expressions like '-(avg xs) (sum ys)' silently produce 0 instead of the real result. Pre-existing bug for the median family; surfaced by sum/avg landing in the same hot path. Both classification passes (jit_cranelift and compile_cranelift) now list OP_SUM/OP_AVG in the numeric-output set so reg_always_num flows through correctly.
Sixteen regression tests pinning sum and avg behaviour across --run-tree, --run-vm, and (when enabled) --run-cranelift: happy paths, empty-list edge cases (sum returns 0, avg errors), wrong-type and non-numeric-element errors, and the F64-shadow hazard that breaks arithmetic on stats helper results. examples/sum-avg.ilo demonstrates the four common shapes and is picked up by examples_engines so every engine has to round-trip the file.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Three codegen tests using compile_to_object_bytes (no libilo.a needed) so the AOT branches for sum and avg, plus the F64-shadow refresh under a subsequent OP_SUB_NN, are exercised by cargo llvm-cov.
7 tasks
danieljohnmorris
added a commit
that referenced
this pull request
May 16, 2026
Helpers struct gains min_lst / max_lst FuncIds in both jit_cranelift and compile_cranelift, declared as 2-param/1-return imports resolved from jit_min_lst / jit_max_lst at link time. Both opcodes register in the guaranteed-numeric-output allowlist for reg_always_num analysis, and the JIT site refreshes the F64 shadow on the result so subsequent arithmetic (`max xs - min xs`) reads the fresh value rather than a stale shadow. Same pattern PR #295 used for OP_SUM / OP_AVG / OP_MEDIAN and the rest of the stats helpers.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ilo file.ilo --run-vmand--run-craneliftreportedCompile error: undefined function: sum/avg. The tree-walker had both builtins wired up directly; the VM compiler had dedicated arms formedian,stdev,variance,quantile, andcumsumbut none forsumoravg, so they fell through to the named-function lookup and errored.Manifesto framing: every agent reaching for
sum xson the VM hits a confusing "undefined function" diagnostic that names a real builtin, then has to rewrite the call asfld + xs 0. Tokens wasted and trust burned. The fix restores symmetry with the rest of the stats family across all three engines.While fixing this I noticed the existing stats family (
median,stdev,variance,quantile) had a Cranelift correctness bug: the per-register F64 shadow used by the OP_*_NN fast paths was never refreshed after a helper call, so any arithmetic over a stats result silently produced0on--run-cranelift. Fixed in the same PR with cross-cutting coverage.Repro
Before:
After:
2.5on every engine. Empty list:sum []returns0,avg []errors withavg: cannot average an empty list, matching tree-walker semantics on every backend.What's in the diff
ae3768cvm: newOP_SUM(165) andOP_AVG(166) opcodes with compile arms inbuild_call, dispatch handlers,vm_sum/vm_avghelpers, andjit_sum/jit_avgfor the Cranelift handoff. Un-ignoresvm_sum_basic,vm_sum_empty,vm_avg_basicnow that the VM backs them.393d7a8cranelift: wire the new opcodes through the JIT and AOT compilers (helper FuncIds, declarations, symbol registrations, codegen blocks). In the same commit, refresh the F64 shadow after every stats helper call (median, quantile, stdev, variance, sum, avg) so subsequent OP_*_NN fast paths read the real numeric result instead of a stale zero. Both numeric-output classification passes (jit + AOT) include the new ops soreg_always_numflows through.4f191c0test: 16-test cross-engine regression file pinning the happy paths, the empty-list contracts (sum=0, avg=error), the wrong-type / non-numeric-element errors, and the F64-shadow hazard (including the pre-existing median variant). Newexamples/sum-avg.ilopicked up byexamples_enginesso every backend round-trips it.Test plan
cargo test --release --features cranelift --test regression_sum_avg_cross_engine— 16/16 passcargo test --release --features cranelift --test examples_engines— passcargo test --release --features cranelift --lib— pass (the only failures withCARGO_TARGET_DIRset are pre-existing AOT tests that look forlibilo.ain the default target dir; clean target passes 7/7)cargo fmt --checkcleanFollow-ups
None for this fix. The OP_SUM/OP_AVG dispatch in the VM has the same inline shape as OP_MEDIAN; a future refactor could collapse the six stats codegen blocks behind a small helper, but that's pure cleanup with no behavioural change.