feat(program): add label-based program builder#76
Conversation
Authoring minivm bytecode by hand required computing PC-relative branch byte offsets and tracking constant/type pool indices manually, as seen in the duplicated patchBranch helper and hardcoded BR_IF offsets in the benchmarks. Add a label-patching assembler and ergonomic facade, modeled on LLVM's IRBuilder and classic two-pass assemblers: - instr.Builder: emit instructions and symbolic Labels, branch with Br/BrIf/BrTable, and Assemble to back-patch each branch into the signed 16-bit offset the interpreter expects. - program.Builder: wraps instr.Builder with interned constant and type pools (Const/ConstGet/Type dedup by String() and return a stable index). - types.FunctionBuilder: gains Label/Bind/Br/BrIf/BrTable for function bodies. Build now returns an error; MustBuild panics for fixtures. Migrate benchmarks/programs.go to the builders and delete patchBranch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
siyul-park
left a comment
There was a problem hiding this comment.
PR Review — feat(program): add label-based program builder
Decision
Approve with Minor Comments — pending CI completion.
Merge Readiness Summary
This PR is technically sound and merge-ready once CI passes. It adds a well-designed label-patching assembler (instr.Builder and program.Builder) that eliminates hand-computed branch offsets and constant pool indices. The implementation is correct, thoroughly tested, and all visible call sites have been migrated. However:
- CI is in_progress — The Ubuntu 24.04 @ Go 1.26.2 check started at 23:04:09Z but has not yet completed. Merge should await its completion.
- Breaking API change —
types.FunctionBuilder.Build()now returns(*Function, error). All in-repo call sites have been updated to either.Build()+ error check or.MustBuild()(for fixtures). If there are external consumers of this API, they will break.
Both are resolvable (waiting on CI is standard, breaking changes are acceptable if intentional), but should be explicitly confirmed before merge.
Readiness Checklist
- Scope control: Pass — feature is clearly scoped; no unrelated refactoring
- Correctness: Pass — offset calculation correct, label binding logic sound, error handling complete
- Test coverage: Pass — new tests cover forward/backward branches, BR_TABLE, error paths, constant/type dedup, and e2e integration
- Architecture consistency: Pass — follows LLVM IRBuilder pattern; two-pass assembler is idiomatic
- Risk / compatibility: Pass with caveat — breaking API change is intentional and all in-repo call sites updated
- Maintainability: Pass — clean, well-structured code; good naming and separation of concerns
- CI / validation: Pending — Ubuntu job in_progress; WIP check passed
Key Observations
Strengths
-
Correct offset calculation — The formula
offset = bytePos[target] − (bytePos[branch] + width)is correctly implemented ininstr.Builder.Assemble(), matching the interpreter's expectations. -
Comprehensive test coverage —
- Forward and backward branch resolution tested
- BR_TABLE with multiple targets tested
- Error paths (unbound label, offset overflow) covered
- Constant and type deduplication verified
- E2E tests (
BenchmarkJITIssue60) confirm migrated programs produce identical results through both interp and JIT
-
Clean API design — The builder pattern mirrors LLVM's IRBuilder; method chaining is idiomatic and readable. The two-pass approach (emit, then assemble) is a standard, proven pattern.
-
Thorough migration — All visible call sites to
FunctionBuilder.Build()have been updated; 40+ test locations changed consistently from.Build()to.MustBuild(). -
Documentation —
docs/architecture.mdupdated to explain the new builders and offset encoding.
Items to Confirm Before Merge
-
CI completion — Await the Ubuntu 24.04 @ Go 1.26.2 job. The mergeable_state is "unstable", which typically indicates pending CI. No test failures are visible yet.
-
External API consumers —
FunctionBuilder.Build()is now(fn *Function, err error)instead offn *Function. If this is part of a public module API, downstream packages may break. The PR description doesn't mention any follow-up or deprecation period, so verify this is intentional and that no external packages depend on the old signature. -
MustBuild vs Build clarity — The convention (
.MustBuild()for fixtures,.Build()with error checks in real code) is sound and well-applied throughout. No changes needed, but document this convention in the types package if not already done.
Questions
None that would block merge, but worth reviewing:
- Is the breaking change to
FunctionBuilder.Build()intentional, and have all external consumers (if any) been notified or updated? - The
interp.WithCutoff(1)removal in benchmarks — was this a stale option that actually caused test failures, or preventive cleanup? (Mentioned in PR description as "removed option that broke the bench test binary".)
Minimal Path to Merge
- Await completion of the Ubuntu 24.04 @ Go 1.26.2 CI job and confirm it passes.
- Confirm there are no external consumers of the
FunctionBuilder.Build()API that would break.
Suggested Follow-up Issues
None required for this PR. If the breaking change impacts external packages, consider creating a deprecation strategy in a future PR (e.g., aliasing Build to MustBuild with a deprecation warning for a release cycle), but that is optional and not blocking.
Final Recommendation
Merge after CI completes successfully. The code is correct, well-tested, and architecturally sound. The breaking API change is intentional and in-repo call sites are properly updated.
Generated by Claude Code
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
+ Coverage 63.36% 63.53% +0.16%
==========================================
Files 63 65 +2
Lines 11782 11898 +116
==========================================
+ Hits 7466 7559 +93
- Misses 3526 3546 +20
- Partials 790 793 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Changes Made
Hand-writing minivm bytecode meant computing PC-relative branch byte offsets and tracking constant/type pool indices by hand — see the duplicated
patchBranchhelper and the hardcodedBR_IF, 26in the benchmarks. This adds a label-patching assembler and an ergonomic facade, modeled on LLVM'sIRBuilderand classic two-pass assemblers.instr.Builder(new) — emit instructions and symbolicLabels, branch withBr/BrIf/BrTable, thenAssembleto back-patch each branch into the signed-16-bit byte offset the interpreter expects (relative to the end of the branch instruction). One rule covers every branch incl.BR_TABLE:offset = bytePos[target] − (bytePos[branch] + width). SentinelsErrUnboundLabel,ErrOffsetRange.program.Builder(new) — wrapsinstr.Builderwith interned constant/type pools:Const/ConstGet/Typededup byString()and return a stable index, so authors never track pool indices.Build() (*Program, error).types.FunctionBuilder— gainsLabel/Bind/Br/BrIf/BrTablefor function bodies (routes through an internalinstr.Builder).Buildnow returns anerror;MustBuildpanics for statically-known fixtures (likeregexp.MustCompile).Emitunchanged.benchmarks/programs.go— migrated to the builders;patchBranchand the hardcoded offsets deleted. Also drops a staleinterp.WithCutoff(1)(removed option) that broke the bench test binary.docs/architecture.md— documents the builder in theprogram/section.Before → after:
Related Issues
Additional Information
Test plan
go test -race ./...green across the repo.go vet ./...clean (main + benchmarks modules).BR_TABLE, unbound-label + offset-overflow errors, constant/type dedup, goldenProgram.String()match vs a hand-patched program.BenchmarkJITIssue60(indirect-fib, closure-counter, typed-array-sum) passes through interp and JIT — results asserted, so migrated programs are functionally identical.🤖 Generated with Claude Code