Implement parse functions of SCF operations#20
Conversation
Implement ForOp::parse based on upstream scf::ForOp::parse, including ensureTerminator handling for the implicit `jeff.yield`. Add `SingleBlockImplicitTerminator<"jeff::YieldOp">` to all four SCF ops (for, while, doWhile, switch) so the printer's elision of empty yields round-trips correctly through parse. Make the induction-variable type annotation required (`: T`) at parse time rather than defaulting to index, which jeff does not accept, and simplify the matching print branch. Add unittests/IR with string-based parse tests for ForOp covering valid forms, parse-level errors, verify-level errors, and a print/parse round-trip idempotency check. Signed-off-by: rturrado <rturrado@gmail.com>
Implement WhileOp::parse and rewrite WhileOp::print to use a new syntax `[: (types)] args(...) { cond } args(...) { body }`, where the leading function-type annotation is present only when the op has loop-carried values, and the per-region `args(...)` clauses are always emitted (possibly empty) so they serve as the structural separator between the two regions. The condition region's implicit `i1` yield type is no longer
printed.
Verify at parse time that both `args(...)` clauses bind the same operand names, reflecting that a Jeff while has a single op-level operand list shared between the condition and body regions.
Add ODS descriptions for ForOp, WhileOp, and SwitchOp documenting their semantics, syntax, verifier invariants, and differences from the corresponding scf ops.
Add a TODO in the stub SwitchOp::parse flagging that the verifier's `in_values.size() == out_values.size()` constraint should be relaxed when the parser is implemented; the constraint is a carry-over from loop iter-args and is not part of switch semantics.
Add unittests/IR/test_parse_while_op.cpp covering valid forms and invalid forms.
Refactor test_parse_for_op.cpp to a `const std::string src` style for consistency with the new while-op tests.
Signed-off-by: rturrado <rturrado@gmail.com>
Implement DoWhileOp::parse and rewrite DoWhileOp::print to use the same syntax as `jeff.while`, with the body region declared first and the condition region second to reflect the do-while semantic that the body runs unconditionally on the first iteration before the predicate is evaluated. The leading function-type annotation is present only when the op has loop-carried values, and the per-region `args(...)` clauses are always emitted (possibly empty) so they serve as the structural separator between the two regions. The condition region's implicit `i1` yield type is no longer printed. Verify at parse time that both `args(...)` clauses bind the same operand names, reflecting that a Jeff doWhile has a single op-level operand list shared between the body and condition regions. Add an ODS description for DoWhileOp documenting its semantics, syntax, verifier invariants, and the body-first difference from `jeff.while`, cross-referencing `jeff.while` for the differences from `scf.while`. Add unittests/IR/test_parse_do_while_op.cpp covering valid forms, parse-level errors, and a print/parse round-trip idempotency check. Signed-off-by: rturrado <rturrado@gmail.com>
Implement SwitchOp::parse and rewrite SwitchOp::print to use a folded syntax that states the op's operands and their types together as a single function-type, decoupled from the result types:
```
%r1, %r2 = jeff.switch (%sel, %a, %b) : (i32, T_in...) -> (T_out...)
case 0 args(%x, %y) { ... }
case 1 args(%x, %y) { ... }
default args(%x, %y) { ... }
```
Each region's `args(...)` lists only local block-argument names, not assignments — the operands are already named once in the op's header, so the block arguments are bound positionally to `in_values` and their types are inferred. Case labels are explicit but contiguous starting at 0; the parser enforces this so that print-then-parse is faithful.
Decouple `in_values` from `out_values` in the verifier. They were previously forced to match in count and type as a carry-over from loop iter-args, but that constraint is not part of switch semantics. Each region's block arguments now match `in_values` and each region's `jeff.yield` matches the op's result types as two separate checks. Drop `hasVerifier` from the ODS since the op-level `verify` is now trivial.
Update the ODS description for `jeff.switch` to reflect the new syntax, spelling out the implicit inferences (positional binding, inferred arg types).
Add implicit-inference notes to the `jeff.while` and `jeff.doWhile` descriptions. Fix a stale local-name swap in the `jeff.doWhile` example.
Add unittests/IR/test_parse_switch_op.cpp covering valid forms, parse-level errors, unsupported selector types, and verifier-level errors.
Signed-off-by: rturrado <rturrado@gmail.com>
…hile` The operands of a Jeff while/doWhile loop are stated once at the op level. Repeating them in the second region's `args(...)` clause was redundant — and the verifier had to check that both clauses agreed on the operands. Following the design discussion on issue PennyLaneAI#7, the second region now lists only local block-argument names; the operands are inherited positionally from the first region. `jeff.while` now prints as `: (types) args(%c_x = %a, ...) { cond } args(%b_x, ...) { body }`, and `jeff.doWhile` symmetrically as `: (types) args(%b_x = %a, ...) { body } args(%c_x, ...) { cond }`. Drop the "both args clauses must bind the same operands" check from both parsers (and the `llvm::formatv`-based diagnostic helper that supported it), since the constraint can no longer be expressed in the syntax. Drop the `<llvm/Support/FormatVariadic.h>` include that became unused. Update the ODS descriptions for both ops: drop the "RHS of both `args(...)` clauses must reference the same op operands" sentence, and add an "Implicit inferences" bullet documenting that the second region's `args(...)` lists names only. Update tests in test_parse_while_op.cpp and test_parse_do_while_op.cpp to use the new syntax in the valid and round-trip cases, drop the `InvalidCondBodyOperandsDiffer` test (no longer expressible — and that's the point), and rename `InvalidCondBodyArgCountMismatch` to `InvalidBodyCondArgCountMismatch` in the doWhile suite to reflect the body-first region order. Signed-off-by: rturrado <rturrado@gmail.com>
Fixes the two `misc-include-cleaner` warnings flagged by CI lint on PR PennyLaneAI#20 by dropping unused `<cstddef>` and adding `<mlir/Support/LLVM.h>` (so `mlir::dyn_cast` resolves to a directly included header). Also tidies a couple of comments. Signed-off-by: rturrado <rturrado@gmail.com>
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
Hi, @rturrado! Thanks a lot for your contribution! It's really nice to see people's interest in jeff and jeff-mlir! 🙂
You can find some comments below. As this is already looking really good, most should be very easy to resolve. In the tests, I have some questions for @dime10 and/or @burgholzer regarding program validity.
Co-authored-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com>
Co-authored-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com>
I really loved working on this!
Thanks for the thorough review. I'm also very glad that this task is leading to an interesting discussion on Jeff's own specification. I will start by tackling a few of the review comments next week: |
… JeffOps.cpp Signed-off-by: rturrado <rturrado@gmail.com>
Signed-off-by: rturrado <rturrado@gmail.com>
Signed-off-by: rturrado <rturrado@gmail.com>
|
I have just read the title of that issue (Should Just to confirm. |
… step Update ForOP description at JeffOps.td. Enforce check at ForOp::verifyRegions at JeffOps.cpp. Add tests to check the constraint at test_parse_for_op.cpp. Signed-off-by: rturrado <rturrado@gmail.com>
Yes, for now, we will assume that SCF blocks are fully isolated from above, so variables need to be passed in. Thanks a lot! |
Signed-off-by: rturrado <rturrado@gmail.com>
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments! 🙂
I went through your changes and left a couple more comments that should be rather easy to address.
I'll also go ahead and request a review from @burgholzer and @dime10 to get one more opinion (one of you, feel free to ignore). I'm quite optimistic that we can merge this soon! 🙂
Co-authored-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com>
Add jeff.yield to ForOpTest::BasicFormI{32,64} so they meaningfully differ from ForOpTest::ImplicitYield (previously both shapes looked identical).
Fix ForOpTest::Nested to make %lo, %hi, and %s visible to the inner jeff.for by passing them through the outer loop's args (Jeff regions are isolated from above).
Signed-off-by: rturrado <rturrado@gmail.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks for your work on this @rturrado 🙏🏼
I did not check every single line in detail here, but had a brief look through everything and this LGTM 👍🏼
Given how @denialhaag already went through everything, I am pretty confident that this should be ready to go in.
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
@rturrado, thanks again for all the effort! 🙂
|
Thanks to you, guys! A pleasure! |
4 participants
Summary
parsefor the four Jeff SCF ops (jeff.for,jeff.while,jeff.doWhile,jeff.switch) and rewrites their printers to match, completing issue Implementparsefunctions of SCF operations #7.jeff.switch, adopts a folded function-type header (selector and in-values grouped into one operand list and type signature) that decouplesin_valuestypes from result types, and drops the carry-over iter-args-style verifier check (in_values.size() == out_values.size()).jeff.whileandjeff.doWhile, the second region'sargs(...)lists only local block-argument names — the operands are stated once in the first region and inherited positionally.Closes #7.
Notes for reviewers
I have taken the liberty of adding op descriptions in
JeffOps.td, on the basis that the equivalent docs for MLIR's SCF operations are genuinely useful in practice. Feel free to add, remove, or modify anything in there.There's likewise still time to modify anything regarding the syntax of Jeff's SCF operations — the folded function-type form for
jeff.switchand the second region'sargs(...)without assignments forjeff.while/jeff.doWhileare both worth a second look, since they're new design choices, not adaptations of an existing pattern.