Skip to content

feat(ir,vm): unchecked-add/subtract/multiply as first-class ops - #811

Open
mparrett wants to merge 2 commits into
mainfrom
wt/ir-unchecked-lowering
Open

feat(ir,vm): unchecked-add/subtract/multiply as first-class ops#811
mparrett wants to merge 2 commits into
mainfrom
wt/ir-unchecked-lowering

Conversation

@mparrett

@mparrett mparrett commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Why

unchecked-add, unchecked-subtract and unchecked-multiply reached the IR only as generic calls to the core var. The Go lowerer had two options for such a call, a CachedVarFn trampoline or the NativePrimsIntact prim call, and typeinfer learned nothing about the result either way. So any kernel written on wrapping arithmetic (a hash, a PRNG, a checksum) lost the typed native path at the operations that make up its inner loop.

This surfaced while checking whether a pure let-go xxh3 could be AOT-compiled (nnunley's suggestion on #799). All 52 of its defns lowered, but every u64+ / u64* helper came out as a boxed prim call with vm.Value in and out, because the arithmetic inside had no IR op to type.

What

The three ops become first-class on both backends, shaped like the bitwise ops that already are.

  • VM: OP_UNCHECKED_ADD / SUB / MUL, appended after OP_DIV. Each handler has the same shape as OP_ADD: the Int/Int path inline, and everything else falls back to the generic implementation. That implementation is vm.NumUncheckedAdd / Subtract / Multiply, new in pkg/vm/numbers.go, and it is the only one: the core fns in pkg/rt/lang.go and the AOT helpers in pkg/rt/golower_runtime.go call it too, so the opcode, the var and the lowered code cannot disagree about what an operand may be. Operands coerce through vm.ToInt (Float, Boolean, Ratio, an int64-range BigInt), which is the contract feat(core): unchecked-add/subtract/multiply/negate/inc/dec/divide-int — Clojure parity #64 set for the family; only what ToInt rejects is a type error. The bytecode compiler emits the opcodes for binary unqualified calls through the existing tryFastOpcode gate, which already handles shadowing.
  • IR: catalog rows in pkg/ir/ir_ops.lg, builtin-ops entries, an UncheckedOp type that types like BitwiseOp (only int/int is proven :int; any other operand stays unknown and takes the coercing runtime helper, so no numeric contagion), bytecode and Go lowering, and membership in the purity, constfold, and commutative sets.
  • Go lowering: int/int lowers to native + - *. Go integer arithmetic already wraps, which is the unchecked contract. Any other operand mix routes to rt.UncheckedAddValue / SubValue / MulValue, which coerce and wrap through the same vm.NumUnchecked*. The helper dispatch keys on the op keyword ahead of the op-string arms, because these ops share "+" "-" "*" with :add :sub :mul and must not fall into the promoting AddValue family.
  • Arity: the core fns are strictly binary, so binary-only-ops sends any other arity to a generic call and the runtime arity error still surfaces, instead of the n-ary fold or the (+ x) identity the other builtins get.

Generated artifacts (op_generated.go, core_compiled.lgb, the manifest and sums) are regenerated with make generate.

Adding opcodes changes the opcode-set signature, so bundles built by an older lg are rejected by this one. That is the same trade #235 made when it added OP_DIV; no migration entry is added, matching that precedent.

What it buys

Measured on a Knuth MMIX LCG step (unchecked-multiply then unchecked-add, 2,000,000 iterations, darwin/arm64, three interleaved rounds each):

path before after
interpreted, fast opcode vs var call (same binary) 351–363 ms 250–266 ms
lowered and linked as a Go override 94–102 ms 39–40 ms

The lowered gain needs the leaf to carry a ^long hint so its params lower to int; the entry point stays untyped so it remains override-eligible.

For the pure xxh3 as written (untyped helpers, every call boxed) the whole-hash time did not move, about 75–85 ms per 20,000 hashes either way. There the cost is the boxed call boundary between its fifty small defns, the boundary ABI cost tracked in #722, and this change does not reach it.

Verification

  • New lowering tests in pkg/ir/lisp_lower_go_test.go: int/int emits the native operator and no runtime helper; a boxed operand emits the Unchecked*Value helper and never the promoting one.
  • New test/core_tier3_test.lg cases pin the opcode against the core fn: wrap at Long/MAX_VALUE, checked + - * still overflow on the same inputs, a shadowing local wins, and a wrong arity reaches the arity error. A parity case runs each coercing input (1.5, 2.9, true, 1/2) and each rejected one (a string, nil, a BigInt past int64) through both the direct call and apply, so the opcode and the var are asserted equal rather than assumed; unchecked-inc and unchecked-dec on a float are covered because core.lg composes them from these ops.
  • make test and make check-generated pass; builds for linux/amd64, js/wasm, plan9/amd64, and wasip1/wasm.

@mparrett
mparrett requested a review from nnunley September 6, 2026 23:40
@mparrett
mparrett marked this pull request as ready for review September 6, 2026 23:52
@mparrett
mparrett requested a review from nooga September 7, 2026 04:28
@mparrett
mparrett force-pushed the wt/ir-unchecked-lowering branch from d41bb3c to 149cf9b Compare September 7, 2026 13:22
@nooga

nooga commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Review summary

The IR/opcode/lowering plumbing itself is solid — traced tryFastOpcode's shadowing guard, the binary-only-ops arity fallback, the op-str dispatch-before-op-string-arms trick that keeps unchecked-* from falling into the promoting AddValue/SubValue/MulValue family, and the type inference/constfold/purity wiring. All of it does what the PR description says, go build/go vet are clean, go test ./pkg/... is green (aside from an unrelated worktree-path quirk in pkg/genmanifest's go-deps test), the new test/core_tier3_test.lg and pkg/ir/lisp_lower_go_test.go cases all pass, and go generate ./pkg/rt/ reproduces core_compiled.lgb/manifest/sums byte-for-byte.

