From 0426f87047b4af3bb4303163823f93a9bd64e6a0 Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Fri, 17 Jul 2026 19:40:44 +0100 Subject: [PATCH] Fix generators losing state across manual .next() calls `const g = gen(); g.next()` (manual iteration on a real function* generator, as opposed to for...of) never advanced generator state: `g` is a storage-less const binding, so each `g.next()` property access fell into MLIRPropertyAccessCodeLogic's bound-ref fallback, which allocated a brand-new temp alloca seeded from the pristine, unmutated original on every call site. for...of avoided this because its lowering materializes one alloca up front and reuses it. Fix: a ScopedHashTable cache on MLIRGenImpl, keyed by the accessed object's SSA identity, lets repeated bound-ref accesses reuse the ref materialized on the first access. Scoped (not just cleared) at function entry via BoundRefCacheScopeT, mirroring symbolTable's own RAII scoping, since nested closures re-enter mlirGenFunctionBody recursively. Two follow-up fixes were needed after the initial full-suite run surfaced real regressions: - Gate the cache to real codegen passes only (!dummyRun && !allowPartialResolve). The discovery/type-inference pass generates then erases throwaway ops, and caching pointers into those ops let a later real pass read back a dangling or address-recycled mlir::Value, producing "null operand found" errors in unrelated functions. - Restrict cache hits to the same MLIR block as the access site. A ref materialized inside a nested `{ }` block doesn't dominate a reuse site outside it ("operand does not dominate this use"). A precise fix would use MLIR's DominanceInfo (not used anywhere else in this codebase); same-block is a conservative, cheap approximation that's never wrong, just sometimes conservative. A previous, broader fix attempt (forcing every bound-method-bearing const into real storage at declaration time) was reverted for breaking interface/symbol value-passing; this fix is narrower; it only affects call sites that already needed an address. Adds 00generator_manual_next.ts (moved from docs/bugs/, now a real passing regression test). Full suite: 702/702 passing. Co-Authored-By: Claude Sonnet 5 --- .../TypeScript/MLIRLogic/MLIRCodeLogic.h | 52 ++++++++++++++++++- .../TypeScript/MLIRLogic/MLIRDefines.h | 3 ++ tslang/lib/TypeScript/MLIRGenAccessCall.cpp | 22 +++++++- tslang/lib/TypeScript/MLIRGenFunctions.cpp | 2 + tslang/lib/TypeScript/MLIRGenImpl.h | 14 +++++ tslang/test/tester/CMakeLists.txt | 2 + .../tester/tests}/00generator_manual_next.ts | 3 -- 7 files changed, 93 insertions(+), 5 deletions(-) rename {docs/bugs => tslang/test/tester/tests}/00generator_manual_next.ts (87%) diff --git a/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h b/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h index 405e3c9dd..e785d7e9d 100644 --- a/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h +++ b/tslang/include/TypeScript/MLIRLogic/MLIRCodeLogic.h @@ -1150,8 +1150,18 @@ class MLIRPropertyAccessCodeLogic mlir::Attribute fieldId; mlir::Value argument; CompileOptions& compileOptions; + llvm::ScopedHashTable *boundRefMaterializedCache = nullptr; public: + // optional: lets a bound-method property access (e.g. `g.next` on a storage-less + // `const` binding) reuse the ref it materialized on a previous access instead of + // minting a fresh one seeded from the pristine, never-mutated value each time. See + // TupleNoError() and MLIRGenImpl::boundRefMaterializedCache. + void setBoundRefMaterializedCache(llvm::ScopedHashTable *cache) + { + boundRefMaterializedCache = cache; + } + MLIRPropertyAccessCodeLogic(CompileOptions& compileOptions, mlir::OpBuilder &builder, mlir::Location location, mlir::Value expression, StringRef name) : builder(builder), location(location), expression(expression), name(name), compileOptions(compileOptions) @@ -1239,11 +1249,31 @@ class MLIRPropertyAccessCodeLogic auto elementType = mth.isBoundReference(elementTypeForRef, isBoundRef); auto refValue = getExprLoadRefValue(location); + if (isBoundRef && !refValue && boundRefMaterializedCache) + { + // only reuse a ref materialized in the SAME block as this access: a ref + // minted inside a nested block (e.g. a `{ }` scope) does not dominate uses + // outside that block, and checking real dominance would need MLIR's + // DominanceInfo, which isn't otherwise used in this codebase. Same-block is + // a conservative, cheap approximation -- it misses some reuse opportunities + // across block boundaries but never returns a ref that fails to dominate. + auto cached = boundRefMaterializedCache->lookup(expression); + if (cached && cached.getParentBlock() == builder.getInsertionBlock()) + { + refValue = cached; + } + } + if (isBoundRef && !refValue) { // allocate in stack refValue = builder.create(location, mlir_ts::RefType::get(expression.getType()), expression); + + if (boundRefMaterializedCache) + { + boundRefMaterializedCache->insert(expression, refValue); + } } if (refValue) @@ -1260,7 +1290,7 @@ class MLIRPropertyAccessCodeLogic location, elementTypeForRef, expression, MLIRHelper::getStructIndex(builder, fieldIndex)); } - template ValueOrLogicalResult TupleGetSetAccessor(T tupleType, mlir::Attribute fieldId) + template ValueOrLogicalResult TupleGetSetAccessor(T tupleType, mlir::Attribute fieldId) { MLIRCodeLogic mcl(builder, compileOptions); @@ -1380,11 +1410,31 @@ class MLIRPropertyAccessCodeLogic auto elementType = mth.isBoundReference(elementTypeForRef, isBoundRef); auto refValue = getExprLoadRefValue(location); + if (isBoundRef && !refValue && boundRefMaterializedCache) + { + // only reuse a ref materialized in the SAME block as this access: a ref + // minted inside a nested block (e.g. a `{ }` scope) does not dominate uses + // outside that block, and checking real dominance would need MLIR's + // DominanceInfo, which isn't otherwise used in this codebase. Same-block is + // a conservative, cheap approximation -- it misses some reuse opportunities + // across block boundaries but never returns a ref that fails to dominate. + auto cached = boundRefMaterializedCache->lookup(expression); + if (cached && cached.getParentBlock() == builder.getInsertionBlock()) + { + refValue = cached; + } + } + if (isBoundRef && !refValue) { // allocate in stack refValue = builder.create(location, mlir_ts::RefType::get(expression.getType()), expression); + + if (boundRefMaterializedCache) + { + boundRefMaterializedCache->insert(expression, refValue); + } } if (refValue) diff --git a/tslang/include/TypeScript/MLIRLogic/MLIRDefines.h b/tslang/include/TypeScript/MLIRLogic/MLIRDefines.h index f6abf84c8..adab23935 100644 --- a/tslang/include/TypeScript/MLIRLogic/MLIRDefines.h +++ b/tslang/include/TypeScript/MLIRLogic/MLIRDefines.h @@ -46,6 +46,9 @@ using VariablePairT = std::pair; using SymbolTableScopeT = llvm::ScopedHashTableScope; +// see MLIRGenImpl::boundRefMaterializedCache +using BoundRefCacheScopeT = llvm::ScopedHashTableScope; + typedef std::pair SafeTypeKeyType; using SafeTypesMapScopeT = llvm::ScopedHashTableScope; diff --git a/tslang/lib/TypeScript/MLIRGenAccessCall.cpp b/tslang/lib/TypeScript/MLIRGenAccessCall.cpp index 7976c84e9..9821be471 100644 --- a/tslang/lib/TypeScript/MLIRGenAccessCall.cpp +++ b/tslang/lib/TypeScript/MLIRGenAccessCall.cpp @@ -32,6 +32,10 @@ namespace mlirgen { assert(objectValue); MLIRPropertyAccessCodeLogic cl(compileOptions, builder, location, objectValue, name); + if (!genContext.dummyRun && !genContext.allowPartialResolve) + { + cl.setBoundRefMaterializedCache(&boundRefMaterializedCache); + } return mlirGenPropertyAccessExpressionLogic(location, objectValue, false, cl, genContext); } @@ -41,6 +45,10 @@ namespace mlirgen { assert(objectValue); MLIRPropertyAccessCodeLogic cl(compileOptions, builder, location, objectValue, name); + if (!genContext.dummyRun && !genContext.allowPartialResolve) + { + cl.setBoundRefMaterializedCache(&boundRefMaterializedCache); + } return mlirGenPropertyAccessExpressionLogic(location, objectValue, isConditional, cl, genContext); } @@ -48,6 +56,10 @@ namespace mlirgen mlir::Attribute id, const GenContext &genContext) { MLIRPropertyAccessCodeLogic cl(compileOptions, builder, location, objectValue, id); + if (!genContext.dummyRun && !genContext.allowPartialResolve) + { + cl.setBoundRefMaterializedCache(&boundRefMaterializedCache); + } return mlirGenPropertyAccessExpressionLogic(location, objectValue, false, cl, genContext); } @@ -56,6 +68,10 @@ namespace mlirgen const GenContext &genContext) { MLIRPropertyAccessCodeLogic cl(compileOptions, builder, location, objectValue, id); + if (!genContext.dummyRun && !genContext.allowPartialResolve) + { + cl.setBoundRefMaterializedCache(&boundRefMaterializedCache); + } return mlirGenPropertyAccessExpressionLogic(location, objectValue, isConditional, cl, genContext); } @@ -65,8 +81,12 @@ namespace mlirgen const GenContext &genContext) { MLIRPropertyAccessCodeLogic cl(compileOptions, builder, location, objectValue, id, argument); + if (!genContext.dummyRun && !genContext.allowPartialResolve) + { + cl.setBoundRefMaterializedCache(&boundRefMaterializedCache); + } return mlirGenPropertyAccessExpressionLogic(location, objectValue, isConditional, cl, genContext); - } + } ValueOrLogicalResult MLIRGenImpl::mlirGenPropertyAccessExpressionLogic(mlir::Location location, mlir::Value objectValue, bool isConditional, MLIRPropertyAccessCodeLogic &cl, diff --git a/tslang/lib/TypeScript/MLIRGenFunctions.cpp b/tslang/lib/TypeScript/MLIRGenFunctions.cpp index e5a86087b..9539efe25 100644 --- a/tslang/lib/TypeScript/MLIRGenFunctions.cpp +++ b/tslang/lib/TypeScript/MLIRGenFunctions.cpp @@ -1172,6 +1172,7 @@ namespace mlirgen } SymbolTableScopeT varScope(symbolTable); + BoundRefCacheScopeT boundRefCacheScope(boundRefMaterializedCache); auto location = loc(functionLikeDeclarationBaseAST); @@ -1282,6 +1283,7 @@ namespace mlirgen LLVM_DEBUG(llvm::dbgs() << "\n!! >>>> SYNTH. FUNCTION: '" << fullFuncName << "' ~~~ " << (genContext.dummyRun ? "dummy run" : "") << (genContext.allowPartialResolve ? " allowed partial resolve" : "") << "\n";); SymbolTableScopeT varScope(symbolTable); + BoundRefCacheScopeT boundRefCacheScope(boundRefMaterializedCache); SmallVector attrs; processFunctionAttributes(attrs, genContext); diff --git a/tslang/lib/TypeScript/MLIRGenImpl.h b/tslang/lib/TypeScript/MLIRGenImpl.h index d1fe838cf..70def137f 100644 --- a/tslang/lib/TypeScript/MLIRGenImpl.h +++ b/tslang/lib/TypeScript/MLIRGenImpl.h @@ -10744,6 +10744,20 @@ class MLIRGenImpl llvm::ScopedHashTable symbolTable; + // Caches the stack-allocated ref materialized for a storage-less value (e.g. a + // `const` binding with no backing storage) the first time a bound-method property + // access needs an address for it (see MLIRPropertyAccessCodeLogic::TupleNoError). + // Without this, each access re-materializes a fresh copy seeded from the pristine, + // never-mutated SSA value, so repeated calls like `g.next()` on a `const`-bound + // generator never observe state changes made by earlier calls. Keyed by mlir::Value + // identity, which is stable and unique within a function. Scoped (not just cleared) + // at each mlirGenFunctionBody entry via BoundRefCacheScopeT, mirroring symbolTable's + // own scoping -- codegen for a nested closure recurses into mlirGenFunctionBody + // while the enclosing function's generation is still on the call stack, so a plain + // clear-on-entry would permanently drop the outer function's cache entries instead + // of restoring them when the nested closure's generation finishes. + llvm::ScopedHashTable boundRefMaterializedCache; + NamespaceInfo::TypePtr rootNamespace; NamespaceInfo::TypePtr currentNamespace; diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index 1a5599f85..78f8cb4f4 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -123,6 +123,7 @@ add_test(NAME test-compile-00-numbers COMMAND test-runner "${PROJECT_SOURCE_DIR} add_test(NAME test-compile-00-equals COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00equals.ts") add_test(NAME test-compile-00-bool-arith-ops COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00bool_arith_ops.ts") add_test(NAME test-compile-00-mixed-type-ops COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00mixed_type_ops.ts") +add_test(NAME test-compile-00-generator-manual-next COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00generator_manual_next.ts") add_test(NAME test-compile-00-funcs COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00funcs.ts") add_test(NAME test-compile-00-funcs-capture COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00funcs_capture.ts") add_test(NAME test-compile-00-funcs-vararg COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00funcs_vararg.ts") @@ -467,6 +468,7 @@ add_test(NAME test-jit-00-numbers COMMAND test-runner -jit "${PROJECT_SOURCE_DIR add_test(NAME test-jit-00-equals COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00equals.ts") add_test(NAME test-jit-00-bool-arith-ops COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00bool_arith_ops.ts") add_test(NAME test-jit-00-mixed-type-ops COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00mixed_type_ops.ts") +add_test(NAME test-jit-00-generator-manual-next COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00generator_manual_next.ts") add_test(NAME test-jit-00-funcs COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00funcs.ts") add_test(NAME test-jit-00-funcs-capture COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00funcs_capture.ts") add_test(NAME test-jit-00-funcs-vararg COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00funcs_vararg.ts") diff --git a/docs/bugs/00generator_manual_next.ts b/tslang/test/tester/tests/00generator_manual_next.ts similarity index 87% rename from docs/bugs/00generator_manual_next.ts rename to tslang/test/tester/tests/00generator_manual_next.ts index 69ead4ea6..f5e23ae92 100644 --- a/docs/bugs/00generator_manual_next.ts +++ b/tslang/test/tester/tests/00generator_manual_next.ts @@ -8,9 +8,6 @@ // original -- on every single call site. So every manual `.next()` call restarted the // generator instead of resuming it. `for...of` happened to work because its lowering // materializes the generator object into one persistent local up front and reuses it. -// -// fix: a const whose value is a tuple with a bound-method field (e.g. a generator or -// closure object) now gets real stack storage, matching what for...of already relied on. function* gen() { for (let i = 0; i < 5; i++) {