|
| 1 | +# LowerToAffineLoops.cpp / LowerToLLVM.cpp — bug review (2026-07-13) |
| 2 | + |
| 3 | +Static correctness audit of the two MLIR lowering passes |
| 4 | +(`tslang/lib/TypeScript/LowerToAffineLoops.cpp`, TypeScript dialect → Affine/SCF/CF, |
| 5 | +2325 lines; `tslang/lib/TypeScript/LowerToLLVM.cpp`, → LLVM IR, 6585 lines). |
| 6 | +Neither file had uncommitted changes or recent history at review time (last |
| 7 | +real touch was PR #198) — this was a full-file static audit, not a diff |
| 8 | +review, done via 8 parallel finder passes + direct verification. |
| 9 | + |
| 10 | +Status key: **CONFIRMED** = verified by reading the exact lines and comparing |
| 11 | +against sibling code/behavior. **PLAUSIBLE** = strong evidence, not |
| 12 | +independently reproduced with a running compiler. |
| 13 | + |
| 14 | +## Fixed in this pass (items 1–3) |
| 15 | + |
| 16 | +Verified: full ctest suite green after all three fixes (339/339 JIT, 342/342 |
| 17 | +AOT, `ctest --test-dir __build/tslang/windows-msbuild-2026-debug -C Debug -j8`). |
| 18 | +Items 1–2 additionally verified by direct JIT execution of new test cases; |
| 19 | +item 3 verified via `--emit=mlir-affine --mtriple=x86_64-pc-linux-gnu` IR |
| 20 | +inspection (this Windows JIT host can't execute the Itanium exception ABI the |
| 21 | +fix targets, since `isWindows` is derived from the target triple and this |
| 22 | +build's SEH path never sets `cmpValue` at all — see the method note at the |
| 23 | +bottom). |
| 24 | + |
| 25 | +### 1. `ForOp` lowering drops loop-carried results for `for(;;)` — CONFIRMED, FIXED, VERIFIED |
| 26 | + |
| 27 | +`LowerToAffineLoops.cpp:611-696` (`ForOpLowering`). `ValueRange args;` is |
| 28 | +default-constructed and only assigned from `condOp.getArgs()` in the |
| 29 | +`ConditionOp` branch (line 669). The `NoConditionOp` branch (infinite |
| 30 | +`for(;;)`, lines 674-678) never assigns `args`, so `rewriter.replaceOp(forOp, |
| 31 | +args)` at line 692 replaces all of `forOp`'s results with an empty list. |
| 32 | + |
| 33 | +**Failure scenario:** a `for(;;) { ... }` loop with loop-carried values |
| 34 | +(`ForOp`/`NoConditionOp` support `Variadic<AnyType>:$args`) hits the |
| 35 | +`replaceOp` result-count mismatch — an assert/crash in `applyPartialConversion`, |
| 36 | +or malformed IR in release builds. |
| 37 | + |
| 38 | +**Fix:** assign `args = noCondOp.getArgs()` in the `else` branch, mirroring the |
| 39 | +`ConditionOp` branch. |
| 40 | + |
| 41 | +**Verified:** JIT-executed a `for(;;) { if (i>=5) break; sum += i; i++; }` |
| 42 | +returning `sum` — correctly returns 10 (0+1+2+3+4), no crash. |
| 43 | + |
| 44 | +### 2. `ArrayShiftOp` uses wrong GEP element type — CONFIRMED, FIXED, VERIFIED |
| 45 | + |
| 46 | +`LowerToLLVM.cpp:2623-2624` (`ArrayShiftOpLowering`). The GEP computing the |
| 47 | +"rest of the array" source pointer for the post-shift `MemoryMoveOp` was built |
| 48 | +with `th.getI32Type()` as the GEP element type: |
| 49 | + |
| 50 | +```cpp |
| 51 | +auto offset1 = |
| 52 | + rewriter.create<LLVM::GEPOp>(loc, th.getPtrType(), th.getI32Type(), currentPtr, ValueRange{incSize}); |
| 53 | +``` |
| 54 | +
|
| 55 | +Every sibling array-mutation pattern (`ArrayPushOp`, `ArrayUnshiftOp`, |
| 56 | +`ArraySpliceOp`) and even the immediately preceding GEP in the same function |
| 57 | +(line 2620, `offset0`) correctly use `llvmElementType` as the GEP element type. |
| 58 | +
|
| 59 | +**Failure scenario:** `Array<number>.shift()` (element type `f64`, 8 bytes) |
| 60 | +computed the shift's source offset using a 4-byte stride instead of 8, |
| 61 | +corrupting array contents or reading out of bounds for arrays of |
| 62 | +wider-than-i32 elements. |
| 63 | +
|
| 64 | +**Fix:** changed the GEP element type from `th.getI32Type()` to |
| 65 | +`llvmElementType`. |
| 66 | +
|
| 67 | +**Verified:** JIT-executed `[10.5,20.5,30.5,40.5].shift()` — `first === 10.5`, |
| 68 | +remaining array is exactly `[20.5, 30.5, 40.5]` in order. |
| 69 | +
|
| 70 | +### 3. Mismatched typed `catch` silently falls through instead of rethrowing — CONFIRMED, FIXED, VERIFIED |
| 71 | +
|
| 72 | +`LowerToAffineLoops.cpp:1620-1633` (`TryOpLowering`, non-Windows/Itanium path). |
| 73 | +When the RTTI comparison (`cmpValue`) for a typed `catch` clause is false (the |
| 74 | +thrown value's type doesn't match the declared catch type), the conditional |
| 75 | +branch's false edge went to `continuation` — ordinary post-try control flow — |
| 76 | +instead of propagating/rethrowing the exception. The code carried its own |
| 77 | +admission of this: `// TODO: when catch not matching - should go into result |
| 78 | +(rethrow)`. |
| 79 | +
|
| 80 | +**Failure scenario:** `try { throw new TypeError() } catch (e: RangeError) {}` |
| 81 | +on the Itanium/Linux path silently swallowed the mismatched exception and |
| 82 | +continued normal execution instead of propagating it to an enclosing handler |
| 83 | +or terminating the process. |
| 84 | +
|
| 85 | +**Fix:** on RTTI mismatch, branch to a new block that creates a `NullOp` + |
| 86 | +`ThrowOp` (the existing "rethrow current exception" idiom used elsewhere in |
| 87 | +this function, e.g. the finally-rethrow path at line ~1605) instead of |
| 88 | +branching to `continuation`. If a parent `TryOp`'s landing pad exists |
| 89 | +(`parentTryOpLandingPad`), `tsContext->unwind[rethrowOp]` is set so |
| 90 | +`ThrowOpLowering` emits `ThrowUnwindOp` targeting it directly; otherwise |
| 91 | +`ThrowOpLowering` falls back to `ThrowCallOp` (process-level rethrow). No |
| 92 | +`EndCatchOp` is needed before the rethrow, matching the existing precedent's |
| 93 | +comment ("we do not need EndCatch as throw will redirect execution anyway"). |
| 94 | +
|
| 95 | +**Verified:** could not execute the Itanium ABI path natively on this Windows |
| 96 | +JIT host (see method note), so verified via `--emit=mlir-affine |
| 97 | +--mtriple=x86_64-pc-linux-gnu` IR dump instead of JIT execution: |
| 98 | +
|
| 99 | +- Single-level mismatch (`try{throw TypeErrorX}catch(e:RangeErrorX){}`, no |
| 100 | + parent try): false branch now emits `ts.Null` + `ts.ThrowCall` instead of |
| 101 | + branching to the continuation block. |
| 102 | +- Nested case (`try{try{throw TypeErrorX}catch(e:RangeErrorX){}}catch(e:TypeErrorX){}`): |
| 103 | + inner mismatch emits `ts.Null` + `ts.ThrowUnwind(...)[^parentLandingPad]`, |
| 104 | + which correctly re-enters the outer try's landing pad, re-evaluates its RTTI |
| 105 | + compare, matches, and the outer catch runs — confirmed by reading the full |
| 106 | + block chain in the IR dump. |
| 107 | +- Matching-catch happy path re-verified unaffected (JIT-executed, returns the |
| 108 | + caught value correctly). |
| 109 | +- Full ctest suite (339 JIT + 342 AOT) green with this change in place. |
| 110 | +
|
| 111 | +## Remaining known issues (not yet fixed, tracked here for follow-up) |
| 112 | +
|
| 113 | +4. **`LowerToAffineLoops.cpp:494,567,635,717`** (While/DoWhile/For/Label |
| 114 | + lowering) — the `visitorBreakContinue` walk recurses into nested |
| 115 | + loop/label regions, not just the immediate body. An unlabeled |
| 116 | + `break`/`continue` inside a nested loop can have its `tsContext->jumps[op]` |
| 117 | + entry set by either the inner or outer loop's walk, with the last one to |
| 118 | + run winning — order-dependent, so a nested unlabeled `break` could jump out |
| 119 | + of both loops instead of just the inner one. PLAUSIBLE, not yet fixed. |
| 120 | +
|
| 121 | +5. **`LowerToAffineLoops.cpp:1351,1358,1370`** (`TryOpLowering`, `finally` |
| 122 | + region cloning) — the `finally` region is cloned twice (normal-exit and |
| 123 | + return-cleanup copies) *after* the `parentTryOp`/`jumps` side-tables were |
| 124 | + populated by walking only the original region. A nested `try` or a |
| 125 | + `break`/`continue` inside a `finally` block loses correct wiring in the |
| 126 | + cloned copies. PLAUSIBLE, not yet fixed — needs a test case (`grep` of |
| 127 | + `test/` found no try/finally-nesting coverage). |
| 128 | +
|
| 129 | +6. **`LowerToLLVM.cpp:1271`** (`SymbolCallInternalOpLowering`) — `llvmFuncType` |
| 130 | + can remain null if `moduleOp.lookupSymbol` fails to resolve or the symbol is |
| 131 | + an unexpected kind; only guarded by a debug-only `assert`, no `return |
| 132 | + failure()` guard. PLAUSIBLE, not yet fixed. |
| 133 | +
|
| 134 | +7. **`LowerToLLVM.cpp:2043`** (`NewOpLowering`, stack-allocation path) — |
| 135 | + allocates using `resultType` (converted pointer type) instead of |
| 136 | + `storageType` (actual class layout) used by the sibling heap-allocation |
| 137 | + path; asymmetry could under-allocate the stack slot for `new` with |
| 138 | + `getStackAlloc()` set. PLAUSIBLE, needs confirming what `resultType` |
| 139 | + actually converts to for class types before treating as certain. |
| 140 | +
|
| 141 | +8. **`LowerToLLVM.cpp:4159`** (`LandingPadOpLowering`, Windows path) — |
| 142 | + cleanup-only branch reuses the typed-catch filter value instead of an |
| 143 | + empty/undef cleanup clause; the code has its own `// BUG: in LLVM landing |
| 144 | + pad is not fully implemented` comment. PLAUSIBLE, not yet fixed. |
| 145 | +
|
| 146 | +9. **`LowerToAffineLoops.cpp:1660`** (`TryOpLowering`, `linuxHasCleanups`) — |
| 147 | + merges `catchesBlock`/`finallyBlock` into `cleanupBlockLast` only when |
| 148 | + `catchHasOps`/`finallyHasOps`, but `linuxHasCleanups` doesn't imply either; |
| 149 | + a cleanup-only try with no catch/finally content could erase |
| 150 | + `cleanupBlockLast`'s terminator with nothing replacing it. PLAUSIBLE, not |
| 151 | + yet fixed. |
| 152 | +
|
| 153 | +10. **`LowerToAffineLoops.cpp:763`** (`LabelOpLowering`) — falls back to |
| 154 | + `assert(false)` if the label region's terminator block isn't a `MergeOp`; |
| 155 | + in release builds (asserts stripped) this is a silent no-op leaving a |
| 156 | + malformed CFG if the label region's live last block doesn't end in |
| 157 | + `MergeOp` (e.g. dead code after a break-only path). PLAUSIBLE, not yet |
| 158 | + fixed. Same pattern exists in `SwitchOpLowering` at line ~854. |
| 159 | +
|
| 160 | +## Other observations (not correctness bugs, lower priority) |
| 161 | +
|
| 162 | +- **Duplication**: the `visitorBreakContinue` lambda is copy-pasted verbatim |
| 163 | + in `WhileOpLowering`, `DoWhileOpLowering`, `ForOpLowering`, `LabelOpLowering` |
| 164 | + (~14 lines × 4). Six near-identical accessor-lowering structs in |
| 165 | + `LowerToAffineLoops.cpp` (905-1205, `ThisAccessorOp` through |
| 166 | + `BoundIndirectIndexAccessorOp`) could collapse to one templated helper. |
| 167 | + `CallHybridInternalOpLowering`/`InvokeHybridOpLowering` in `LowerToLLVM.cpp` |
| 168 | + duplicate ~100 lines of this-pointer/vtable dispatch logic. |
| 169 | +- **Altitude**: GC-root registration (`LowerToLLVM.cpp:1911`) and |
| 170 | + side-effect detection for global initializers (`LowerToLLVM.cpp:3492`, with |
| 171 | + a commented-out `MemoryEffectOpInterface` version right below it) both |
| 172 | + enumerate specific op/type kinds by hand instead of querying a trait — |
| 173 | + same class of issue as the `jit-globals-not-gc-roots` GC memory. A new type |
| 174 | + or op added elsewhere silently isn't covered. |
| 175 | +- No `CLAUDE.md` exists anywhere in this repo, so no conventions check |
| 176 | + applied. |
| 177 | +
|
| 178 | +## Method note |
| 179 | +
|
| 180 | +MLIR's dialect-conversion driver pre-collects a pre-order worklist (verified |
| 181 | +against `DialectConversion.cpp`), so parent ops always convert before their |
| 182 | +*original* nested children — ruling out a naive "child converted before |
| 183 | +parent sets up its context" race for `tsContext` side-tables. The real hazard |
| 184 | +in this file is region **cloning** (item 5): clones are new `Operation*` |
| 185 | +instances not covered by side-table entries populated before the clone was |
| 186 | +made. |
0 commit comments