Conversation
tetsuo-cpp
left a comment
There was a problem hiding this comment.
Code Review
Clean implementation — reusing forth.pop/forth.push_value as pure SSA values is the right design. Locals map to registers on GPU for free, and entry-block SSA dominance gives you control flow support without any extra work.
Suggestions
Missing error-path tests: The code checks for param and shared memory name conflicts, but neither path has a test. Consider adding:
\ param conflict
\! kernel main
\! param DATA i64[256]
: BAD { data -- } data ;
BAD\ shared conflict
\! kernel main
\! shared BUF i64[64]
: BAD { buf -- } buf ;
BAD{ outside a word definition: Currently falls through to "unknown word: {" which is a confusing error. A targeted diagnostic like "local variables can only be declared inside word definitions" would be friendlier. Low priority.
Minor: Output names after -- are silently skipped without validation — { a b -- 42 } is accepted. Might be worth a code comment noting this is intentional (documentation-only, per ANS Forth).
Everything else looks good. The reverse-order pop loop, localVars.clear() cleanup, and conflict checks are all correct.
Summary
{ a b c -- }syntax for read-only local variables in word definitionsforth.popandforth.push_valueops (pure SSA approach, no new dialect operations)Test plan
gpu.binarytestCloses #8