Repository navigation
feat(ir): fold branches with a known test; infer self-recursive returns - #1087
Conversation
nooga
left a comment
nooga
left a comment
There was a problem hiding this comment.
Self-review: blocking before merge.
The branch fold trusts param types that come from how a param is used. known-truth in pkg/rt/core/ir/passes/branchfold.lg folds a :branch-if whenever the test's inferred type is always truthy. But infer-arg-types gives a param :int just because the param is used in +/* or a comparison. That is the same fact #1076 runs into. So the else arm gets deleted and nil/false arguments throw:
(require 'ir.passes.pipeline)
(binding [*ir-compile* true]
(eval '(defn g1 [n] (if n (+ n 1) 0)))
(eval '(defn g2 [x] (when x (* x 2)))))
(g1 nil) (g1 false) (g2 nil)This PR: cannot add nil and Int, cannot add Boolean and Int, cannot multiply nil and Int. Bytecode and main+#1076: 0 0 nil.
Fix: fold by type only on facts that hold (constants, results of typed ops, the nil/false types). Don't fold on types inferred for args, including tests derived from a LoadArg. Add g1/g2 as regressions in test/ir_branchfold_test.lg.
Other notes:
truthy-type?andfalsey-type?both return true for an empty[:union](every?over an empty seq). Harmless today because truthy is checked first, but fragile now that the answer decides CFG deletion.- Turning return inference on by default reverses #662's opt-in. The body flags that it needs owner sign-off, so record that sign-off explicitly in this thread once the fold is fixed.
- The literal-test fold in ifCompiler and the self-recursive guess-and-confirm look sound. fib, float/string/nil-returning recursions, a str-wrapped recursion and cond+:else all match bytecode.
mparrett
left a comment
mparrett
left a comment
There was a problem hiding this comment.
I recommend fixing both findings below before merging. Branch folding changes which arm executes for a valid nil input, and enabling recursive return inference by default introduces silent integer overflow in previously checked code.
Reviewed c98e7cf against base b825882. Build and smoke checks passed. The new branchfold suite passed 9 tests / 39 assertions, and the four focused recursive return inference tests passed. Both regressions were reproduced against the PR and its base.
| (let [o (ir/op nid f)] | ||
| (cond | ||
| (= :load-arg o) true | ||
| (or (not= :block-arg o) (contains? seen nid)) false |
There was a problem hiding this comment.
[P1] Trace argument-derived types through value-preserving calls
The argument-origin guard stops at any op other than load-arg/block-arg, but type inference also propagates an argument's type through aset, which returns its value argument unchanged. For (defn f [a x] (if (aset a 0 x) (+ x 1) :no)), arithmetic use infers x as :int, then aset inherits :int and its branch is folded to true. Calling (f (object-array 1) nil) returns :no through ordinary bytecode and through the base revision's IR compiler, but throws "cannot add nil and let-go.lang.Int" through this revision's strict IR compiler. Please preserve the branch when a result's inferred type depends on an unproven argument type, including through aset, and add this case to the VM/IR parity tests.
There was a problem hiding this comment.
Fixed in b829168. arg-derived? now follows the calls typeinfer copies a type through. That set comes from the new typeinfer/type-source (aset → its value argument, plus the transient ops, aclone and aget), so the two can't drift apart. It also treats a self-call's result as unproven, since that type was only confirmed against the same arg-inferred param types. Your case (f (object-array 1) nil) now returns :no under strict IR, and it's in the VM/IR parity tests as bf-arg-aset.
| (lower-go/returns-as? f decl) | ||
| (or (not (has-self-call? f)) | ||
| (some (fn [t] (lower-go/returns-as? f t)) self-return-guesses))) | ||
| (or (some (fn [t] (infer-with-self-return! f fname t)) |
There was a problem hiding this comment.
[P1] Preserve checked arithmetic before enabling return inference by default
The new default guess moves previously boxed recursive arithmetic onto unchecked Go operators. A concrete regression is (defn step [n] (if (zero? n) 9223372036854775807 (+ (step (dec n)) 1))): native compilation on the base revision keeps the recursive result boxed and calls rt.AddValue, so (step 1) raises overflow and can be caught. On this PR, Step returns int64 and the generated addition is a raw arg__2 + 1; the native binary silently returns -9223372036854775808. I reproduced both native binaries. Although unchecked typed arithmetic predates this PR, this unhinted program only starts taking that path because of the new default inference. Please land checked integer lowering first, or keep the affected inference opt-in until arithmetic semantics are preserved.
There was a problem hiding this comment.
Agreed. In b829168 the unhinted :int/:float guesses are behind ir.passes.pipeline/*infer-self-returns*, off by default, until checked integer lowering lands. A declared return type is still checked against the body, so the ^long miscompile stays fixed. TestUnhintedRecursionStillBoxes is back, and the inference tests opt in. Your step shape lowers boxed through rt.AddValue again. make check-selfhost holds.
mparrett
left a comment
mparrett
left a comment
There was a problem hiding this comment.
Follow-up re-review: both blockers from my previous review are addressed at b829168.
- The strict IR-compiled aset reproduction now returns :no for nil, matching ordinary bytecode. Argument-derived type tracking now follows the value through the call.
- The original unhinted recursive overflow reproduction, compiled to a native binary at this revision, catches overflow again. Unhinted self-return inference is opt-in, so the default path retains checked rt.AddValue.
Re-ran the original reproductions and focused tests: the branchfold suite passed 9 tests / 42 assertions, and all five selected recursive return inference tests passed. The earlier build and smoke checks also passed at this same head. No remaining blockers from my review; clearing my previous request for changes.
cond's :else arm compiles to (if :else x nil). Nothing folded it, so the IR kept a dead nil arm feeding the join, and a recursive fn like Niva's fib lowered with a boxed vm.Value return and rt.AddValue. - ir.passes.branchfold: a branch-if whose test is a constant, or whose inferred type is always truthy or always falsey, becomes a branch; blocks left unreachable are detached and single-predecessor chains spliced. Runs after constfold-1 and constfold-3, then re-infers types from scratch when it changed anything. - A self-call's return type is now a guess the body must confirm: the declared ^long/^double when present, else :int, then :float. A contradicted guess is dropped. This fixes the ^long miscompile where the self-call was typed int64 while the fn returned vm.Value, and it types unhinted int recursion. A self-call must hit the same var (compared by resolution) and the same arity. - pkg/compiler: an if with a scalar literal test skips the test and branch; the dead arm is still compiled so its errors surface. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
infer-arg-types types a param :int because it feeds arithmetic, not because the caller passes an Int, so (if n (+ n 1) 0) lost its else arm under *ir-compile* and threw on nil/false. known-truth now skips type-based folding for a :load-arg or a block param an argument flows into; constants and the results of typed ops still fold. truthy-type?/falsey-type? no longer answer true for an empty union. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ses opt-in aset returns its value argument and typeinfer copies that operand's type, so (if (aset a 0 x) (+ x 1) :no) still folded on x's arg-inferred :int and threw on a nil x. arg-derived? now follows the calls typeinfer copies a type through (typeinfer/type-source) and treats self-call results as unproven. The unhinted :int/:float self-return guesses move behind *infer-self-returns*: a guessed int64 return puts recursive arithmetic on Go's wrapping + where rt.AddValue throws on overflow, so they wait for checked integer lowering. A declared return type is still checked against the body, which keeps the ^long miscompile fixed. TestUnhintedRecursionStillBoxes is back; the inference tests opt in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b829168 to
d2d6a0c
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Perf (pkg/vm micros) — base vs head, same runnerBase bench-ratchet — 16 regression(s) > 12.0% budget, 0 missing, 0 new
Showing 16 of 53 benchmarks (past budget). Full table in the run summary → |
Repeat A/B (variance-reduced) — informational shadow checkPer-cycle logs are in the job summary. Per-snapshot evidence — raw samples, cycle order, provenance — is in the Informational only. This check never gates the PR — the workflow header says why. |
|
@nooga looks like a false positive on the perf regression. |
Why
Niva compiles
fibto Clojure as acondwith an:elsearm. Lowered to Go withlg compile, it ran at 2.02× the time of Niva's own direct-to-Go output. The lowered fn had an unboxedint64param but returned a boxedvm.Value, and added withrt.AddValue. Two things caused that::elseis never folded.condexpands its last arm to(if :else x nil).build-ifemits a:branch-iffor it, and neither the IR passes nor the bytecodeifCompilerdrops the arm that can't run. So a constantnilkeeps feeding the join, and the fn's return type becomes int-or-nil.:unknownunless the fn has a return hint (AOT lowering: returns stay boxed (vm.Value+rt.AddValue) even for recursive int fns — ~3× off pure Go #599/feat(ir): seed self-recursive calls from a declared return type (#599) #662), so(+ (fib …) (fib …))stays boxed either way.The
^longhint was also broken in this exact shape. The self-calls were typedint64from the hint, while the fn's real Go return type wasvm.Value(because of the nil arm), and the generated Go did not compile:What changes
ir.passes.branchfold(new). A:branch-ifwhose test is a constant, or whose inferred type is always truthy or always falsey, becomes a:branch. A type counts only when it doesn't rest on an argument's: tests that trace back to a:load-arg(through block params, type-copying calls likeaset, or self-calls) keep both arms, because infer-arg-types types a param:intjust because it feeds arithmetic. The changes:It runs after
constfold-1and again afterconstfold-3. When it changes something, it resets types and runs a typeinfra epoch straight away. That has to happen because typeinfer's facts only widen from their seed, so a join that lost an edge would otherwise keep its old, wider type.uniform-pass-list(lambda-lifted siblings) folds without re-inferring, which keeps their uniform ABI.Self-call return types are a guess the body has to confirm. The guess is the declared return type if there is one. With
ir.passes.pipeline/*infer-self-returns*bound true, an unhinted fn also tries:int, then:float. A guess is kept only if every return value then lowers to that Go type (lower-go/returns-as?). That's sound by induction on call depth: the fn really returns T, which is what each self-call site assumed. A contradicted guess is dropped and the self-calls stay boxed. This fixes the^longmiscompile above.core/conjinside an(ns foo) (defn conj …)wrapper doesn't count) and land on the same arity (one arity of a multi-arity defn calling another doesn't count; variadic fns are left out).collect-ns-exportsrecords cross-package signatures without the declared-types binding.pkg/compiler. Anifwhose test is a scalar literal (keyword, number, string, char, boolean, nil) skips the test and theBRANCH_FALSE. The arm that can't run is still compiled, behind aJUMP, so its compile errors surface exactly as before.Return inference stays opt-in
An earlier revision made the unhinted guesses the default, which reversed #662's opt-in. Review showed that a guessed
int64return moves unhinted recursive arithmetic from checkedrt.AddValueonto Go's wrapping+, so it would silently overflow where it used to throw. The guesses are now behind*infer-self-returns*until checked integer lowering lands.TestUnhintedRecursionStillBoxesis back, and the inference tests opt in. Declared return types are still checked against the body, which fixes the^longmiscompile.Position on the three paths
.lgbshrinks from 321,667 to 320,991 bytes.*ir-compile*: getsbranchfoldand the return guess throughoptimize-fn.-tags gogen_ir/lg compile: same pipeline. This is where the boxing goes away for declared returns, or unhinted ones with*infer-self-returns*on.Numbers
Niva's
fib(36)benchmark program, Apple M1 Pro, hyperfine. Both builds aremake generate && make buildwith #1085 stacked on top, because Niva's runtime needs those shims. They differ only in this PR.main+ #1085lg compile), 20 runsThese numbers were measured with the unhinted guesses on by default. With them opt-in, unhinted
fibgets the:elsefold but keeps a boxed return unless it declares^longor opts in. These are same-machine runs, so read them as the ratio and the mechanism, not as ratchet claims.make bench-ratchetpasses its deterministic gate (allocs and bytes within 2%, IR compile included). There is no timing baseline for this machine profile, so CI's runners will have to do the timing check.Verification
test/ir_branchfold_test.lg, which checks both the folded shape and that the IR-compiled fn returns the same as the VM-compiled one. Cases:cond … :else, with and without recursion;loop;pkg/ir/lisp_declared_return_test.go: unhinted inference,cond … :else(novm.Keyword("else")survives), and a contradicted^longthat stays boxed.make testgo test -short ./...go test ./test/ -run TestClojureTestSuitego test -tags gogen_ir ./pkg/irgo test -tags bootstrap -short ./...make lowered && make test SHORT=false PKG=./test/e2e RUN="TestEngineParityGate|TestGogenAOTDiff"make check-selfhostmake check-generatedmake check-generated-manifestNot in this PR
Typed
int64+/-/*lower to raw Go operators, which wrap on overflow, while the VM throwsinteger overflow. That predates this PR (an unhinted(- n 2)on an int param already lowers raw), but return inference means more code takes that path. It should get its own fix.🤖 Generated with Claude Code