-
Notifications
You must be signed in to change notification settings - Fork 18
Description
Description
PR #360 (salsa consolidation) added infrastructure for compiling PREVIOUS and INIT builtins directly to single opcodes (LoadPrev/LoadInitial) with corresponding BuiltinFn::Previous/BuiltinFn::Init variants and VM snapshot buffers (prev_values, initial_values). However, in builtins_visitor.rs, PREVIOUS and INIT still fall through to the module expansion path rather than using this new single-opcode compilation.
As a result:
- The opcode infrastructure (
LoadPrev/LoadInitial) adds complexity but is not exercised for user models. - The
prev_valuesandinitial_valuessnapshot buffers are allocated but never read viaLoadPrev/LoadInitialduring simulation. PREVIOUSandINITcalls still require module instantiation, which is more expensive than a direct opcode.
Why it matters
- Correctness/clarity: Scaffolded-but-unused code paths are confusing for maintainers -- it is unclear whether the opcodes are intended to work or are aspirational.
- Compilation efficiency: Module expansion for
PREVIOUS/INITcreates additional synthetic variables and module instances. A single-opcode path avoids that overhead. - Memory: The snapshot buffers are allocated regardless, so activating the opcode path would make the allocation worthwhile.
Components affected
src/simlin-engine/src/builtins_visitor.rs(activation point -- the "fall through to module expansion" path)src/simlin-engine/src/bytecode.rs(LoadPrev,LoadInitialopcodes)src/simlin-engine/src/vm.rs(snapshot buffer usage)
Possible approach
In builtins_visitor.rs, when encountering PREVIOUS(x) or INIT(x), emit the corresponding LoadPrev/LoadInitial opcode instead of falling through to module expansion. This requires ensuring the snapshot buffers are correctly populated during VM initialization and that the opcode semantics match the module-expansion behavior for all edge cases (e.g., arrays, error handling).
Context
Identified during review of PR #360 (engine: consolidate compilation onto salsa incremental path).