Skip to content

Comments

feat: replace PARAM with \! kernel header syntax#31

Merged
tetsuo-cpp merged 2 commits intocanonfrom
feat/kernel-header-syntax
Feb 19, 2026
Merged

feat: replace PARAM with \! kernel header syntax#31
tetsuo-cpp merged 2 commits intocanonfrom
feat/kernel-header-syntax

Conversation

@tetsuo-cpp
Copy link
Owner

@tetsuo-cpp tetsuo-cpp commented Feb 19, 2026

Summary

  • Replace PARAM <name> <size> with structured \! header block (\! kernel, \! param, \! shared directives)
  • Support scalar params (i64) in addition to array params (i64[N])
  • Detect kernels via forth.kernel attribute instead of hardcoded name matching
  • Make --kernel flag required in warpforth-runner

Test plan

  • All 61 lit tests pass (check-warpforth)
  • C++ formatting clean (cmake --build build --target format)
  • Python lint/format clean (ruff check + ruff format --check)
  • GPU end-to-end tests (pytest -v -m gpu)

Addresses #30

Copy link
Owner Author

@tetsuo-cpp tetsuo-cpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 i64 produces an i64 function argument instead of memref<Nxi64>
  • Kernel detection via attributeforth.kernel unit attr replaces hardcoded "main" name matching
  • --kernel required 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 headerEndPtr to hand off to the lexer for code
  • Good error diagnostics with emitErrorAt using SMLoc for precise source locations
  • Correct handling of Forth comments (\ ...) vs directives (\!) in the header region — the if/else if chain checks \\! before \\
  • ParamRefOpConversion correctly branches on MemRefType vs i64 for scalar params

Issues and suggestions:

  1. sawKernel/sawDirective can be replaced by kernelName.empty()kernelName starts empty and is set when \! kernel is parsed, so kernelName.empty() is equivalent to !sawKernel. And sawDirective is always equal to sawKernel at the point it's checked (line 275) — the PARAM/SHARED branch can only set sawDirective after sawKernel is already true. Removing both booleans and using kernelName.empty() simplifies the logic.

  2. SharedDecl is dead code — The struct is defined, sharedDecls is 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.

  3. std::stoll can throw — In parseHeader() at the array size parsing, after the digit-only validation loop this is unlikely to fail, but a 20+ digit number would throw std::out_of_range. Could use llvm::StringRef::getAsInteger() which returns an error code instead.

  4. Duplicated header parsing in Pythonconftest.py has _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.

  5. Stale error message in conftest.py_parse_param_declarations still says "Forth source has no 'param' declarations". Should reference the new \! param syntax.

  6. 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/trim lambdas in parseHeader() could use llvm::StringRef::ltrim()/rtrim() directly.
  • splitWS similarly could use llvm::StringRef::split() with tokenization helpers.

Risks

  • Breaking change for any existing .forth files outside the repo — old PARAM syntax will error. Intentional per the PR description.
  • The --kernel flag being required in warpforth-runner is a minor UX change but more explicit is arguably better.

Test Coverage

  • All existing lit tests updated to new syntax
  • New scalar-param.forth test 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/sawDirectivekernelName.empty(), remove unused SharedDecl or mark as TODO, and add header error tests.

@tetsuo-cpp tetsuo-cpp merged commit 0ef5853 into canon Feb 19, 2026
1 check passed
@tetsuo-cpp tetsuo-cpp deleted the feat/kernel-header-syntax branch February 19, 2026 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant