Remove unused typed local opcodes from compiler and VM#74
Conversation
The 10 typed local opcodes (OP_GET_LOCAL_INT..OP_SET_LOCAL_REF) had identical implementations to OP_GET_LOCAL/OP_SET_LOCAL — plain register copies with no type-specific behavior. The optimization decision (OP_ADD_FLOAT vs OP_RT_ADD) comes from ExpressionType at compile time, not from the load opcode. Removing these simplifies the dispatch surface without changing any optimization behavior. Changes: - Remove TypedGetLocalOp/TypedSetLocalOp helpers from compiler - Compiler now always emits OP_GET_LOCAL/OP_SET_LOCAL - Remove typed local case arms from VM ExecuteLoop and ExecuteCoreOp - Remove typed local entries from WASM translator - Opcode enum entries preserved for .sbc backward compatibility Made-with: Cursor
📝 WalkthroughWalkthroughRemoves all type-specific local load/store opcodes and related helpers; compiler, Wasm translator, VM, and docs now use only generic Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler as Compiler
participant Translator as WasmTranslator
participant VM as VM
Compiler->>Translator: emit OP_GET_LOCAL / OP_SET_LOCAL for a local slot
Translator->>VM: translate to wasm local.get / local.set (or bytecode stream)
VM->>VM: Execute generic OP_GET_LOCAL / OP_SET_LOCAL\n(access slot without per-type branch)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
souffle/Souffle.Wasm.Translator.pas (1)
638-647:⚠️ Potential issue | 🔴 CriticalKeep the legacy typed-local opcodes mapped here.
Souffle.Bytecode.pas:101-111still declaresOP_GET_LOCAL_INTthroughOP_SET_LOCAL_REFfor old.sbccompatibility. After this change, those legacy opcodes are no longer recognized by the Wasm translator and will fall into the defaultEmitUnreachablepath, so translating persisted bytecode that predates this PR will break.🔧 Minimal fix
- OP_GET_LOCAL: + OP_GET_LOCAL, + OP_GET_LOCAL_INT, + OP_GET_LOCAL_FLOAT, + OP_GET_LOCAL_BOOL, + OP_GET_LOCAL_STRING, + OP_GET_LOCAL_REF: begin ABuilder.EmitLocalGet(Bx); ABuilder.EmitLocalSet(A); end; - OP_SET_LOCAL: + OP_SET_LOCAL, + OP_SET_LOCAL_INT, + OP_SET_LOCAL_FLOAT, + OP_SET_LOCAL_BOOL, + OP_SET_LOCAL_STRING, + OP_SET_LOCAL_REF: begin ABuilder.EmitLocalGet(A); ABuilder.EmitLocalSet(Bx); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@souffle/Souffle.Wasm.Translator.pas` around lines 638 - 647, The Wasm translator removed handling for legacy typed-local opcodes (e.g., OP_GET_LOCAL_INT .. OP_SET_LOCAL_REF) so older .sbc files will fall through to EmitUnreachable; restore support by adding case branches mapping each legacy opcode to the same actions as the generic handlers (use EmitLocalGet/EmitLocalSet with the appropriate operands), i.e., implement cases for OP_GET_LOCAL_INT, OP_GET_LOCAL_REF, OP_SET_LOCAL_INT, OP_SET_LOCAL_REF (and any other legacy OP_GET/OP_SET variants declared in Souffle.Bytecode.pas) that call ABuilder.EmitLocalGet(...) and ABuilder.EmitLocalSet(...) the same way OP_GET_LOCAL and OP_SET_LOCAL do so legacy bytecode is recognized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@souffle/Souffle.Wasm.Translator.pas`:
- Around line 638-647: The Wasm translator removed handling for legacy
typed-local opcodes (e.g., OP_GET_LOCAL_INT .. OP_SET_LOCAL_REF) so older .sbc
files will fall through to EmitUnreachable; restore support by adding case
branches mapping each legacy opcode to the same actions as the generic handlers
(use EmitLocalGet/EmitLocalSet with the appropriate operands), i.e., implement
cases for OP_GET_LOCAL_INT, OP_GET_LOCAL_REF, OP_SET_LOCAL_INT, OP_SET_LOCAL_REF
(and any other legacy OP_GET/OP_SET variants declared in Souffle.Bytecode.pas)
that call ABuilder.EmitLocalGet(...) and ABuilder.EmitLocalSet(...) the same way
OP_GET_LOCAL and OP_SET_LOCAL do so legacy bytecode is recognized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0597decc-fbe2-4459-a801-951d00309e7b
📒 Files selected for processing (3)
souffle/Souffle.VM.passouffle/Souffle.Wasm.Translator.pasunits/Goccia.Compiler.Expressions.pas
💤 Files with no reviewable changes (1)
- souffle/Souffle.VM.pas
|
TODO: Remove opcode enums entirely - Backwards compatibility is not a priority at the moment and we should up the SBC version number |
Suite Timing
Measured on ubuntu-latest x64. |
Benchmark Results254 benchmarks Interpreted: 🟢 46 improved · 🔴 1 regressed · 207 unchanged · avg +4.7% arraybuffer.js — Interp: 🟢 2, 12 unch. · avg +3.9% · Bytecode: 14 unch. · avg -2.1%
arrays.js — Interp: 🟢 4, 15 unch. · avg +3.3% · Bytecode: 19 unch. · avg +1.2%
async-await.js — Interp: 🟢 3, 3 unch. · avg +6.5% · Bytecode: 6 unch. · avg +0.7%
classes.js — Interp: 31 unch. · avg +3.8% · Bytecode: 31 unch. · avg +0.5%
closures.js — Interp: 🟢 1, 10 unch. · avg +4.7% · Bytecode: 11 unch. · avg +0.5%
collections.js — Interp: 🟢 2, 10 unch. · avg +2.7% · Bytecode: 12 unch. · avg +1.4%
destructuring.js — Interp: 🟢 10, 12 unch. · avg +6.6% · Bytecode: 22 unch. · avg +1.7%
fibonacci.js — Interp: 8 unch. · avg +3.6% · Bytecode: 🟢 1, 7 unch. · avg +2.6%
for-of.js — Interp: 7 unch. · avg +4.5% · Bytecode: 7 unch. · avg -1.1%
iterators.js — Interp: 🟢 7, 13 unch. · avg +6.8% · Bytecode: 20 unch. · avg +2.2%
json.js — Interp: 🟢 1, 19 unch. · avg +3.7% · Bytecode: 🔴 1, 19 unch. · avg -0.0%
jsx.jsx — Interp: 🟢 1, 20 unch. · avg +4.9% · Bytecode: 21 unch. · avg +2.4%
numbers.js — Interp: 11 unch. · avg +2.2% · Bytecode: 🟢 1, 10 unch. · avg +0.9%
objects.js — Interp: 🟢 1, 6 unch. · avg +5.5% · Bytecode: 🟢 1, 6 unch. · avg +3.9%
promises.js — Interp: 🟢 12 · avg +11.0% · Bytecode: 🟢 1, 11 unch. · avg +1.9%
strings.js — Interp: 🔴 1, 10 unch. · avg +2.7% · Bytecode: 🟢 2, 9 unch. · avg +2.3%
typed-arrays.js — Interp: 🟢 2, 20 unch. · avg +3.7% · Bytecode: 🟢 2, 🔴 1, 19 unch. · avg +0.3%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
The typed local opcodes (56-65) are no longer emitted by the compiler and had no case arms in the VM. Remove the dead enum entries entirely and bump SOUFFLE_FORMAT_VERSION from 3 to 4. Update documentation to reflect that local access always uses OP_GET_LOCAL/OP_SET_LOCAL. Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
souffle/Souffle.Bytecode.pas (1)
98-106: Consider adding a comment documenting the removed typed local opcodes (56-65).Similar to the comment on line 104 about opcode 68, a brief note about the removed typed local opcodes would help future maintainers understand the gap between
OP_CONCAT = 55andOP_NEW_BLUEPRINT = 66.📝 Optional documentation comment
// ── Core: String ── OP_CONCAT = 55, // ABC R[A] := String(R[B]) + String(R[C]) + // opcodes 56-65 removed: were OP_GET_LOCAL_INT through OP_SET_LOCAL_REF (typed local opcodes) + // use OP_GET_LOCAL/OP_SET_LOCAL instead — type optimization is via arithmetic/compare opcodes + // ── Core: Blueprint ── OP_NEW_BLUEPRINT = 66, // ABx R[A] := new Blueprint(name=Constants[Bx])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@souffle/Souffle.Bytecode.pas` around lines 98 - 106, Add a brief comment between OP_CONCAT and OP_NEW_BLUEPRINT in Souffle.Bytecode.pas documenting that opcode slots 56–65 were removed (the typed-local opcodes) and why/what replaced them—mirroring the style of the existing comment for the removed OP_BLUEPRINT_METHOD (68); reference the surrounding symbols OP_CONCAT, OP_NEW_BLUEPRINT and the removed range 56-65 so future maintainers understand the numeric gap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@souffle/Souffle.Bytecode.pas`:
- Around line 98-106: Add a brief comment between OP_CONCAT and OP_NEW_BLUEPRINT
in Souffle.Bytecode.pas documenting that opcode slots 56–65 were removed (the
typed-local opcodes) and why/what replaced them—mirroring the style of the
existing comment for the removed OP_BLUEPRINT_METHOD (68); reference the
surrounding symbols OP_CONCAT, OP_NEW_BLUEPRINT and the removed range 56-65 so
future maintainers understand the numeric gap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4ac18b4-dba1-46fb-8d54-825d0a8a1239
📒 Files selected for processing (2)
docs/souffle-vm.mdsouffle/Souffle.Bytecode.pas
- Rewrite optimization-log.md with correct PR numbers for all 22 PRs since #73 (previous version had multiple wrong PR-to-description mappings) - Add missing PRs: #73, #74, #75, #76, #77, #79, #80, #81, #82, #83, #85, #88, #89, plus infrastructure PRs #90, #92, #98 - Fix GC section: Collect-after-each-file is deliberate, not a workaround; any GC revisit needs fresh analysis - Fix minor items: link to actual issues #103 and #104, correct naming convention context (shared units should NOT use Goccia.* prefix) - Create docs/decision-log.md with 1-3 sentence summaries linking to detailed documentation - Link fpc-generics-performance.md and fpc-dispatch-performance.md from AGENTS.md, code-style.md, design-decisions.md, and optimization-log.md Made-with: Cursor
…105) * Convert spike PDFs to Markdown, update VM docs, add optimization log - Convert all 4 PDF spike documents (string performance, hashmap performance, generics performance, dispatch performance) to Markdown for better AI agent parsing - Fix stale TSouffleValue documentation: 26 bytes → 16 bytes, string[23] → string[13] across souffle-vm.md, design-decisions.md - Update test count to 3,501 across all docs (AGENTS.md, README.md, souffle-vm.md, design-decisions.md) - Add svkString to TSouffleValue kinds list in AGENTS.md - Add THashMap optimization details to AGENTS.md component table - Add docs/optimization-log.md with full VM optimization history, experiment results, remaining work, and key insights for fresh context pickup - Add hashmap spike update note documenting TScopeMap removal, TOrderedStringMap refactoring, and THashMap hash optimization Made-with: Cursor * Fix optimization log PR numbers, add decision log, link all spikes - Rewrite optimization-log.md with correct PR numbers for all 22 PRs since #73 (previous version had multiple wrong PR-to-description mappings) - Add missing PRs: #73, #74, #75, #76, #77, #79, #80, #81, #82, #83, #85, #88, #89, plus infrastructure PRs #90, #92, #98 - Fix GC section: Collect-after-each-file is deliberate, not a workaround; any GC revisit needs fresh analysis - Fix minor items: link to actual issues #103 and #104, correct naming convention context (shared units should NOT use Goccia.* prefix) - Create docs/decision-log.md with 1-3 sentence summaries linking to detailed documentation - Link fpc-generics-performance.md and fpc-dispatch-performance.md from AGENTS.md, code-style.md, design-decisions.md, and optimization-log.md Made-with: Cursor * Address PR review comments: fix ratios, bytes/chars, stale refs - Fix generics spike ratio values (arithmetic errors from original PDF) - Change "13 characters" to "13 bytes" in souffle-vm.md (FPC string[N] is byte-counted), add UTF-8 note - Mark TScopeMap as removed/historical in hashmap spike document - Fix Key Insight #4 PR reference (was #88 async/await, not boolean) - Remove stale GOCCIA_EXT_EVAL_CLASS / FPendingClasses / decorator bridge references from AGENTS.md, souffle-vm.md, design-decisions.md (decorators are now compiled natively via BEGIN/APPLY/FINISH opcodes) - Fix "44 second" → "44-second" hyphenation in string spike Made-with: Cursor * Expand TScopeMap history in hashmap spike with full timeline The spike originally recommended TScopeMap for scope bindings based on micro-benchmarks at N≤10. After implementation (PR #66), real-world profiling showed the assumption about scope sizes was incorrect — global and bridged scopes far exceed 20 bindings, causing a 2.7× regression. The update section now tells the complete timeline: original recommendation → implementation → profiling discovery → revert. Made-with: Cursor * Add grill-me interview skill Introduce a new 'grill-me' agent skill that relentlessly interviews users about a plan or design, exploring each branch of the decision tree until a shared understanding is reached and dependencies are resolved. The skill prefers to inspect the codebase when that can answer a question. Updated skills lock to include the new skill.
Summary
ExpressionTypeat compile time, not from which load opcode was used.Test plan
Made with Cursor
Summary by CodeRabbit
Refactor
Chores
Documentation