One lowering for every backend, one vocabulary for every script - #66
Open
ewowi wants to merge 2 commits into
Open
One lowering for every backend, one vocabulary for every script#66ewowi wants to merge 2 commits into
ewowi wants to merge 2 commits into
Conversation
The IR walk existed three times, once per target, and the two device copies differed by two identifier tokens. It is now written once and each backend is a short adapter naming its assembler. Separately, the three roles were handed three different system-variable tables, so a name meant one thing in a layout and was reserved in an effect; there is one table now, and x/y/z are ordinary loop counters everywhere. Performance: no hot-path change. All four boards emit byte-identical exec blocks to the previous three-file version, which is the evidence the collapse preserves behavior. Core - moonlive_lower.h holds the one IR walk, a template over the assembler. What is per-target is encodings, and those already lived behind the assembler: no frame constant, register name or instruction appears in any backend file. 537 lines of triplicated algorithm became 190 shared plus 62 of adapter. - The template deduces the register and label types from the assembler rather than naming a global Reg, so it compiles as core code without a backend in scope. RegId/LabelId, not Reg/Label, because the backend's own names are in scope wherever it is instantiated and reusing them shadows those declarations. Light domain - One lightSysVars() table for all three roles; layoutSysVars/effectSysVars/ modifierSysVars remain as aliases so no call site changed. The split prevented no mistake (a layout reading width got a compile error, which is the same outcome as reading a value that is always zero) and created a trap: the tables were different vocabularies rather than nested ones, so disasm.py compiled against the widest and refused grid.mlv, the shipped default layout. - A modifier's coordinate is xPos/yPos/zPos. x/y/z are what an author reaches for as loop counters, and grid.mlv uses both, so reserving them globally would break the most ordinary code there is. - Host gained movReg/branchGeU/branchNe and adopted the devices' FillElems; its cmp/branchIf are private now, since a flags pair cannot be shared with a backend that has none. Those differences turned out to be free choices, not ISA facts. Tests - The test that specified the per-role split now specifies the single vocabulary, including that reading a coordinate outside a modifier is legal and reads 0. - A new test pins that the three role accessors return the same table, so a future re-split has to say so rather than silently reintroducing the trap. Docs/CI - Plan-20260813 is closed and marked (shipped): every step shipped, dropped or superseded, with reasons. Step 5 (delete the allocator) is dropped because its precondition never came true, and step 7 (factor the bindings onto one base) is superseded: the three bindings derive from sibling bases, so a shared base needs virtual inheritance and would change the layout of every module in the system. - Plan-20260817 takes over as the road to launch: scripts become classes with members, functions and named entry points, then if/else, reading a light back, arrays, wider values and the editing loop. Recursion is a requirement rather than a follow-up, per the predecessor's own table. - MIGRATING is exempt for MoonLive until it launches: nobody runs scripts on a device yet, so an entry would describe an upgrade path no user can take. The two pre-launch entries are removed and the policy is stated. - CLAUDE.md: running a gate script to check work in progress is still starting a gate list, which is the product owner's to fire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Owner
|
@coderabbitai, can you review 22ae7f5 and report your findings in copy pastable content for agents |
A script declares a class and the host calls its functions, the way a compiled
module does: an effect writes tick(), a layout placeLights(), a modifier
modifyLogical(). The old form was a bare list of statements with controls declared
by a COMMENT, which is not C and did not resemble the thing it stands in for.
Performance: no hot-path change. Measured on the S3 at 8x8, before and after:
plasma 274 -> 231us, ripples 505 -> 466us, lines 125 -> 96us, and every exec block
is byte-identical, so the syntax is front-end only.
Core
- The grammar's top level is `class Name { ... }`, the only form. Optional was
considered and rejected: it would keep a second parse path, a second set of rules
to document and a second thing to test, permanently, so a handful of two-line
scripts could stay two lines shorter.
- Each function gets its OWN frame. The lowering emitted one prologue per program,
so an entry point's address pointed past the frame setup and calling it ran a
routine whose frame was never established. The host arguments moved with it: they
were parked at IR index 0, before any prologue, writing into a frame that did not
exist yet.
- A symbol table over the emitted block: the parser records the IR index a function
starts at, the shared lowering converts it to a byte offset while emitting, and
MoonLive::entry(name) turns a name into a callable address. One allocation holds
every function, so a script may define as many as it likes.
- The class name is carried for diagnostics, independent of the file name, the way
a C translation unit and the functions inside it are.
Light domain
- A NAME IS A MOMENT, NOT A ROLE. The host owns moments and runs whatever the
script defined for each: tick when a frame renders, placeLights when lights are
placed, modifyLogical when a coordinate is folded. Nothing validates which names
a class defines, so one class can serve several moments and an effect that also
folds coordinates needs no feature added for it. A per-role name with a tick()
fallback came first and was dropped: nothing needed the fallback, and it left two
ways to write a modifier.
- forEachCoord is renamed placeLights, in the compiled layouts as well as the
scripted ones. It never called back per coordinate: it runs once and emits all of
them, which is a producer, not an iteration. Checked against MoonLight before
renaming, where the equivalent is onLayout and the genuine per-item callbacks
(forEachLightIndex, our forEachDestination) are correctly named and untouched.
Tests
- The symbol table is pinned by a class with TWO functions, where offset 0 would be
wrong and calling the wrong one is silent because both compile. Verified to fail
when the map is stubbed back to zero.
- The structural checker re-reads the frame at every prologue instead of judging a
block by its first: with several routines per block, every function after the
first went unchecked, on the target where the frame contract is fatal. Verified
by shrinking the Xtensa reserve to 16, which makes it fire.
- Entry-point dispatch is pinned by two functions writing different pixels, and by
a class defining both tick and modifyLogical, each called at its own moment.
- mmScript() wraps a bare body in the class ceremony so a test still reads as the
one behavior it is about. It hands out a ring of buffers rather than one: a table
of scripts held every row aliased to the last, which would have tested the same
script N times instead of failing. Oversized input is refused rather than
truncated, after a 54 KB runaway-script case was silently clipped to 4 KB and
stopped being a runaway.
Docs
- The language reference, the controls example, the system-variable table and the
layout and modifier examples describe the class form.
- Plan-20260817 records the corrected step order: per-function frames come BEFORE
wiring the bindings, which the code taught us by segfaulting when they did not.
Step 5 gains its concrete answer, a held MoonLiveScript member rather than a
shared base, with the sibling-base blocker measured rather than assumed.
- Backlog: duplicate module names are reachable and silent (found on the bench, a
layout and an effect both named MoonLive, so the UI rendered one card's controls
under the other's heading), and deleting a module by name removes the first match.
Verified on all four boards (S3, classic ESP32, P4, S31): every binding compiles
and runs, exec blocks unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The MoonLive IR walk existed three times, once per backend, and the two device
copies differed by two identifier tokens. It is written once now, with each
backend reduced to a short adapter naming its assembler. Separately, the three
script roles were handed three different system-variable tables, so a name meant
one thing in a layout and was reserved in an effect. There is one table now.
Both are subtraction: the code shrank, and the only reason this PR is net-positive
in lines is the two plan documents.
One lowering, every backend
src/core/moonlive/moonlive_lower.hholds the IR walk as a template over theassembler. 537 lines of triplicated algorithm became 190 shared plus 62 of
adapter.
What is genuinely per-target turned out to be only the encodings, and those
already lived behind the assembler: no frame constant, register name or
instruction appears in any of the three backend files. The host's apparent
differences (a
Movthat added zero, a two-instruction branch, aFillElemsusing a third scratch register) were free choices rather than ISA facts, so it
gained
movReg/branchGeU/branchNeand adopted the devices'FillElems. Itscmp/branchIfare private now, because a flags pair cannot be shared with abackend that has none.
The evidence it preserves behavior: all four boards emit byte-identical exec
blocks to the three-file version. Rose 1880 B, ripples 2372 B, plasma 1124 B,
unchanged on S3, classic ESP32, P4 and S31.
One vocabulary, every script
lightSysVars()serves all three roles;layoutSysVars/effectSysVars/modifierSysVarsremain as aliases, so no call site changed.The split it replaces prevented no mistake, since a layout reading
widthgot acompile error, which is the same outcome as reading a value that is always zero.
What it did create was a trap: the tables were different vocabularies rather than
nested ones, so
disasm.pycompiled against the widest and refusedgrid.mlv,the shipped default layout, as "name is a system variable". A modifier's
coordinate is
xPos/yPos/zPosnow, which freesx/y/zas the loopcounters an author actually reaches for.
Plans
Plan-20260813 is closed and marked
(shipped). Every step is resolved:stack machine makes spilling unreachable. It does not: all three lowerings still
call it, and it carries the only coverage the register algorithm has.
MoonModule, so a shared base needs virtual inheritance and would change thelayout of every module in the system to serve three of them. The direction
objected too: once a script defines named entry points, the bindings stop being
three kinds and become one kind with different entry points present, which is a
dispatch question rather than an inheritance one.
Plan-20260817 takes over as the road to launch: scripts become classes with
members, functions and named entry points, then
if/else, reading a light back,arrays, wider-than-a-byte values, and the editing loop. Recursion is a requirement
rather than a follow-up, per the predecessor plan's own table ("each activation
gets its own frame").
Also
device yet, so an entry documents an upgrade path no user can take. The two
pre-launch entries are removed and the policy is written down.
a gate list, which is the product owner's to fire.
Verification
All ten applicable pre-commit gates green. All three ISAs build clean. The renamed
modifier was confirmed compiling and folding on both device families: Xtensa (S3,
188 B) and RISC-V (P4, 216 B).
🤖 Generated with Claude Code