But there's a real correctness bug in the headline claim. The PR says: "The handlers use the same int64 round trip and error wording as CoreUncheckedAdd and friends in pkg/rt/lang.go, so the opcode and the var never disagree." That's not true for any operand that's a Float/BigInt/Boolean/Ratio/BigDecimal:

  • The new VM opcode (pkg/vm/vm.go, the OP_UNCHECKED_ADD/SUB/MUL case) does a strict a.(Int) type assertion — anything else is a hard type error.
  • CoreUncheckedAdd/Subtract/Multiply in pkg/rt/lang.go — called by the generic var path and by UncheckedAddValue/SubValue/MulValue in pkg/rt/golower_runtime.go (the AOT boxed-operand path) — use vm.ToInt(), which silently coerces Float/BigInt/Boolean/Ratio/BigDecimal instead of rejecting them.

Confirmed by direct execution:

(unchecked-add 1.5 2)             ;; direct call, hits the fast opcode
;; => error: unchecked-add expected integer, got Float

(apply unchecked-add [1.5 2])     ;; same op, bypasses the fast-opcode compiler gate
;; => 3

Whether unchecked-add throws or silently coerces now depends on incidental call shape (direct unqualified binary call vs. apply, stored-in-a-var, wrong arity, etc.), not on the arguments. unchecked-ops-reject-non-integers-test in test/core_tier3_test.lg only exercises the direct fast-opcode form, so it shipped green while asserting a premise ("the opcode keeps the core fn's Int-only contract") that isn't actually true of the core fn.

I also traced the same permissive integer?/vm.ToInt gap into pkg/rt/core/ir/passes/constfold.lg's new fold entries (integer? is true for BigInt too), which could let a foldable BigInt literal silently truncate instead of erroring at compile time — same root cause, different path. I couldn't reproduce that second one through an ordinary top-level defn (plain or ^long-hinted both threw correctly), so it may only be reachable through a narrower IR-optimize tier (deftype/protocol methods, gogen_ir AOT) — flagging for your awareness rather than as a fully confirmed second bug.

Suggested fix: tighten CoreUncheckedAdd/Subtract/Multiply in pkg/rt/lang.go to reject non-Int the same way the opcode does (a strict type switch instead of vm.ToInt), rather than loosening the opcode — the PR's own framing ("Int-only in let-go, the core fns reject anything but Int") says that was already the intended contract, it's just not what the code currently does.

