refactor: lower control flow to cf dialect instead of scf#20
refactor: lower control flow to cf dialect instead of scf#20tetsuo-cpp merged 1 commit intocanonfrom
Conversation
tetsuo-cpp
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-motivated refactoring. The move from SCF to CF is the right call — interleaved control flow patterns like multi-WHILE and WHILE+UNTIL genuinely can't be expressed with structured regions. The net result is a big simplification: ~340 lines of complex region manipulation code (convertRegionTypes, inlineRegionBefore, mergeBlocks) replaced by trivial PopFlagOp/PopOp/PushValueOp conversions, the isWhileLoop() lookahead hack eliminated, and one fewer pipeline stage.
Test coverage is solid — new interleaved CF tests at translation, conversion, and pipeline levels, plus GPU e2e tests.
Suggestions
cfStack lacks validation on pop: The pop_back_val() calls for ELSE, THEN, UNTIL, WHILE, REPEAT don't check the tag or guard against empty stack. A user writing IF REPEAT or THEN without IF would get a crash or garbage IR rather than a diagnostic. Only LOOP checks loopStack.empty(). Consider adding:
- Empty checks before popping (like
LOOPalready does) - Tag assertions, e.g.
assert(tag == CFTag::Dest)in REPEAT
DO/LOOP counter as alloca vs SSA: The memref<1xi64> alloca for the loop counter means every I/J/K access requires a memref.load. LLVM's mem2reg should optimize these, but worth keeping an eye on PTX output quality vs the old scf.for induction variable approach.
Minor: redundant arith.constant 0 : index emissions in check/body blocks for DO/LOOP — canonicalizer handles it, but could be hoisted.
Overall LGTM. The main action item would be defensive validation on cfStack pops.
Summary
Related: #16