feat: replace PARAM with \! kernel header syntax#31
Conversation
tetsuo-cpp
left a comment
There was a problem hiding this comment.
Code Review
Overview
This PR introduces a structured header block syntax (\! kernel, \! param, \! shared) to replace the previous PARAM <name> <size> declarations. Key changes:
- New header parser in
ForthToMLIR.cpp— a line-based pre-pass that processes\!directives before the Forth lexer runs - Scalar param support —
\! param SCALE i64produces ani64function argument instead ofmemref<Nxi64> - Kernel detection via attribute —
forth.kernelunit attr replaces hardcoded"main"name matching --kernelrequired in runner — explicit kernel name for PTX function lookup- All 30+ test files updated to new syntax
Code Quality & Correctness
Well done:
- Clean separation: header parsing is line-based and independent of the Forth lexer, using
headerEndPtrto hand off to the lexer for code - Good error diagnostics with
emitErrorAtusingSMLocfor precise source locations - Correct handling of Forth comments (
\ ...) vs directives (\!) in the header region — theif/else ifchain checks\\!before\\ ParamRefOpConversioncorrectly branches onMemRefTypevsi64for scalar params
Issues and suggestions:
-
sawKernel/sawDirectivecan be replaced bykernelName.empty()—kernelNamestarts empty and is set when\! kernelis parsed, sokernelName.empty()is equivalent to!sawKernel. AndsawDirectiveis always equal tosawKernelat the point it's checked (line 275) — the PARAM/SHARED branch can only setsawDirectiveaftersawKernelis already true. Removing both booleans and usingkernelName.empty()simplifies the logic. -
SharedDeclis dead code — The struct is defined,sharedDeclsis populated during parsing, but nothing consumes it. Consider either removing it from this PR or adding a comment/TODO that it's scaffolding for a future feature. -
std::stollcan throw — InparseHeader()at the array size parsing, after the digit-only validation loop this is unlikely to fail, but a 20+ digit number would throwstd::out_of_range. Could usellvm::StringRef::getAsInteger()which returns an error code instead. -
Duplicated header parsing in Python —
conftest.pyhas_parse_kernel_name()and_parse_param_declarations()with near-identical line iteration and\!/--stripping logic. A small helper like_iter_header_directives(forth_source)would reduce duplication. -
Stale error message in conftest.py —
_parse_param_declarationsstill says"Forth source has no 'param' declarations". Should reference the new\! paramsyntax. -
No negative tests for header validation — The parser validates many error conditions (missing
\! kernel, duplicate kernel, directive after code, unknown directive, duplicate param names) but there are no lit tests exercising these paths. Consider adding at least a couple, e.g.header-missing-kernel-error.forth,header-duplicate-param-error.forth.
Minor Nits
- The
ltrim/rtrim/trimlambdas inparseHeader()could usellvm::StringRef::ltrim()/rtrim()directly. splitWSsimilarly could usellvm::StringRef::split()with tokenization helpers.
Risks
- Breaking change for any existing
.forthfiles outside the repo — oldPARAMsyntax will error. Intentional per the PR description. - The
--kernelflag being required inwarpforth-runneris a minor UX change but more explicit is arguably better.
Test Coverage
- All existing lit tests updated to new syntax
- New
scalar-param.forthtest for scalar param support - GPU end-to-end tests updated
- Missing: negative tests for header error paths (as noted above)
Verdict
Clean, well-structured PR. The core parser logic is correct and the attribute-based kernel detection is a nice improvement over name matching. Main suggestions: simplify sawKernel/sawDirective → kernelName.empty(), remove unused SharedDecl or mark as TODO, and add header error tests.
Summary
PARAM <name> <size>with structured\!header block (\! kernel,\! param,\! shareddirectives)i64) in addition to array params (i64[N])forth.kernelattribute instead of hardcoded name matching--kernelflag required in warpforth-runnerTest plan
check-warpforth)cmake --build build --target format)ruff check+ruff format --check)pytest -v -m gpu)Addresses #30