Repository navigation
vm: AGET/ASET opcodes, and fast opcodes for heads that resolve to the core var - #1058
Conversation
8c039c2 to
7104cc9
Compare
nooga
left a comment
There was a problem hiding this comment.
Reviewed the current head against #1057. The fast-opcode gate checks core Var identity, retaining local and namespace shadowing while accepting qualified/aliased core heads. AGET/ASET stack effects, fixed arities, caught errors, element coercion and aset's original-value result agree with the runtime functions. The appended opcode-set signature and regenerated bundle preserve the existing incompatible-opcode rejection policy.
Local verification at the #1060 stack tip: full VM, compiler, bytecode and IR package suites pass, including fast-opcode resolution and opcode-versus-function result/error tests. The stated separation of direct compiler opcodes, IR bytecode and native array accessors is clear. No actionable finding; approve after #1057.
7104cc9 to
0e1901f
Compare
0e1901f to
5cb373a
Compare
… core var The fast-opcode gate accepted any resolvable unqualified head and refused every qualified one, so a syntax-quoted clojure.core/+ went through the var as a call (#1045) while a same-named var defined in the current ns still got the core opcode. The gate now requires the head to resolve to the clojure.core var itself, qualified or not, aliases included. aget with two args and aset with three compile to OP_AGET and OP_ASET, the same shape as OP_ADD: the typed-array case runs inline without LOAD_VAR, INVOKE, or the variadic argument slice, and every other input reports the error the core fn would, so bytecode and the var agree on every input. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5cb373a to
d467e98
Compare
|
@nooga Thanks for the review! Merging. |
Fixes the lowered-Go half of #1045. Stacked on #1058 (base `wt/aget-opcode`), which fixed the bytecode half. It does not depend on #1058's code; it is stacked so the two halves of the issue review together. `intrinsic-builtin-op` in `build.lg` required `(nil? (namespace head))`, so the `core/<` and `core/inc` that syntax-quote emits built as ordinary var calls, as did any explicit `clojure.core/<`. Under `lg compile` that turned the issue's `qual` loop and every `dotimes` counter into `vm.Value` locals with a cached-var trampoline per op. The gate now accepts a qualified head on the same terms #1058 gave the bytecode gate. A qualified symbol cannot be shadowed by a local, so it skips that check and keeps the identity check: `resolve` of the head must be the `clojure.core` var of that name. That admits aliases and refuses a qualified head that names another namespace's `inc`, including through an alias named `core`. A `core/` head that does not resolve falls back to the core var, the same fallback `fastOpcodeCoreName` has, so the two engines agree. The unqualified path is unchanged; it already did the identity check. `*ir-compile*` builds bytecode from the same IR, so on that path qualified heads now get `OP_ADD` and the other fast opcodes too, as the plain compiler does after #1058. ## Verified `qual` now lowers like `plain`: `j int64`, `acc int64`, `j < arg0`, no `CachedVarFn`. New tests in `lisp_lower_go_qualified_gate_test.go` cover the repro spelled `core/` and `clojure.core/`, an aliased head, a `dotimes` counter lowering to `int64` through `lower-ns-to-go` (which macroexpands, as `lg compile` does), and a qualified non-core `inc` staying a call, directly and through an alias named `core`. The four positive tests fail with the old gate restored. `pkg/vm`, `pkg/ir`, `pkg/compiler`, `pkg/genmanifest` and `pkg/rt/...` pass; `make generate` and `check-generated` are clean; linux, js/wasm, plan9 and wasip1 build. Not timed: the lowered loop has the same native locals and operators as `plain`, so the issue's `plain` numbers are the expectation. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fixes #1045. Stacked on #1057 (the base branch is
wt/array-lowering); it only needs #1057 for the typed-slice fields onvm.TypedArray, so it can be retargeted tomainif that one stalls.Two changes to the bytecode compiler's fast-opcode gate and the VM, both in the shape
OP_ADDalready has.The gate
tryFastOpcodeaccepted 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+,<andinc(dotimesincluded), went through the var as a call. That is Namespace-qualified core ops (clojure.core/+from syntax-quote) miss the fast arithmetic path, sodotimesruns ~3× slower #1045.(defn + ...)in the current ns still compiled(+ a b)toOP_ADD, because the gate only checked that+resolved, not to what.The gate now requires the head to resolve to the
clojure.corevar itself: unqualified and not shadowed by a local, qualified to the core ns (same fallbackcompileFormuses before refers exist), or through an alias. The IR side (intrinsic-builtin-opinbuild.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 toOP_AGETand(aset a i v)toOP_ASET. The handlers assert*vm.TypedArrayandInt, bounds-check, and index the typed slice. Every other input produces the errorclojure.core/agetorasetwould, with the same text, so a program cannot tell which path ran (the helper lives inpkg/vmbecause the VM cannot importpkg/rt; the three error constructors are out of line so the hot path inlines). The compiler emits them only for those arities, so nestedagetstays a call.asetreturns the value as given, as the core fn does.Binding happens at compile time, as with every fast opcode: redefining
agetafter 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.lgbis regenerated. A bundle from an olderlgis 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(1Mdouble-array, 50 reps, VM):The #1045 repro (10M iterations, two rounds each):
plainqualvia-dotimesqualnow runs atplainspeed.dotimeskeeps thevolatile!cost the repro adds.Verified
pkg/vm,pkg/compiler,pkg/bytecode,pkg/ir,pkg/genmanifestandpkg/rt/...suites;make generateclean andcheck-generatedgreen; 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-argagetand 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
clojure.core/+underlg compileor*ir-compile*still lowers as a call. Same five lines inintrinsic-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 emitOP_AGET; that path builds from IR ops, andagethas no IR op. The AOT path has its own native accessors (AOT lowering: lower aget/aset on typed arrays to native accessors #1057).ADD_F/GTE_Ior unboxed slots. What is left in thesum-arrprofile is operand shuffling andFloatboxing, which needs a frame-level change rather than another opcode.🤖 Generated with Claude Code