Repository navigation
AOT lowering: lower aget/aset on typed arrays to native accessors - #1057
Conversation
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
mparrett
left a comment
There was a problem hiding this comment.
self-review
Reviewed ed17f627e79d9bf9b4ff65b8168529c7d0962d54 against e9789b7d534cce6a570fc0781f220f07c30be17e.
[P2] Preserve array kind safety across function-level recur
For (defn f [^doubles a n] (if (< n 1) (aget a 0) (recur (long-array 3) (dec n)))), calling f with a double-array and n = 1 returns 0 in bytecode and in the base revision's lowered code. This PR's lowered code instead returns double-array expected, got int-array: recur-fn-arg-expr sees the same Go type (*vm.TypedArray) and assigns the long-array into a0, while the hinted array kind remains :float and the next iteration calls AtFloat64. The new kind guard in coerce-arg-to-type only covers ordinary calls, so it never runs on this back-edge. A non-array recur operand can also produce uncompilable Go (a0 = vm.String("x")). Validate recur operands against the parameter's lattice kind and keep the function boxed or fall back to bytecode when that kind cannot hold across iterations. Cover this with a generated-Go execution test.
Validation: the new array lowering/accessor tests and the full pkg/ir, pkg/vm, pkg/rt/..., and pkg/compiler suites passed. Generated-Go probes reproduced both the kind error and the non-array assignment compile failure. The pkg/rt run passed after allowing its local HTTP test listener outside the sandbox.
Overall correctness: incorrect due to the reproduced recur regression.
|
Self-review finding addressed in 669ece0 |
669ece0 to
e8abf8d
Compare
nooga
left a comment
There was a problem hiding this comment.
Re-reviewed the current head against main. The prior function-level recur finding is resolved: a changed or unproven array kind rejects native lowering, and the rejected sibling is removed from the direct-call registry. Stable kinds remain native. Array arguments carry lattice kinds alongside their common Go pointer type, and call-site guards preserve the boxed fallback.
Local verification at the #1060 stack tip: VM, compiler, bytecode and IR suites pass, including the array accessors, call guards and recur regressions. An additional generated-Go execution probe produces [0 0.0] under both native and bytecode for changing/stable recur kinds, with confirmed native dispatch of its run entry. No actionable finding in this delta. Performance claims were not remeasured.
e8abf8d to
36b469d
Compare
|
What follows is Claude's, posted under my account. @nooga You approved the pre-rebase head, so one change since then: the rebase onto main picked up #1017, which changed how lowered code calls a sibling it doesn't link directly. It now emits |
A kernel written with double-array, ^doubles and aget/aset lowered to Go that boxed the index per access and the element on the way back, so the arithmetic around it ran on rt.AddValue. The runtime already knew the element type (vm.TypedArray is a kind plus a native slice); the lattice and the lowerer could not see it. - lattice: [:array K] for K in :byte :int :float :object, declared by the array constructors (typeinfer) or a ^doubles/^longs/^ints/^bytes/^objects param hint (build), never inferred from elements. Non-maskable, joins like :dtype. aget types as the element, alength as :int, aset as its value, aclone as its argument. - lower-go: every array kind is *vm.TypedArray in Go. aget/aset/alength on a proven kind with a native int index emit the typed accessors under the rt.NativePrimsIntact root guard, trampoline in the else branch. Registry entries carry :param-types so a call into a *vm.TypedArray param compares element kinds: same kind passes, another kind or a non-array bails to the trampoline (bytecode semantics), an unproven vm.Value is guarded with a Kind() check. - vm.TypedArray: one typed slice field per kind instead of data any, and AtFloat64/SetFloat64 (and the int64, byte, Value pairs), each inlinable: one bounds compare, the access, errors on a cold path. A wrong kind is an error, never a panic. - rt.NativePrimsIntact inlines; its LG_GUARD_DEBUG path moved to a cold fn. sum-arr over a 1M double-array: VM 232-263 ms, AOT before 27-31 ms, AOT after 0.95-1.03 ms, hand-written Go 0.93-0.99 ms (Apple M2, Go 1.27.1). Closes #1055. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…bling Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#1039 skips the trailing typeinfra epoch on a function no middle pass edited, and that epoch was what typed an aget visited before the array constructor it reads: the call joined to :unknown on the first visit. The transient ops got the same :bottom rule in #1038; aget and aclone now follow it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
nnunley
left a comment
There was a problem hiding this comment.
Correctness reads right to me: the recur back-edge check, the kind guard on direct calls, and the join rule (different array kinds join to :any) keep a wrong kind from reaching the accessor. One thing before merge: the timings in the description are three same-machine runs, and AGENTS.md says perf claims go through docs/perf/ratchet.md, so either route them there or label them as indicative. I left a note on the struct layout inline; it isn't blocking.
| // the Go inliner's budget to be worth emitting, and an interface assertion | ||
| // per access put them over it (cost 88 against a budget of 80). Exactly one | ||
| // field is live, the one `kind` names; the others stay nil. | ||
| type TypedArray struct { |
There was a problem hiding this comment.
Not blocking. I measured the layouts in a standalone copy on go1.27.1: a one-element double-array goes from 56 B in 3 allocs (the slice header in data any was its own allocation) to 120 B in 2, so 64 more bytes per array but one fewer alloc. If the size matters, one layout keeps both: kind, ptr unsafe.Pointer, and one length field per kind with only the live kind's set, so a wrong kind sees length 0 as the nil slices do now. That is a 48-byte struct, 56 B in 2 allocs, and AtFloat64/SetFloat64 still inline (cost 78 and 79), though that is close to the budget of 80 and it brings unsafe into TypedArray. Your call.
There was a problem hiding this comment.
What follows is Claude's, posted under my account.
Keeping this layout. Your measurements give the trade: 64 more bytes per array and one fewer allocation, against unsafe in TypedArray and AtFloat64 at 78 of the 80 inline budget. Staying under that budget is why the struct has a field per kind, so I'd rather keep the headroom. The kernels this targets use a few large arrays, where 64 bytes each doesn't register. If array count shows up in a profile, your compact layout is the fix.
Separately, per your review, the description's timings are now labeled as indicative same-machine runs.
36b469d to
6b206e2
Compare
|
What follows is Claude's, posted under my account. @nooga @nnunley One commit since your approvals changes typeinfer code, so I'm holding the merge until one of you has looked at it. Rebasing onto main after #1038 and #1039 merged broke two tests here: The cause is the ordering problem #1038 fixed for the transient ops. This PR's
If the change looks right, please re-approve and I'll merge this, then #1058 and #1060. |
|
Since this PR opens up native numeric kernels, here is a numeric simplification design I sketched earlier and never shipped, in case it's useful next to this work. Refs #268. The idea is one dedicated numeric pass in the IR pipeline, not rules spread across constfold and typeinfer:
The constraints: a fold refuses on overflow and on divide by zero, as the VM does; mixed Int/Float goes through One thing this turned up: Phase A's |
|
@nooga Thanks for the review! Merging. |
… core var (#1058) Fixes #1045. Stacked on #1057 (the base branch is `wt/array-lowering`); it only needs #1057 for the typed-slice fields on `vm.TypedArray`, so it can be retargeted to `main` if that one stalls. Two changes to the bytecode compiler's fast-opcode gate and the VM, both in the shape `OP_ADD` already has. ## The gate `tryFastOpcode` accepted any unqualified head that resolved to something and refused every qualified head. Both halves were wrong: - `(clojure.core/+ a b)`, which is what syntax-quote emits, so every macro-written `+`, `<` and `inc` (`dotimes` included), went through the var as a call. That is #1045. - `(defn + ...)` in the current ns still compiled `(+ a b)` to `OP_ADD`, because the gate only checked that `+` resolved, not to what. The gate now requires the head to resolve to the `clojure.core` var itself: unqualified and not shadowed by a local, qualified to the core ns (same fallback `compileForm` uses before refers exist), or through an alias. The IR side (`intrinsic-builtin-op` in `build.lg`) already does the identity check; it still refuses qualified heads, so the lowered-Go half of #1045 stays open and is noted below. ## The opcodes `(aget a i)` compiles to `OP_AGET` and `(aset a i v)` to `OP_ASET`. The handlers assert `*vm.TypedArray` and `Int`, bounds-check, and index the typed slice. Every other input produces the error `clojure.core/aget` or `aset` would, with the same text, so a program cannot tell which path ran (the helper lives in `pkg/vm` because the VM cannot import `pkg/rt`; the three error constructors are out of line so the hot path inlines). The compiler emits them only for those arities, so nested `aget` stays a call. `aset` returns the value as given, as the core fn does. Binding happens at compile time, as with every fast opcode: redefining `aget` after a caller compiled does not reach the opcode. That is the existing policy for `+`, not a new one, and it is the reason the gate above had to get stricter rather than looser. The two opcodes append to the enum, so the opcode-set signature changes and `core_compiled.lgb` is regenerated. A bundle from an older `lg` is rejected by the decoder, as for any opcode addition. ## Measured Same host, interleaved runs, load average 15 to 21 at the time, so the ratios are approximate. `sum-arr` (1M `double-array`, 50 reps, VM): | | base | this PR | |---|---:|---:| | ms/rep, three rounds | 335 / 302 / 273 | 197 / 202 / 195 | The #1045 repro (10M iterations, two rounds each): | | base | this PR | |---|---:|---:| | `plain` | 1647 / 2442 | 1495 / 1354 | | `qual` | 5260 / 5073 | 1660 / 1268 | | `via-dotimes` | 6186 / 5290 | 3721 / 2753 | `qual` now runs at `plain` speed. `dotimes` keeps the `volatile!` cost the repro adds. ## Verified `pkg/vm`, `pkg/compiler`, `pkg/bytecode`, `pkg/ir`, `pkg/genmanifest` and `pkg/rt/...` suites; `make generate` clean and `check-generated` green; builds for linux, js/wasm, plan9 and wasip1. New tests: the VM handlers on every array kind and every error path with the core fn's text (`typed_array_opcode_test.go`); the gate accepting unqualified, qualified and aliased heads and refusing a shadowing local, a 3-arg `aget` and an ns-level redefinition, with the program seeing the redefinition's result; and opcode-vs-core-fn agreement on results and caught error messages. ## Not in this PR - The IR gate still refuses qualified heads, so `clojure.core/+` under `lg compile` or `*ir-compile*` still lowers as a call. Same five lines in `intrinsic-builtin-op`. I kept this PR to the bytecode side; I can fold the IR gate in here if you prefer one PR. - `*ir-compile*` bytecode does not emit `OP_AGET`; that path builds from IR ops, and `aget` has no IR op. The AOT path has its own native accessors (#1057). - No typed `ADD_F`/`GTE_I` or unboxed slots. What is left in the `sum-arr` profile is operand shuffling and `Float` boxing, which needs a frame-level change rather than another opcode. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
The README described the lowering as a proposal and timed the hinted row on the PR branch. Re-timed on main at 9981aa04 with both kernels: the hinted one is now a checked-in file, sum_arr_doubles.lg, and the lowered-Go section shows both outputs. The VM row picks up OP_AGET from #1058. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
) * experiments/aot/arrays: the sum-arr ladder for typed-array lowering Measures what aget/aset over a double-array costs when lowered to Go today (rt.CoreAgetf per element, boxed index and result) against the three Go shapes a kind-aware lowering brackets: TypedArray.Get direct, the slice hoisted once, and plain []float64. Adds the module to the go-modules CI matrix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * experiments/aot/arrays: the after row, from nooga/let-go#1057 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * experiments/aot/arrays: re-measure after nooga/let-go#1057 merged The README described the lowering as a proposal and timed the hinted row on the PR branch. Re-timed on main at 9981aa04 with both kernels: the hinted one is now a checked-in file, sum_arr_doubles.lg, and the lowered-Go section shows both outputs. The VM row picks up OP_AGET from #1058. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * experiments/aot/arrays: retime with no contending jobs The f749233 rows were taken while another session's jobs held the CPU and read about 2x slow. Retimed on the same lg at load 5-6 with nothing else timing; ratios unchanged, absolutes back in line with the 2026-10-10 run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…slot #1057 gave aget, alength and byte-array guarded direct calls to their Go entries (rt.CoreAgetf and friends), taken while rt.NativePrimsIntact holds. Those entries return (vm.Value, error), but typeinfer can give the call's node a native slot: float64 for aget on a ^doubles array, int64 for alength, *vm.TypedArray for byte-array. typed-call-result-stmts assigned the vm.Value straight into that slot, so the generated Go did not build. The trampoline branch of the same if already unboxed, through boxed-call-stmts (#1044). The (vm.Value, error) case of typed-call-result-stmts now goes through boxed-call-stmts too, so both branches take the result into tb<nid>, check the error, and comma-ok assert it to the slot's type. A vm.Value slot, or a dead result, emits what it did before. test/gogen/native_prim_typed_slot.lg covers aget with a boxed index, alength of an unhinted array, and a byte-array local. On main all three sites fail to build. Fixes #1081 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…slot (#1084) * fix(lower-go): unbox a native prim's direct-call result into a typed slot #1057 gave aget, alength and byte-array guarded direct calls to their Go entries (rt.CoreAgetf and friends), taken while rt.NativePrimsIntact holds. Those entries return (vm.Value, error), but typeinfer can give the call's node a native slot: float64 for aget on a ^doubles array, int64 for alength, *vm.TypedArray for byte-array. typed-call-result-stmts assigned the vm.Value straight into that slot, so the generated Go did not build. The trampoline branch of the same if already unboxed, through boxed-call-stmts (#1044). The (vm.Value, error) case of typed-call-result-stmts now goes through boxed-call-stmts too, so both branches take the result into tb<nid>, check the error, and comma-ok assert it to the slot's type. A vm.Value slot, or a dead result, emits what it did before. test/gogen/native_prim_typed_slot.lg covers aget with a boxed index, alength of an unhinted array, and a byte-array local. On main all three sites fail to build. Fixes #1081 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: regenerate generated.sums after rebasing onto main Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Closes #1055, the array half of #358.
A numeric kernel written the Clojure way,
double-arrayplus^doublesplusaget/aset, lowered to Go that boxed the index on every access, got the element back boxed, and so ran its whole accumulator chain onrt.AddValue. The runtime already held the element type (vm.TypedArrayis a kind plus a native slice); nothing in the lattice or the lowerer could see it. This gives the lattice an array kind and the lowerer a use for it.What changes
[:array K]for K in:byte :int :float :object, mirroringvm.ArrayKind. Non-maskable, joins like:dtype. The kind is declared, never inferred from elements:typeinfertypes the six array constructors (long-arrayandfloat-arrayalias the 64-bit kinds they construct), andagetas the element type,alengthas:int,asetas its value,acloneas its argument.^doubles,^floats,^longs,^ints,^bytes,^objectson params, next to^double/^longinbuild.lg. The "unsupported hint" warning names them.*vm.TypedArray. It implementsvm.Value, so a local boxes by interface conversion and the same backing store is shared with any un-lowered code that holds the array.vm.TypedArray:AtFloat64/SetFloat64,AtInt64/SetInt64,AtByte/SetByte,AtValue/SetValue, each one unsigned bounds compare and the access, with the kind check folded in (a wrong kind finds a nil field) and all error construction in a cold path, so each inlines (cost under 80). To get there the struct keeps one typed slice per kind instead ofdata any;Unbox()still returns the active slice.aget/aset/alengthon a proven array kind with a native int index emit the accessor,if rt.NativePrimsIntact() { typed } else { trampoline }like the other native primitives. An int value into a double-array widens, asTypedArray.Setcoerces. Anything that does not line up (unknown kind, boxed index, nestedaget) falls through to today's path.*vm.TypedArrayparam: the Go spec does not carry the kind, so direct-call registry entries now carry:param-types(lattice) beside:param-specs. A proven array of the same kind passes through; a different kind, or a proven non-array, bails the call to the trampoline, which is what bytecode does (the hint is ignored there andagetworks on any array); an unprovenvm.Valuegets the scalar guard's shape with&& tg.Kind() == vm.ArrayFloatin the condition.rt.NativePrimsIntactnow inlines (itsLG_GUARD_DEBUGpath moved to a cold function). It runs once per guarded site, which in these loops is once per element, so every guarded native-primitive call now pays one atomic load instead of a call.Measured
Indicative only: three runs each on one machine, not ratchet results.
sum-arrover a 1Mdouble-array, 50 reps, Apple M2, Go 1.27.1, load under 7:rt.CoreAgetf,rt.AddValue)[]float64The lowered loop is
arg__1, callErr = arg0.AtFloat64(i),s = s + arg__1, onarg0 *vm.TypedArrayands float64. The kernel, the three Go shapes it brackets and the generated Go before/after are in mparrett/let-go-lab#52.Verified
pkg/ir/lisp_lower_go_array_test.go(hintedagetlowers toAtFloat64with noAddValue;asetwidens an int value; the constructor types a local;^longsuses the int64 accessors; avm.Valuearg into an array param is guarded with the kind check; a long-array or a string into a^doublessibling bails to the trampoline; an unhinted param keeps today's path) andpkg/vm/typed_array_lowered_test.go(accessors share the backing store withGet, bounds and kind errors are errors, not panics).tryprints the same lines compiled as interpreted.make generate,make check-generated,go testoverpkg/vm,pkg/ir,pkg/rt/...,pkg/genmanifest,pkg/compiler/...; gofmt; linux, js/wasm, plan9 and wasip1 builds; the scoped comment lint over the touched files reports only pre-existing lines.Wiki
concepts/typed-arrays.mddescribes the pre-PR state (thedata anystruct, "what the lowering does with an array: nothing", "why it stops there") andconcepts/go-backend.mdlists the proven types as scalars only. Both become stale on merge; I will send the wiki update once this lands.Not in this PR
vm.Valueindex. The intrinsic needs a native int; a boxed index keeps the boxed path.loopwhose body is(when test (side-effecting-call) (recur …))fails in legalize ("expected int64, got Keyword") onmaintoo; theif/dospelling lowers. Filed separately.🤖 Generated with Claude Code