feat: support scalar kernel params in runner and test infra#33
feat: support scalar kernel params in runner and test infra#33tetsuo-cpp merged 6 commits intocanonfrom
Conversation
tetsuo-cpp
left a comment
There was a problem hiding this comment.
Code Review
Clean PR — scalar param support is well-structured across both the C++ runner and Python test infra. The ParamDecl dataclass is a nice improvement over the raw tuple. A few items to consider:
Actionable
-
Empty value handling in
parseParam(warpforth-runner.cpp): Passing--param i64:or--param i64[]:will hitstd::stollon an empty string and throw an uncaught exception. Worth validating thatvalueStris non-empty (and that at least one value was parsed for arrays). -
Pointer-lifetime comment (
warpforth-runner.cpp:~214):scalarValuesexists sokernelArgs[i]can point at&scalarValues[i]across the kernel launch. This is correct but non-obvious — a brief comment would help future readers. -
Out-of-bounds
output_param(conftest.py:387): The guardoutput_param < len(decls)silently skips the scalar check when the index is out of bounds, deferring the error to a less obvious place. Consider failing early on out-of-bounds too.
Optional / Nits
- C-style casts in the kernel arg ternary — consistent with the rest of the file but
static_castwould be more idiomatic C++17. - No negative tests for the runner's error paths (bad type prefix, scalar with multiple values, scalar as output). A quick lit test checking for the expected stderr would be nice coverage.
- The
MIDFileCheck line is sensitive to MLIR attribute ordering — could become flaky if attribute print order changes upstream.
Overall looks good 👍
Summary
--paramformat towarpforth-runner:i64:42(scalar) andi64[]:1,2,3(array)conftest.pyto parse scalar param declarations and pass them with the typed formatscalar-param.forth) and GPU test (test_scalar_param) for mixed scalar+array kernelsCloses #32