From 4c158e81172d5b5733663dad1bf2694a274f110a Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Tue, 14 Jul 2026 19:37:08 +0100 Subject: [PATCH] Fix lowering pass bugs in LowerToAffineLoops and LowerToLLVM - Implemented visitBreakContinueInScope to correctly handle unlabeled break/continue in nested loops. - Replaced assert with notifyMatchFailure in SymbolCallInternalOpLowering for unresolved callee symbols. - Adjusted stack allocation in NewOpLowering to use the correct storage type. - Added test for triple nested unlabeled break/continue behavior. --- docs/LowerToAffineLoops-LowerToLLVM-review.md | 112 +++++++---- tslang/lib/TypeScript/LowerToAffineLoops.cpp | 184 +++++++++++------- tslang/lib/TypeScript/LowerToLLVM.cpp | 9 +- tslang/test/tester/tests/00break_continue.ts | 24 +++ 4 files changed, 216 insertions(+), 113 deletions(-) diff --git a/docs/LowerToAffineLoops-LowerToLLVM-review.md b/docs/LowerToAffineLoops-LowerToLLVM-review.md index a4324b8de..7542fd4e5 100644 --- a/docs/LowerToAffineLoops-LowerToLLVM-review.md +++ b/docs/LowerToAffineLoops-LowerToLLVM-review.md @@ -108,15 +108,80 @@ JIT host (see method note), so verified via `--emit=mlir-affine caught value correctly). - Full ctest suite (339 JIT + 342 AOT) green with this change in place. -## Remaining known issues (not yet fixed, tracked here for follow-up) +## Second pass (2026-07-14) — items 4, 6, 7, 9, 10 fixed + +Fixed on branch `fix/lowering-passes-bugs-2`, verified via full ctest suite +(688/688 passed, JIT + AOT) plus two research-agent-verified deep dives (items +4 and 7) that confirmed the failure mechanism with file:line evidence before +the fix was written. 4. **`LowerToAffineLoops.cpp:494,567,635,717`** (While/DoWhile/For/Label - lowering) — the `visitorBreakContinue` walk recurses into nested - loop/label regions, not just the immediate body. An unlabeled - `break`/`continue` inside a nested loop can have its `tsContext->jumps[op]` - entry set by either the inner or outer loop's walk, with the last one to - run winning — order-dependent, so a nested unlabeled `break` could jump out - of both loops instead of just the inner one. PLAUSIBLE, not yet fixed. + lowering) — FIXED. Confirmed via agent investigation: the old + `visitorBreakContinue`/`walk()` recursed into nested loop/label/switch + regions, and an unlabeled `break`/`continue` matched *any* enclosing + scope's walk (`MLIRHelper::matchLabelOrNotSet` returns `true` whenever the + op has no label of its own). Correctness depended entirely on undocumented + MLIR dialect-conversion worklist ordering (last write to + `tsContext->jumps[op]` wins, and the innermost loop's pattern happened to + run last) — not a real guarantee, and confirmed to have zero explicit + ordering protection in the code. Replaced with a shared + `visitBreakContinueInScope` helper (manual recursive region walk, added + just above `WhileOpLowering`) that threads independent + `eligibleForUnlabeledBreak`/`eligibleForUnlabeledContinue` flags through + the descent: both flags clear on crossing a nested While/DoWhile/For/Label + boundary, and the break-only flag additionally clears on crossing a nested + `SwitchOp` (switch owns unlabeled `break` but not `continue`, matching JS + scoping). Labeled break/continue matching the scope's own label is still + found no matter how many boundaries are crossed. Added + `test_triple_nested_unlabeled` to `00break_continue.ts` (3 levels of + nested `for`, innermost unlabeled `break`/`continue`), JIT-executed and + checked by value. + +6. **`LowerToLLVM.cpp:1271`** (`SymbolCallInternalOpLowering`) — FIXED. + Replaced the debug-only `assert(llvmFuncType)` with + `return rewriter.notifyMatchFailure(...)` when the callee symbol doesn't + resolve to a known function-like op, matching the idiomatic `return + failure()` pattern used elsewhere in this file. + +7. **`LowerToLLVM.cpp:2043`** (`NewOpLowering`, stack-allocation path) — + FIXED. Confirmed via agent investigation: `resultType = tch.convertType(newOp.getType())` + converts a class type to an opaque LLVM pointer + (`LowerToLLVM.cpp:5967-5969`, `ClassType → LLVM::LLVMPointerType`), so the + old `ch.Alloca(resultType, 1)` allocated 8 bytes (`sizeof(ptr)`) on the + stack regardless of the class's actual field layout — a stack + under-allocation/overflow for any stack-`new`'d class with fields. Fixed + by sizing off `tch.convertType(storageType)` (the actual converted + `ClassStorageType`/struct), mirroring both the sibling heap-allocation path + a few lines below and the `AllocaOpLowering` pattern earlier in the file. + The now-unused `resultType` local was removed. No test added: the one + test targeting this path (`00class_stack.ts`) has the stack-alloc call + form commented out with a pre-existing, unrelated `TODO: ERROR can't + create class with vtable on stack` blocker. + +9. **`LowerToAffineLoops.cpp:1666`** (`TryOpLowering`, `linuxHasCleanups`) — + FIXED. A cleanup-only try (no catch, no finally — e.g. the `TryOp` + synthesized for a `using` declaration) hit the `linuxHasCleanups` block + with neither `catchHasOps` nor `finallyHasOps`, so `cleanupBlockLast`'s + `ResultOp` terminator was erased with nothing replacing it — malformed IR. + Fixed by adding the missing branch: build a Linux cleanup landing pad + (`LandingPadOp` cleanup=true + `BeginCleanupOp`) at the start of + `cleanupBlock` and replace the terminator with `EndCleanupOp` (targeting + `parentTryOpLandingPad` if set), mirroring the existing Windows + unconditional cleanup-landing-pad setup and the Linux + `finallyHasOps`-with-no-catch precedent already in this function. + `EndCleanupOp` always lowers to `LLVM::ResumeOp` (confirmed by reading + `EndCleanupOpLowering`), so this only affects the exception-unwind path, + consistent with Itanium cleanup-landingpad semantics. Exercised indirectly + by the existing `00disposable.ts`/`01disposable.ts`/`02disposable.ts` + JIT+AOT tests (a bare `using` block with no explicit try/catch/finally is + exactly this code path), all passing. + +10. **`LowerToAffineLoops.cpp:763`** (`LabelOpLowering`) and the same pattern + in `SwitchOpLowering` (~854) — FIXED. Both `assert(false)` fallbacks + (silent no-op in release builds, leaving a malformed CFG) replaced with + `return rewriter.notifyMatchFailure(...)`. + +## Remaining known issues (not yet fixed, tracked here for follow-up) 5. **`LowerToAffineLoops.cpp:1351,1358,1370`** (`TryOpLowering`, `finally` region cloning) — the `finally` region is cloned twice (normal-exit and @@ -126,36 +191,15 @@ JIT host (see method note), so verified via `--emit=mlir-affine cloned copies. PLAUSIBLE, not yet fixed — needs a test case (`grep` of `test/` found no try/finally-nesting coverage). -6. **`LowerToLLVM.cpp:1271`** (`SymbolCallInternalOpLowering`) — `llvmFuncType` - can remain null if `moduleOp.lookupSymbol` fails to resolve or the symbol is - an unexpected kind; only guarded by a debug-only `assert`, no `return - failure()` guard. PLAUSIBLE, not yet fixed. - -7. **`LowerToLLVM.cpp:2043`** (`NewOpLowering`, stack-allocation path) — - allocates using `resultType` (converted pointer type) instead of - `storageType` (actual class layout) used by the sibling heap-allocation - path; asymmetry could under-allocate the stack slot for `new` with - `getStackAlloc()` set. PLAUSIBLE, needs confirming what `resultType` - actually converts to for class types before treating as certain. - 8. **`LowerToLLVM.cpp:4159`** (`LandingPadOpLowering`, Windows path) — cleanup-only branch reuses the typed-catch filter value instead of an empty/undef cleanup clause; the code has its own `// BUG: in LLVM landing - pad is not fully implemented` comment. PLAUSIBLE, not yet fixed. - -9. **`LowerToAffineLoops.cpp:1660`** (`TryOpLowering`, `linuxHasCleanups`) — - merges `catchesBlock`/`finallyBlock` into `cleanupBlockLast` only when - `catchHasOps`/`finallyHasOps`, but `linuxHasCleanups` doesn't imply either; - a cleanup-only try with no catch/finally content could erase - `cleanupBlockLast`'s terminator with nothing replacing it. PLAUSIBLE, not - yet fixed. - -10. **`LowerToAffineLoops.cpp:763`** (`LabelOpLowering`) — falls back to - `assert(false)` if the label region's terminator block isn't a `MergeOp`; - in release builds (asserts stripped) this is a silent no-op leaving a - malformed CFG if the label region's live last block doesn't end in - `MergeOp` (e.g. dead code after a break-only path). PLAUSIBLE, not yet - fixed. Same pattern exists in `SwitchOpLowering` at line ~854. + pad is not fully implemented` comment. PLAUSIBLE, not yet fixed — + deliberately left alone in the second pass: this is deep Windows SEH + landing-pad construction with the original author's own admission it's + incomplete, and a wrong guess here risks a worse regression than the + status quo. Needs someone with direct SEH/landingpad-clause expertise, or + a way to single-step actual Windows exception dispatch through it. ## Other observations (not correctness bugs, lower priority) diff --git a/tslang/lib/TypeScript/LowerToAffineLoops.cpp b/tslang/lib/TypeScript/LowerToAffineLoops.cpp index bf912fbdc..9c1011144 100644 --- a/tslang/lib/TypeScript/LowerToAffineLoops.cpp +++ b/tslang/lib/TypeScript/LowerToAffineLoops.cpp @@ -469,6 +469,63 @@ struct ResultOpLowering : public TsPattern } }; +// Populates tsContext->jumps for break/continue ops that belong to this loop/label scope. +// An unlabeled break/continue binds to the *nearest* enclosing loop/label (continue) or +// loop/label/switch (break), so once the walk descends past such a boundary it must stop +// treating unlabeled break/continue as claimable by this (outer) scope -- otherwise it can +// steal a jump target that belongs to the nested scope, depending on which scope's pattern +// happens to run last in the dialect-conversion worklist. A nested Switch is a boundary for +// unlabeled break only (switch doesn't own continue -- that still passes through to the +// enclosing loop). Labeled break/continue matching this scope's own label must still be found +// no matter how many boundaries are crossed, since a label may target an outer loop from deep +// inside nested loops/switches (e.g. `outer: while(...) { while(...) { break outer; } }`). +static void visitBreakContinueInScope(mlir::Region ®ion, mlir::StringAttr scopeLabel, + bool eligibleForUnlabeledBreak, bool eligibleForUnlabeledContinue, + llvm::function_ref onMatch) +{ + for (auto &block : region) + { + for (auto &op : block) + { + if (auto breakOp = dyn_cast(&op)) + { + auto opLabel = breakOp.getLabelAttr(); + auto hasOwnLabel = opLabel && opLabel.getValue().size() > 0; + if ((eligibleForUnlabeledBreak && !hasOwnLabel) || + (hasOwnLabel && MLIRHelper::matchLabelOrNotSet(scopeLabel, opLabel))) + { + onMatch(&op, /*isBreak*/ true); + } + + continue; + } + + if (auto continueOp = dyn_cast(&op)) + { + auto opLabel = continueOp.getLabelAttr(); + auto hasOwnLabel = opLabel && opLabel.getValue().size() > 0; + if ((eligibleForUnlabeledContinue && !hasOwnLabel) || + (hasOwnLabel && MLIRHelper::matchLabelOrNotSet(scopeLabel, opLabel))) + { + onMatch(&op, /*isBreak*/ false); + } + + continue; + } + + auto isLoopOrLabelBoundary = isa(op); + auto isSwitchBoundary = isa(op); + + for (auto &nested : op.getRegions()) + { + visitBreakContinueInScope(nested, scopeLabel, + eligibleForUnlabeledBreak && !isLoopOrLabelBoundary && !isSwitchBoundary, + eligibleForUnlabeledContinue && !isLoopOrLabelBoundary, onMatch); + } + } + } +} + struct WhileOpLowering : public TsPattern { using TsPattern::TsPattern; @@ -491,22 +548,10 @@ struct WhileOpLowering : public TsPattern // logic to support continue/break - auto visitorBreakContinue = [&](Operation *op) { - if (auto breakOp = dyn_cast_or_null(op)) - { - auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, breakOp.getLabelAttr()); - if (set) - tsContext->jumps[op] = continuation; - } - else if (auto continueOp = dyn_cast_or_null(op)) - { - auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, continueOp.getLabelAttr()); - if (set) - tsContext->jumps[op] = cond; - } - }; - - whileOp.getBody().walk(visitorBreakContinue); + visitBreakContinueInScope(whileOp.getBody(), labelAttr, /*eligibleForUnlabeledBreak*/ true, + /*eligibleForUnlabeledContinue*/ true, [&](Operation *op, bool isBreak) { + tsContext->jumps[op] = isBreak ? continuation : cond; + }); // end of logic for break/continue @@ -564,22 +609,10 @@ struct DoWhileOpLowering : public TsPattern // logic to support continue/break - auto visitorBreakContinue = [&](Operation *op) { - if (auto breakOp = dyn_cast_or_null(op)) - { - auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, breakOp.getLabelAttr()); - if (set) - tsContext->jumps[op] = continuation; - } - else if (auto continueOp = dyn_cast_or_null(op)) - { - auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, continueOp.getLabelAttr()); - if (set) - tsContext->jumps[op] = cond; - } - }; - - doWhileOp.getBody().walk(visitorBreakContinue); + visitBreakContinueInScope(doWhileOp.getBody(), labelAttr, /*eligibleForUnlabeledBreak*/ true, + /*eligibleForUnlabeledContinue*/ true, [&](Operation *op, bool isBreak) { + tsContext->jumps[op] = isBreak ? continuation : cond; + }); // end of logic for break/continue @@ -632,22 +665,10 @@ struct ForOpLowering : public TsPattern // logic to support continue/break - auto visitorBreakContinue = [&](Operation *op) { - if (auto breakOp = dyn_cast_or_null(op)) - { - auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, breakOp.getLabelAttr()); - if (set) - tsContext->jumps[op] = continuation; - } - else if (auto continueOp = dyn_cast_or_null(op)) - { - auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, continueOp.getLabelAttr()); - if (set) - tsContext->jumps[op] = incr; - } - }; - - forOp.getBody().walk(visitorBreakContinue); + visitBreakContinueInScope(forOp.getBody(), labelAttr, /*eligibleForUnlabeledBreak*/ true, + /*eligibleForUnlabeledContinue*/ true, [&](Operation *op, bool isBreak) { + tsContext->jumps[op] = isBreak ? continuation : incr; + }); // end of logic for break/continue @@ -715,22 +736,10 @@ struct LabelOpLowering : public TsPattern // logic to support continue/break - auto visitorBreakContinue = [&](Operation *op) { - if (auto breakOp = dyn_cast_or_null(op)) - { - auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, breakOp.getLabelAttr()); - if (set) - tsContext->jumps[op] = continuation; - } - else if (auto continueOp = dyn_cast_or_null(op)) - { - auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, continueOp.getLabelAttr()); - if (set) - tsContext->jumps[op] = begin; - } - }; - - labelOp.getLabelRegion().walk(visitorBreakContinue); + visitBreakContinueInScope(labelOp.getLabelRegion(), labelAttr, /*eligibleForUnlabeledBreak*/ true, + /*eligibleForUnlabeledContinue*/ true, [&](Operation *op, bool isBreak) { + tsContext->jumps[op] = isBreak ? continuation : begin; + }); // end of logic for break/continue @@ -761,7 +770,7 @@ struct LabelOpLowering : public TsPattern } else { - assert(false); + return rewriter.notifyMatchFailure(labelOp, "label region's last block does not terminate with MergeOp"); } rewriter.replaceOp(labelOp, continuation->getArguments()); @@ -852,7 +861,7 @@ struct SwitchOpLowering : public TsPattern } else { - assert(false); + return rewriter.notifyMatchFailure(switchOp, "cases region's last block does not terminate with MergeOp"); } rewriter.replaceOp(switchOp, continuation->getArguments()); @@ -1656,18 +1665,43 @@ struct TryOpLowering : public TsPattern if (linuxHasCleanups) { - auto resultOp = cast(cleanupBlockLast->getTerminator()); - rewriter.eraseOp(resultOp); - - if (catchHasOps) + if (catchHasOps || finallyHasOps) { - rewriter.mergeBlocks(catchesBlock, cleanupBlockLast); - } - else if (finallyHasOps) + auto resultOp = cast(cleanupBlockLast->getTerminator()); + rewriter.eraseOp(resultOp); + + if (catchHasOps) + { + rewriter.mergeBlocks(catchesBlock, cleanupBlockLast); + } + else + { + rewriter.mergeBlocks(finallyBlock, cleanupBlockLast); + } + } + else { - rewriter.mergeBlocks(finallyBlock, cleanupBlockLast); + // cleanup-only try (e.g. lowered from a `using` declaration with no explicit + // catch/finally): the Windows path already sets up its own landing pad for this + // case unconditionally at cleanupHasOps&&isWindows above; mirror that here for + // Linux so cleanupBlockLast keeps a valid terminator instead of losing it. + rewriter.setInsertionPointToStart(cleanupBlock); + + auto landingPadCleanupOp = rewriter.create( + loc, rttih.getLandingPadType(), rewriter.getBoolAttr(true), ValueRange{undefArrayValue}); + rewriter.create(loc); + + rewriter.setInsertionPoint(cleanupBlockLast->getTerminator()); + mlir::SmallVector unwindDests; + if (parentTryOpLandingPad) + { + unwindDests.push_back(parentTryOpLandingPad); + } + + auto resultOpCleanup = cast(cleanupBlockLast->getTerminator()); + rewriter.replaceOpWithNewOp(resultOpCleanup, landingPadCleanupOp, unwindDests); } - } + } rewriter.replaceOp(tryOp, continuation->getArguments()); diff --git a/tslang/lib/TypeScript/LowerToLLVM.cpp b/tslang/lib/TypeScript/LowerToLLVM.cpp index 2eef41109..4748ea224 100644 --- a/tslang/lib/TypeScript/LowerToLLVM.cpp +++ b/tslang/lib/TypeScript/LowerToLLVM.cpp @@ -1268,7 +1268,10 @@ struct SymbolCallInternalOpLowering : public TsLlvmPattern( loc, llvmFuncType, ::mlir::FlatSymbolRefAttr::get(rewriter.getContext(), op.getCallee()), transformed.getCallOperands()); @@ -2061,12 +2064,10 @@ struct NewOpLowering : public TsLlvmPattern storageType = valueRef.getElementType(); } - auto resultType = tch.convertType(newOp.getType()); - mlir::Value value; if (newOp.getStackAlloc().has_value() && newOp.getStackAlloc().value()) { - value = ch.Alloca(resultType, 1); + value = ch.Alloca(tch.convertType(storageType), 1); } else { diff --git a/tslang/test/tester/tests/00break_continue.ts b/tslang/test/tester/tests/00break_continue.ts index c46e70c49..5c6ed930c 100644 --- a/tslang/test/tester/tests/00break_continue.ts +++ b/tslang/test/tester/tests/00break_continue.ts @@ -4,6 +4,7 @@ function main() { assert(test_for() == 4, "failed. 3"); test_for_empty(); test_while_labeled(); + assert(test_triple_nested_unlabeled() == 394, "failed. 4"); test_label(); @@ -110,6 +111,29 @@ function test_while_labeled() { print("done."); } +function test_triple_nested_unlabeled() { + // an unlabeled break/continue at 3 levels of nesting must only affect its own + // (innermost) loop, regardless of the order the lowering passes visit the loops in. + let outerRuns = 0; + let middleRuns = 0; + let innerRuns = 0; + for (let i = 0; i < 3; i++) { + outerRuns++; + for (let j = 0; j < 3; j++) { + middleRuns++; + for (let k = 0; k < 10; k++) { + if (k == 2) continue; + if (k == 5) break; + innerRuns++; + } + } + } + + // outer runs 3 times, middle runs 3*3=9 times, inner: each of the 9 middle + // iterations runs k=0,1,(skip 2),3,4 then breaks at k==5 -> 4 increments each = 36 + return outerRuns * 100 + middleRuns * 10 + innerRuns / 9; +} + function test_label() { let c = 3; let a = 3;