Skip to content

Fix nested break/continue scoping, stack-new sizing, and 3 other lowering bugs #232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ASDAlexander77 merged 1 commit into from
Jul 14, 2026
+216 −113
Merged

Fix nested break/continue scoping, stack-new sizing, and 3 other lowering bugs #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 78 additions & 34 deletions docs/LowerToAffineLoops-LowerToLLVM-review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)

Expand Down
184 changes: 109 additions & 75 deletions tslang/lib/TypeScript/LowerToAffineLoops.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,63 @@ struct ResultOpLowering : public TsPattern<mlir_ts::ResultOp>
}
};

// 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 &region, mlir::StringAttr scopeLabel,
bool eligibleForUnlabeledBreak, bool eligibleForUnlabeledContinue,
llvm::function_ref<void(Operation *, bool)> onMatch)
{
for (auto &block : region)
{
for (auto &op : block)
{
if (auto breakOp = dyn_cast<mlir_ts::BreakOp>(&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<mlir_ts::ContinueOp>(&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<mlir_ts::WhileOp, mlir_ts::DoWhileOp, mlir_ts::ForOp, mlir_ts::LabelOp>(op);
auto isSwitchBoundary = isa<mlir_ts::SwitchOp>(op);

for (auto &nested : op.getRegions())
{
visitBreakContinueInScope(nested, scopeLabel,
eligibleForUnlabeledBreak && !isLoopOrLabelBoundary && !isSwitchBoundary,
eligibleForUnlabeledContinue && !isLoopOrLabelBoundary, onMatch);
}
}
}
}

struct WhileOpLowering : public TsPattern<mlir_ts::WhileOp>
{
using TsPattern<mlir_ts::WhileOp>::TsPattern;
Expand All @@ -491,22 +548,10 @@ struct WhileOpLowering : public TsPattern<mlir_ts::WhileOp>

// logic to support continue/break

auto visitorBreakContinue = [&](Operation *op) {
if (auto breakOp = dyn_cast_or_null<mlir_ts::BreakOp>(op))
{
auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, breakOp.getLabelAttr());
if (set)
tsContext->jumps[op] = continuation;
}
else if (auto continueOp = dyn_cast_or_null<mlir_ts::ContinueOp>(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

Expand Down Expand Up @@ -564,22 +609,10 @@ struct DoWhileOpLowering : public TsPattern<mlir_ts::DoWhileOp>

// logic to support continue/break

auto visitorBreakContinue = [&](Operation *op) {
if (auto breakOp = dyn_cast_or_null<mlir_ts::BreakOp>(op))
{
auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, breakOp.getLabelAttr());
if (set)
tsContext->jumps[op] = continuation;
}
else if (auto continueOp = dyn_cast_or_null<mlir_ts::ContinueOp>(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

Expand Down Expand Up @@ -632,22 +665,10 @@ struct ForOpLowering : public TsPattern<mlir_ts::ForOp>

// logic to support continue/break

auto visitorBreakContinue = [&](Operation *op) {
if (auto breakOp = dyn_cast_or_null<mlir_ts::BreakOp>(op))
{
auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, breakOp.getLabelAttr());
if (set)
tsContext->jumps[op] = continuation;
}
else if (auto continueOp = dyn_cast_or_null<mlir_ts::ContinueOp>(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

Expand Down Expand Up @@ -715,22 +736,10 @@ struct LabelOpLowering : public TsPattern<mlir_ts::LabelOp>

// logic to support continue/break

auto visitorBreakContinue = [&](Operation *op) {
if (auto breakOp = dyn_cast_or_null<mlir_ts::BreakOp>(op))
{
auto set = MLIRHelper::matchLabelOrNotSet(labelAttr, breakOp.getLabelAttr());
if (set)
tsContext->jumps[op] = continuation;
}
else if (auto continueOp = dyn_cast_or_null<mlir_ts::ContinueOp>(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

Expand Down Expand Up @@ -761,7 +770,7 @@ struct LabelOpLowering : public TsPattern<mlir_ts::LabelOp>
}
else
{
assert(false);
return rewriter.notifyMatchFailure(labelOp, "label region's last block does not terminate with MergeOp");
}

rewriter.replaceOp(labelOp, continuation->getArguments());
Expand Down Expand Up @@ -852,7 +861,7 @@ struct SwitchOpLowering : public TsPattern<mlir_ts::SwitchOp>
}
else
{
assert(false);
return rewriter.notifyMatchFailure(switchOp, "cases region's last block does not terminate with MergeOp");
}

rewriter.replaceOp(switchOp, continuation->getArguments());
Expand Down Expand Up @@ -1656,18 +1665,43 @@ struct TryOpLowering : public TsPattern<mlir_ts::TryOp>

if (linuxHasCleanups)
{
auto resultOp = cast<mlir_ts::ResultOp>(cleanupBlockLast->getTerminator());
rewriter.eraseOp(resultOp);

if (catchHasOps)
if (catchHasOps || finallyHasOps)
{
rewriter.mergeBlocks(catchesBlock, cleanupBlockLast);
}
else if (finallyHasOps)
auto resultOp = cast<mlir_ts::ResultOp>(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<mlir_ts::LandingPadOp>(
loc, rttih.getLandingPadType(), rewriter.getBoolAttr(true), ValueRange{undefArrayValue});
rewriter.create<mlir_ts::BeginCleanupOp>(loc);

rewriter.setInsertionPoint(cleanupBlockLast->getTerminator());
mlir::SmallVector<mlir::Block *> unwindDests;
if (parentTryOpLandingPad)
{
unwindDests.push_back(parentTryOpLandingPad);
}

auto resultOpCleanup = cast<mlir_ts::ResultOp>(cleanupBlockLast->getTerminator());
rewriter.replaceOpWithNewOp<mlir_ts::EndCleanupOp>(resultOpCleanup, landingPadCleanupOp, unwindDests);
}
}
}

rewriter.replaceOp(tryOp, continuation->getArguments());

Expand Down
9 changes: 5 additions & 4 deletions tslang/lib/TypeScript/LowerToLLVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,7 +1268,10 @@ struct SymbolCallInternalOpLowering : public TsLlvmPattern<mlir_ts::SymbolCallIn
llvmFuncType = tch.convertFunctionSignature(op.getContext(), tsFuncType.getResults(), tsFuncType.getInputs(), tsFuncType.isVarArg());
}

assert(llvmFuncType);
if (!llvmFuncType)
{
return rewriter.notifyMatchFailure(op, "unable to resolve callee symbol to a function type");
}

auto callRes = rewriter.create<LLVM::CallOp>(
loc, llvmFuncType, ::mlir::FlatSymbolRefAttr::get(rewriter.getContext(), op.getCallee()), transformed.getCallOperands());
Expand Down Expand Up @@ -2061,12 +2064,10 @@ struct NewOpLowering : public TsLlvmPattern<mlir_ts::NewOp>
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
{
Expand Down
Loading
Loading