diff --git a/tslang/docs/const-let-storage-design.md b/tslang/docs/const-let-storage-design.md new file mode 100644 index 000000000..f321d4606 --- /dev/null +++ b/tslang/docs/const-let-storage-design.md @@ -0,0 +1,197 @@ +# `const`/`let` storage model: design and fix + +Branch: `const-let-storage-rework`. Status: **implemented and verified** (350/350 +JIT + 354/354 compile, 0 failures). + +## 1. The bug this addresses + +Three related generator bugs (see prior memory: `generator-const-manual-next-bug`, +`object-literal-generator-method-type-bug`, `generator-param-value-semantics-bug`) +all trace back to one design assumption in `registerVariable`: + +> `const` bindings never need real storage (an alloca); only `let`/`var` do. + +That assumption is correct for `const` bindings that hold plain values (numbers, +booleans, plain object-literal tuples with no methods). It is **wrong** for +`const` bindings that hold a value-typed aggregate with internal mutable state +accessed through a bound method — today, in practice, that means **generator +wrapper objects** (`{ step, next() {...} }`, built by +`buildGeneratorWrapperDeclaration` in `MLIRGenFunctions.cpp`). Calling `.next()` +mutates the `step` field in place; if the binding has no address, every call +site that needs a ref to mutate through has to invent one from scratch, seeded +from the pristine, never-mutated original value — so state never advances. + +## 2. Why this is narrower than "rework const/let for all types" + +Initial framing (see conversation) assumed the fix was general: decide storage +need by "does the type have mutable identity" across all TS constructs. A +survey of how each construct actually lowers to LLVM shows this is unnecessary +for everything except tuples: + +| TS construct | MLIR type | LLVM representation | Pointer-like? | Mutation-through-alias works today regardless of const/let? | +|---|---|---|---|---| +| `class` instance | `!ts.class<...>` | `NewOp` → alloca/heap, type maps to `LLVMPointerType` | Yes | Yes — SSA value already is a pointer | +| Array (`T[]`) | `!ts.array` | `{ptr, len}` header over heap buffer | Yes | Yes | +| Const/fixed array | `!ts.const_array` | `LLVMPointerType` | Yes | Yes | +| Object needing bound methods | `!ts.object` | `LLVMPointerType` to named struct | Yes | Yes | +| Plain object literal `{a,b}` (no methods) | `!ts.tuple<...>` / `!ts.const_tuple<...>` | bare LLVM struct **by value** | No | N/A — no mutation surface, so it's fine that it's value-like | +| **Generator wrapper `{step, next()}`** | `!ts.tuple<...>` / `!ts.const_tuple<...>` | bare LLVM struct **by value** | **No** | **No — this is the bug** | + +Classes, arrays, and true "object" types are already pointer-like at the LLVM +level, so aliasing/mutation already works no matter what `registerVariable` +does with `const`. The only gap is: **a value-typed tuple aggregate that +contains a bound-method field** (first field/param typed `OpaqueType` / +`ObjectType`, i.e. carries a `this`) needs an address, but today only gets one +if the codebase falls back to the ad hoc `boundRefMaterializedCache`. + +This narrows the fix from "rearchitect storage for every type" to: **detect +"this variable's type has a bound-method field" at `registerVariable` time and +force real storage then, for both `const` and `let`, instead of only +discovering the need lazily/locally at the first property access.** + +## 3. Two independent bugs, two fixes + +### 3a. Local const-storage bug (same function body) — fix now + +Today: `const g = gen(); g.next(); g.next();` relies on +`boundRefMaterializedCache` (`MLIRCodeLogic.h:1153` + +`MLIRGenImpl.h:10775`) — a `ScopedHashTable` consulted +only inside `MLIRPropertyAccessCodeLogic::Tuple`/`TupleNoError`, gated on +`!dummyRun && !allowPartialResolve`, and restricted to same-block reuse because +there's no real dominance check. This works but is a workaround bolted onto a +storage decision that was made too early and too coarsely (by `const`-ness +alone, ignoring the type). + +**Fix as implemented:** `detectFlags` runs before the initializer's type is +resolved (`func(...)` — which re-invokes `mlirGen` on the initializer +expression and has real side effects, so it can only run once — is only +called later, inside `getVariableTypeAndInit`), so the predicate can't be +decided that early. Instead: + +- `MLIRTypeHelper::hasBoundMethodField(type)` (`MLIRTypeHelper.h`, next to + `hasUndefines`) mirrors the existing `hasUndefinesLogic` pattern + (`llvm::any_of(type.getFields(), ...)`), testing each field via + `isBoundReference(fieldType, isBound)` instead of undefined/null — true for + a `TupleType`/`ConstTupleType` with at least one bound-method field. +- `VariableDeclarationInfo::processConstRef` (`MLIRGenImpl.h`) calls + `getVariableTypeAndInit` (as before), then computes + `needsIdentityStorage = mth.hasBoundMethodField(type)` and returns early + (skipping the value-only `ConstRef` path) when true. +- `getVariableTypeAndInit` gained a `typeAndInitResolved` guard so it can + safely be called a second time (a no-op reusing the already-resolved + type/initial) — needed because `registerVariable` now calls + `createLocalVariable` (which also calls `getVariableTypeAndInit` at its top) + *after* `processConstRef` already resolved everything, for the + `needsIdentityStorage` case. Without the guard, the initializer expression + (e.g. the `gen(...)` call) would be code-generated twice. +- `registerVariable` (`MLIRGenVariables.cpp`) branches: + +``` +if (variableDeclarationInfo.isConst) { + processConstRef(...); + if (variableDeclarationInfo.needsIdentityStorage) + createLocalVariable(...); // real alloca, same as `let` +} else { + createLocalVariable(...); +} +``` + +`adjustLocalVariableType` (`MLIRGenImpl.h`) still early-returns for any +`isConst` type without widening it, so a `const` generator's type is +unaffected by this second pass through `createLocalVariable` — only the +storage decision changes. + +**A second bug surfaced by this change, also fixed:** `resolveIdentifierAsVariable` +(`MLIRGenVariables.cpp:910`) only emits a `LoadOp` through a variable's +`RefType` storage when `varDecl->getReadWriteAccess()` is true; otherwise it +returns the raw ref value unloaded. `createVariableDeclaration` +(`MLIRGenImpl.h`) previously only set read-write-access for `!isConst` (i.e. +`let`/`var`) — a plain `const` never had storage, so this didn't matter for +it. Once a `const` generator gets real storage, it hit this gate too: the +first test run (`00generator_manual_next2.ts`, `drainTwo(it)`) crashed +(0xC0000005) because every use of `it` yielded the bare `RefType`/pointer +instead of the loaded struct value, and a cast to the expected +`!llvm.struct<(i32,i32,ptr,ptr)>` from `!llvm.ptr` failed. Fix: +`createVariableDeclaration` now also sets read-write-access when +`needsIdentityStorage` is true. This is correct for the *other* consumer of +this flag too — `MLIRCodeLogic::CaptureTypeStorage` (`MLIRCodeLogic.h:254`) +decides by-value vs. by-ref closure capture from the same flag, and a +`const` generator captured by a closure should be captured by reference for +the same reason it needs storage in the first place. Note this flag's name +is misleading: despite being called "read-write access," it has nothing to +do with TypeScript-level const-reassignment (this codebase does not enforce +reassignment-to-const as a diagnostic anywhere in MLIRGen) — its only two +consumers are the load-through-ref gate and the capture-by-ref decision. + +Once this lands, the `boundRefMaterializedCache` machinery becomes dead for +the case it was built for (a `const` generator already has real storage +before the first `.next()` call, so the property-access fallback in +`MLIRPropertyAccessCodeLogic::Tuple`/`TupleNoError` finds `refValue` already +set via the normal load-ref path and never reaches the cache branch). Verified: +full suite passes with the cache code still in place and untouched. Decision: +leave the cache in place rather than remove it in this change — removing it +in the same change as the storage fix would make it harder to bisect if +something unrelated regresses, and there may be shapes (e.g. object-literal +generator methods, closures) that still exercise it. Revisit removal as a +separate, later cleanup once more confidently redundant. + +### 3b. Cross-function parameter aliasing bug — NOT fixed by 3a, tracked separately + +`generator-param-value-semantics-bug`: passing a `const` (or `let`) generator +as a function parameter still copies it, because: + +- The call lowers a tuple-typed argument as a bare LLVM struct **by value** + (`TupleType`/`ConstTupleType` → unnamed packed `LLVMStructType`, + `LowerToLLVM.cpp:6063-6081`) — standard call ABI, no hidden indirection. +- The callee's `mlirGenFunctionParams` (`MLIRGenFunctions.cpp:1026-1070`) wraps + every incoming argument in `ParamOp`, which lowers + (`LowerToAffineLoops.cpp:183-193`, `ParamOpLowering`) to a **fresh** + `VariableOp` seeded from that by-value argument — a new box, unconditionally, + regardless of const/let on either side. + +Fixing 3a does not touch this path at all: the caller having real storage for +its own `const g` doesn't change what gets copied into the callee's block +argument. This needs a distinct fix at the ABI boundary — e.g. detecting +"parameter type has a bound-method field" (the same predicate from 3a) and +passing such parameters by reference (pointer to the caller's storage) instead +of by value, analogous to how `class`/`array`/`object` types are already +pointer-like and thus already "by reference" in effect. This is deferred: +broader blast radius (touches call-site argument lowering and the function +ABI/calling convention, not just declaration codegen), and out of scope for +this change. Tracked by the existing `generator-param-value-semantics-bug` +memory; revisit after 3a is verified stable. + +## 4. Testing — done, results + +1. Build Debug via PowerShell (per workflow memory) — clean build, all TUs + recompiled. +2. First run of `00generator_manual_next2.ts` crashed 0xC0000005 — not the bug + this change targets, but a second bug this change *surfaced* (see §3a's + `getReadWriteAccess` discovery above). Fixed by also setting + read-write-access when `needsIdentityStorage`. After that fix, this test + and `00generator_manual_next.ts` / `00generator7.ts` all pass cleanly (only + pre-existing benign "using casting to undefined value" / "losing this + reference" warnings). +3. Full suite: **350/350 JIT + 354/354 compile, 0 failures** (`ctest --test-dir + ... -R "^test-jit-" -j8` / `-R "^test-compile-" -j8`), covering the + previously-fragile set from the original 3-round const-manual-next fix + (`00disposable`/`01disposable`/`02disposable`, `00spread`, + `01symbol`, etc.) with no regressions. +4. Confirmed 3b (parameter aliasing) is still present and unchanged: a + throwaway repro (`gen`/`drainTwo` calling `.next()` twice inside the callee, + then asserting the caller's `it.next()` returns the *next* value after + those two calls) hit the JIT-executed program's own `assert()` failing at + runtime — visible only as an apparent "hang" that was actually an invisible + Windows CRT assert dialog (confirmed via `Get-Process` showing a + `tslang.exe` with `MainWindowTitle` "Microsoft Visual C++ Runtime Library"; + see `windows-assert-dialog-looks-like-hang` memory). This is the expected, + pre-existing failure mode for 3b — not newly introduced, not masked. + +## 5. Non-goals + +- Not changing what `const`/`let` mean for reassignment checking. +- Not adding storage for classes/arrays/plain objects — they don't need it. +- Not fixing the parameter-aliasing bug (3b) in this change. +- Not introducing real MLIR `DominanceInfo` — out of scope; the existing + same-block conservatism in the (soon-to-be-mostly-dead) + `boundRefMaterializedCache` path is untouched by this change. diff --git a/tslang/include/TypeScript/MLIRLogic/MLIRTypeHelper.h b/tslang/include/TypeScript/MLIRLogic/MLIRTypeHelper.h index 28165d868..df71a3189 100644 --- a/tslang/include/TypeScript/MLIRLogic/MLIRTypeHelper.h +++ b/tslang/include/TypeScript/MLIRLogic/MLIRTypeHelper.h @@ -1604,6 +1604,36 @@ class MLIRTypeHelper return false; } + template bool hasBoundMethodFieldLogic(T type) + { + return llvm::any_of(type.getFields(), [&](::mlir::typescript::FieldInfo fi) { + bool isBound = false; + isBoundReference(fi.type, isBound); + return isBound; + }); + } + + // true if `type` is a value-typed aggregate (tuple/const-tuple) with at least + // one bound-method field (e.g. the generator wrapper's `next`). Such a value + // has mutable state (advanced by calling the bound method) but, unlike + // class/array/object types, is NOT already pointer-like at the LLVM level -- + // so a binding of this type needs real storage (an address) to let mutation + // be visible across property accesses / call sites, even when declared `const`. + bool hasBoundMethodField(mlir::Type type) + { + if (auto constTuple = dyn_cast(type)) + { + return hasBoundMethodFieldLogic(constTuple); + } + + if (auto tuple = dyn_cast(type)) + { + return hasBoundMethodFieldLogic(tuple); + } + + return false; + } + mlir::Type mergeIntTypes(mlir::Type typeLeft, mlir::Type typeRight, bool& found) { found = false; diff --git a/tslang/lib/TypeScript/MLIRGenImpl.h b/tslang/lib/TypeScript/MLIRGenImpl.h index d4a2eb9a4..1a45d5d81 100644 --- a/tslang/lib/TypeScript/MLIRGenImpl.h +++ b/tslang/lib/TypeScript/MLIRGenImpl.h @@ -693,7 +693,8 @@ class MLIRGenImpl { VariableDeclarationInfo(CompileOptions& compileOptions) : compileOptions(compileOptions), variableName(), fullName(), initial(), type(), storage(), globalOp(), varClass(), scope{VariableScope::Local}, isFullName{false}, isGlobal{false}, isConst{false}, isExternal{false}, isExport{false}, isImport{false}, - isSpecialization{false}, allocateOutsideOfOperation{false}, allocateInContextThis{false}, comdat{Select::NotSet}, deleted{false}, isUsed{false} + isSpecialization{false}, allocateOutsideOfOperation{false}, allocateInContextThis{false}, comdat{Select::NotSet}, deleted{false}, isUsed{false}, + needsIdentityStorage{false}, typeAndInitResolved{false} { }; @@ -784,6 +785,18 @@ class MLIRGenImpl return mlir::failure(); } + // a const binding whose type is a value-typed aggregate with a bound-method + // field (e.g. a generator wrapper's `next`) has mutable state but no + // pointer-like representation -- it still needs real storage so that + // mutation through the bound method is visible across property accesses. + // See docs/const-let-storage-design.md. + MLIRTypeHelper mth(builder.getContext(), compileOptions); + needsIdentityStorage = mth.hasBoundMethodField(type); + if (needsIdentityStorage) + { + return mlir::success(); + } + if (varClass == VariableType::ConstRef) { MLIRCodeLogic mcl(builder, compileOptions); @@ -803,6 +816,18 @@ class MLIRGenImpl mlir::LogicalResult getVariableTypeAndInit(mlir::Location location, const GenContext &genContext) { + // `func` re-runs mlirGen on the initializer expression, which emits real + // ops (e.g. re-invokes a call expression) -- must not run twice. A const + // binding that turns out to need identity storage calls this once via + // processConstRef() and then again via createLocalVariable(); guard so + // the second call just reuses what's already resolved. + if (typeAndInitResolved) + { + return type ? mlir::success() : mlir::failure(); + } + + typeAndInitResolved = true; + auto [type, init, typeProvided] = func(location, genContext); if (!type) { @@ -837,7 +862,13 @@ class MLIRGenImpl VariableDeclarationDOM::TypePtr createVariableDeclaration(mlir::Location location, const GenContext &genContext) { auto varDecl = std::make_shared(fullName, type, location); - if (!isConst || varClass == VariableType::ConstRef) + // readWriteAccess really means "this binding has real RefType storage that + // must be loaded through / can be captured by reference" -- it is not a + // const-reassignment check (this codebase doesn't enforce one at this + // layer). A const with identity storage (see needsIdentityStorage / + // hasBoundMethodField) has real storage exactly like `let`, so it needs + // the same load-through-ref and by-ref-capture treatment. + if (!isConst || varClass == VariableType::ConstRef || needsIdentityStorage) { varDecl->setReadWriteAccess(); // TODO: HACK: to mark var as local and ignore when capturing @@ -925,6 +956,8 @@ class MLIRGenImpl Select comdat; bool deleted; bool isUsed; + bool needsIdentityStorage; + bool typeAndInitResolved; }; mlir::LogicalResult adjustLocalVariableType(mlir::Location location, struct VariableDeclarationInfo &variableDeclarationInfo, const GenContext &genContext) diff --git a/tslang/lib/TypeScript/MLIRGenVariables.cpp b/tslang/lib/TypeScript/MLIRGenVariables.cpp index 4d479eb7e..22f82dcad 100644 --- a/tslang/lib/TypeScript/MLIRGenVariables.cpp +++ b/tslang/lib/TypeScript/MLIRGenVariables.cpp @@ -102,6 +102,13 @@ namespace mlirgen if (variableDeclarationInfo.isConst) { if (mlir::failed(variableDeclarationInfo.processConstRef(location, builder, genContext))) return mlir::Type(); + + // a const binding that turns out to need identity storage (see + // processConstRef / hasBoundMethodField) falls through to the same + // real-storage path as `let` instead of staying a bare SSA value. + if (variableDeclarationInfo.needsIdentityStorage + && mlir::failed(createLocalVariable(location, variableDeclarationInfo, genContext))) + return mlir::Type(); } else if (mlir::failed(createLocalVariable(location, variableDeclarationInfo, genContext))) return mlir::Type(); }