feat: add f64 floating-point support and rename ops to MLIR conventions#38
feat: add f64 floating-point support and rename ops to MLIR conventions#38tetsuo-cpp merged 4 commits intocanonfrom
Conversation
e3d2dc1 to
c0e16d8
Compare
f63b390 to
908ce39
Compare
908ce39 to
36b2c0e
Compare
tetsuo-cpp
left a comment
There was a problem hiding this comment.
Code Review: feat: add f64 floating-point support and rename ops to MLIR conventions
+1228 / -454 across dialect definition, parser, conversion patterns, runner, and tests.
Overview
This PR implements f64 floating-point support for WarpForth using a bitcasting approach: floats are stored as i64 bit patterns on the untyped stack, and F-prefixed operations bitcast before/after arithmetic. Simultaneously, it renames all ops to follow MLIR conventions (literal → constant, add → addi/addf, load → loadi/loadf, etc.).
The scope covers:
- Lexer/Parser: Float literal detection,
Token::Kind::Float,BaseTypeenum for params/shared - Dialect (TableGen): New float arithmetic, comparison, memory, and conversion ops; renamed integer ops
- ForthToMemRef conversion: Templatized
BinaryArithOpConversion,BinaryCmpOpConversion,MemoryLoadOpConversion,MemoryStoreOpConversionwithIsFloat/AddressSpaceparameters - warpforth-runner:
std::variant-based param types for i64/f64 scalars and arrays - Tests: Translation, conversion, pipeline, and GPU end-to-end tests
Strengths
- Clean architecture: The bitcasting approach is the right call for an untyped stack — avoids fundamental changes to the stack type while cleanly supporting floats.
- Excellent template refactoring:
MemoryLoadOpConversion<ForthOp, IsFloat, AddressSpace>andMemoryStoreOpConversioneliminate significant duplication from the previous 4 separate load/store conversion structs. - Thorough test coverage: Tests at every level — translation (6 new
.forthfiles), conversion (2 new.mlirfiles), pipeline (1 new.forth), and GPU end-to-end (8 new test functions). All existing tests updated for the rename. - MLIR naming conventions: The type-suffixed mnemonics (
addi/addf,eqi/eqf,loadi/loadf) and CamelCase class names align well with upstream MLIR style. - Runner precision:
std::setprecision(17)for f64 output is correct for full IEEE 754 double roundtrip fidelity.
Issues & Suggestions
1. ConstantOp value attribute is too permissive
ForthOps.td:128 — The attribute type changed from I64Attr to AnyAttr:
let arguments = (ins Forth_StackType:$input_stack, AnyAttr:$value);
This would accept any attribute (string, array, dictionary, etc.). Consider narrowing to:
AnyAttrOf<[I64Attr, F64Attr]>
or at minimum TypedAttr. This provides compile-time validation and better error messages if someone misuses the op.
2. Float literal parsing: potential edge cases
ForthToMLIR.cpp:133-147 — The isFloat function relies on strtod after checking for ./e/E. A few edge cases to consider:
"."alone —strtodwould fail (end != start + size), so this is fine."1."—strtodparses this as1.0, which is correct Forth behavior."1e"—strtodparses as1butendwould point toe, soend != start + size→ correctly rejected.
These all seem handled correctly. Just flagging that I checked.
3. Float comparison NaN semantics are undocumented
The float comparisons use ordered predicates (OEQ, OLT, OGT, ONE, OLE, OGE), meaning any comparison involving NaN returns false (except ONE which returns true for NaN). This is standard IEEE 754 behavior but worth a note in CLAUDE.md or the op descriptions, since Forth users might not expect this.
4. test/Conversion/ForthToMemRef/float-arithmetic.mlir — semantic oddity
%3 = forth.addf %2 : !forth.stack -> !forth.stack
%4 = forth.subf %3 : !forth.stack -> !forth.stack // only 1 value on stack!
%5 = forth.mulf %4 : !forth.stack -> !forth.stack
%6 = forth.divf %5 : !forth.stack -> !forth.stackAfter addf consumes 2 values and pushes 1, subf would underflow at runtime. This is fine as a FileCheck pattern test (only verifies IR structure), but it could confuse future readers. A comment noting this only tests IR lowering patterns would help.
5. warpforth-runner: no error handling on stoll/stod in parseParam
warpforth-runner.cpp:159-160 — The lambdas toI64 and toF64 call std::stoll / std::stod without try/catch. If the user passes malformed values (e.g. i64:abc), this throws std::invalid_argument with no helpful message. Consider wrapping with error reporting.
Minor Nits
- The PR bundles two logically separate changes (f64 support + op rename). If this were more complex, I'd suggest splitting, but at this scale it's manageable in one PR since the rename is largely mechanical.
Verdict
This is a well-executed PR. The architecture is sound, the template refactoring reduces duplication, and test coverage is thorough. The main actionable item is tightening the ConstantOp attribute type from AnyAttr to AnyAttrOf<[I64Attr, F64Attr]>. The rest are minor polish items.
Summary
F+ F- F* F/), comparison (F= F< F> F<> F<= F>=), memory (F@ F! SF@ SF!), conversion (S>F F>S), and f64 kernel paramsaddi/addf,loadi/loadf,eqi/eqf, etc.) and CamelCase class names (AddIOp,AddFOp,LoadIOp,LoadFOp, etc.).literal→constant.f64/f64[]params viastd::variantTest plan
cmake --build build --target check-warpforthpasses all new and existing testsCloses #6