One more thing worth a sentence in the PR body, not a blocker: the opcode-set signature bump with no new migration entry (bundles from a pre-#811 lg get rejected outright rather than migrated) is intentional per your own description and matches the file's documented reject-with-clear-error fallback — just flagging that I checked it and it's a controlled failure, not silent corruption, so it doesn't need action beyond what's already written up.

Requesting changes on the Int-strictness mismatch above — everything else here looks ready to merge once that's resolved.

@nooga nooga left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unchecked-* fast opcode is stricter than the core fn/var it's supposed to mirror (Float/BigInt/Boolean/Ratio silently coerce off the fast path, hard-error on it) — see summary comment for repro and suggested fix.

@mparrett

mparrett commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed at the new head, toward the var rather than away from it. Your suggestion went the other way, so here is the reasoning.

You were right about the mismatch, and it was worse than the apply case: unchecked-inc and unchecked-dec are (unchecked-add x 1) in core.lg, so they compiled to the strict opcode too and (unchecked-inc 1.5) went from 2 on main to a type error on this branch. The "Int-only, the core fns reject anything else" line in the body was mine and wrong; #64, which added the family, states the contract as "Float and BigInt inputs coerce to int64" and tests it. Tightening the core fns would have rewritten that decision inside an opcode PR, and it would have changed unchecked-inc on floats for everyone.

So the opcode now has the same shape as OP_ADD: Int/Int inline, and everything else falls back to the generic implementation. That implementation moved into pkg/vm as NumUncheckedAdd / Subtract / Multiply, and it is the only one. CoreUnchecked* in lang.go and the AOT Unchecked*Value helpers both call it, so the three paths cannot drift again. The tier3 test now runs each coercing input and each rejected one through both the direct call and apply and asserts the results equal; with the opcode forced strict again it fails, so it exercises the mismatch.

On constfold: with coercion as the runtime contract, folding through the host's unchecked-add yields the value the runtime would, and a BigInt past int64 throws inside the fold, which the surrounding try turns into "leave as IR". It then errors at runtime exactly as unfolded code does, so there is no truncation path. I could not reach a fold of such a literal either, and I agree it is worth knowing rather than a bug.

Separately: Clojure's (unchecked-add 1.5 2) is 3.5, not 3, so the #64 coercion is itself a divergence, and docs/KNOWN_DIVERGENCES.md does not list it. That is a question for the catalog rather than for this PR.

On the opcode-set signature bump: agreed that it is the controlled reject-with-error path, and the body's paragraph on it stands as written.

@mparrett
mparrett requested a review from nooga September 7, 2026 16:53
@mparrett
mparrett force-pushed the wt/ir-unchecked-lowering branch from 92ca395 to d51763f Compare September 7, 2026 20:10
mparrett and others added 2 commits September 7, 2026 13:57
unchecked-add, unchecked-subtract and unchecked-multiply reached the IR
only as generic calls to the core var, so the Go lowerer emitted a
trampoline (or a NativePrimsIntact prim call) and typeinfer learned
nothing about the result. Any kernel written on wrapping arithmetic
lost the typed native path exactly where it matters.

- VM: OP_UNCHECKED_ADD/SUB/MUL, handlers mirror CoreUnchecked* (same
  int64 round trip and error wording). The bytecode compiler emits them
  for binary unqualified calls through the existing fast-opcode gate.
- IR: catalog rows, builtin-ops entries, an UncheckedOp type that types
  like the bitwise ops (int/int -> :int, no contagion), bytecode and Go
  lowering, purity, constfold and commutative membership. int/int lowers
  to native Go + - * (which already wraps); anything else routes to
  rt.Unchecked<Op>Value, dispatched on op-kw ahead of the op-str arms so
  it never falls into the promoting AddValue/SubValue/MulValue.
- Arity != 2 falls back to a generic call so the runtime arity error
  still surfaces (binary-only-ops).

Tests: lowering (native + helper path), tier3 opcode semantics (wrap,
checked ops still overflow, non-int rejection, shadowing, wrong arity).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… var

The OP_UNCHECKED_* handlers asserted Int and hard-errored on anything else,
while the core fns coerce Float, Boolean, Ratio and int64-range BigInt
through vm.ToInt (the #64 contract). So (unchecked-add 1.5 2) errored as a
direct call and returned 3 through apply, and core.lg's unchecked-inc and
unchecked-dec, which compile to the opcode, broke on floats.

The opcode now has the same shape as OP_ADD: Int/Int inline, everything
else through the generic implementation. That implementation moves into
pkg/vm as NumUncheckedAdd/Subtract/Multiply and becomes the only one:
CoreUnchecked* in lang.go and the AOT Unchecked*Value helpers call it, so
the opcode, the var and lowered code cannot drift.

The tier3 test now runs each coercing and each rejected input through both
the direct call and apply and asserts the results equal; it fails with the
opcode forced strict.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mparrett
mparrett force-pushed the wt/ir-unchecked-lowering branch from d51763f to 7d82da2 Compare September 7, 2026 20:57

@nooga nooga left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mparrett Nice fix — routing the opcode through the same NumUncheckedAdd/Subtract/Multiply path as the var/AOT cases (instead of just tightening the core fn, which would've silently changed unchecked-inc/dec on floats) closes the gap more thoroughly than what was asked. Ran the full .lg suite plus the new unchecked_arith_test.lg cases — all green, including the exact repro from the review thread via both direct-call and apply. Green light.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants