Various improvements to enable importing stdlib module - #41
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Code reviewFound 1 new issue — see the inline comment. |
Uh oh!
There was an error while loading. Please reload this page.
Code reviewFound 1 new issue — see the inline comments. |
…ctly `while ... else` never lowered. WhileOpLowering inlined the orelse region without rewriting its normal-completion py.br_yield, so the yield survived with no py.while parent left to satisfy its HasParent trait
Uh oh!
There was an error while loading. Please reload this page.
Code reviewFound 1 new issue — see the inline comment. |
Uh oh!
There was an error while loading. Please reload this page.
Code reviewFound 1 new issue — see the inline comment. |
| if (loop && !loop.isLoopOrelse(region)) { return false; } | ||
| } | ||
| return false; | ||
| } | ||
| // True when a break/continue that binds to the loop whose body is `body` is | ||
| // somewhere replace_loop_branch_yields cannot reach yet — inside a nested | ||
| // region that has not been flattened into ours. Branching it to our target | ||
| // block now would be a cross-region block reference, which is invalid IR. | ||
| // | ||
| // The caller defers (fails the match) until the nested op lowers and inlines | ||
| // the yield into our region, the same innermost-first trick TryOpLowering uses | ||
| // for nested trys. Terminates because the innermost such op has nothing nested | ||
| // to wait on. | ||
| bool has_pending_nested_orelse_control(mlir::Region &body) | ||
| { | ||
| if (body.empty()) { return false; } |
There was a problem hiding this comment.
has_pending_nested_orelse_control only checks the immediately-nested loop's own orelse for a kinded break/continue yield, then WalkResult::skip()s that loop's entire subtree (line 91). So a break/continue written in a second-level orelse — "else nested inside another else", e.g. for a: for b: pass else: for c: pass else: break — is never discovered: the inner loop L2 sits inside L1's orelse and is skipped before it's ever visited as its own loop. The enclosing loop therefore lowers without deferring, and after L1/L2 later flatten, the kinded py.br_yield ends up with no py.for_loop/py.while parent left, violating BranchYieldOp's HasParent trait.
This PR's own integration/tests/loop_else_break_binding.py exercises exactly this shape ("An else nested inside another else", "Three elses deep") and its docstring explicitly claims the deferral "follows the whole chain" — contradicting the comment on line 89-90 here, which is unsound for this shape. (bug)
Code reviewFound 1 new issue — see the inline comment. |
Uh oh!
There was an error while loading. Please reload this page.
| // Rewrites a loop orelse region's normal-completion (kindless) py.br_yield ops | ||
| // into branches to the loop's exit block. | ||
| // | ||
| // Only the kindless ones. A `break`/`continue` written in an orelse binds to | ||
| // the loop *enclosing* this one, so those are left in place: once this region | ||
| // is inlined they sit directly in the enclosing loop's body, where its own | ||
| // replace_loop_branch_yields claims them. | ||
| void replace_orelse_completion_yields(mlir::PatternRewriter &rewriter, | ||
| mlir::Region ®ion, | ||
| mlir::Block *exit_target) | ||
| { | ||
| if (region.empty()) { return; } | ||
| region.walk<WalkOrder::PreOrder>([&rewriter, exit_target](mlir::Operation *operation) { | ||
| if (is_flattened_region_op(operation)) { return WalkResult::skip(); } | ||
| auto yield_op = mlir::dyn_cast<mlir::py::BranchYieldOp>(operation); | ||
| if (!yield_op || yield_op.getKind().has_value()) { return WalkResult::advance(); } | ||
| rewriter.setInsertionPoint(yield_op); | ||
| rewriter.replaceOpWithNewOp<mlir::cf::BranchOp>(yield_op, exit_target); | ||
| return WalkResult::advance(); | ||
| }); |
There was a problem hiding this comment.
break/continue in a nested loop's else that binds to an outer loop now silently skips finally/with.__exit__ cleanup on that control-flow path (bug)
TryOpLowering/WithOpLowering run before ConvertLoopsPass and (per the comment at ~line 409 in this file) assume "a loop consumes its own break/continue," so replace_controlflow_yield and collect_loop_control_kinds skip over any nested ForLoopOp/WhileOp entirely. This PR's new replace_orelse_completion_yields deliberately leaves kinded (break/continue) yields in a nested orelse in place so an outer loop can claim them later — but that means they now escape past the try/with lowering pass without ever being routed through build_finally_loop_exits / LeaveExceptionHandle. For example:
forain [1, 2, 3]:
try:
forbin []:
passelse:
breakfinally:
print("cleanup")binds break to the outer for per Python semantics, but the generated code branches straight out of the loop without ever entering finally, so "cleanup" is never printed. The same gap applies to with (WithOpLowering has the identical skip), where __exit__ is silently never invoked. This is a new failure mode introduced by the orelse-binding mechanism (pre-PR, the break was fully swallowed by the inner loop, so the try's already-wired finally path still ran) — worth teaching TryOpLowering/WithOpLowering about pending outward-binding orelse yields, or deferring them the same way nested loops now defer to each other.
Code reviewFound 1 new issue — see the inline comment. |
No description provided.