From 29dc814a90fd16c7ae09973b113aac9f05a14d8c Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Sat, 11 Jul 2026 16:39:44 +0100 Subject: [PATCH 1/2] Refactor theModule handling in MLIRGenImpl - Add addGlobalConstructor() helper replacing five copy-pasted GlobalConstructorOp emission blocks - Add TempModuleScope RAII guard encapsulating the temp-module swap in evaluate/evaluatePropertyValue/evaluateElementAccess - Add save-and-set constructor to MLIRValueGuard - Remove dead non-MODULE_AS_NAMESPACE branch in mlirGen(ModuleDeclaration) Co-Authored-By: Claude Fable 5 --- .../TypeScript/MLIRLogic/MLIRValueGuard.h | 6 + tslang/lib/TypeScript/MLIRGen.cpp | 207 +++++------------- 2 files changed, 66 insertions(+), 147 deletions(-) diff --git a/tslang/include/TypeScript/MLIRLogic/MLIRValueGuard.h b/tslang/include/TypeScript/MLIRLogic/MLIRValueGuard.h index b06181048..433badc22 100644 --- a/tslang/include/TypeScript/MLIRLogic/MLIRValueGuard.h +++ b/tslang/include/TypeScript/MLIRLogic/MLIRValueGuard.h @@ -16,6 +16,12 @@ class MLIRValueGuard savedValue = value; } + MLIRValueGuard(T &value, T newValue) : value(value) + { + savedValue = value; + value = newValue; + } + ~MLIRValueGuard() { value = savedValue; diff --git a/tslang/lib/TypeScript/MLIRGen.cpp b/tslang/lib/TypeScript/MLIRGen.cpp index f9771355e..6bb1ee5aa 100644 --- a/tslang/lib/TypeScript/MLIRGen.cpp +++ b/tslang/lib/TypeScript/MLIRGen.cpp @@ -615,6 +615,20 @@ class MLIRGenImpl return anyCode; } + // appends GlobalConstructorOp after the last one in the module; LAST priority so it runs after CRT init + void addGlobalConstructor(mlir::Location location, StringRef funcName) + { + mlir::OpBuilder::InsertionGuard insertGuard(builder); + MLIRCodeLogicHelper mclh(builder, location, compileOptions); + + builder.setInsertionPointToStart(theModule.getBody()); + mclh.seekLastOp(theModule.getBody()); + + builder.create( + location, mlir::FlatSymbolRefAttr::get(builder.getContext(), funcName), + builder.getIndexAttr(LAST_GLOBAL_CONSTRUCTOR_PRIORITY)); + } + mlir::LogicalResult generateGlobalEntryCode(mlir::Location location, NodeArray statements, const GenContext &genContext) { @@ -675,18 +689,7 @@ class MLIRGenImpl if (useGlobalCtor) { - auto parentModule = theModule; - MLIRCodeLogicHelper mclh(builder, location, compileOptions); - - builder.setInsertionPointToStart(parentModule.getBody()); - mclh.seekLastOp(parentModule.getBody()); - - // priority is lowest to load as first dependencies - builder.create( - location, - mlir::FlatSymbolRefAttr::get(builder.getContext(), - fullGlobalFuncName), - builder.getIndexAttr(LAST_GLOBAL_CONSTRUCTOR_PRIORITY)); + addGlobalConstructor(location, fullGlobalFuncName); } return mlir::success(); @@ -960,40 +963,7 @@ class MLIRGenImpl mlir::LogicalResult mlirGen(ModuleDeclaration moduleDeclarationAST, const GenContext &genContext) { -#ifdef MODULE_AS_NAMESPACE return mlirGenNamespace(moduleDeclarationAST, genContext); -#else - auto isNamespace = (moduleDeclarationAST->flags & NodeFlags::Namespace) == NodeFlags::Namespace; - auto isNestedNamespace = - (moduleDeclarationAST->flags & NodeFlags::NestedNamespace) == NodeFlags::NestedNamespace; - if (isNamespace || isNestedNamespace) - { - return mlirGenNamespace(moduleDeclarationAST, genContext); - } - - auto location = loc(moduleDeclarationAST); - - auto moduleName = MLIRHelper::getName(moduleDeclarationAST->name); - - auto moduleOp = builder.create(location, StringRef(moduleName)); - - builder.setInsertionPointToStart(&moduleOp.getBody().front()); - - // save module theModule - auto parentModule = theModule; - theModule = moduleOp; - - GenContext moduleGenContext{}; - auto result = mlirGenBody(moduleDeclarationAST->body, moduleGenContext); - auto result = V(result); - - // restore - theModule = parentModule; - - builder.setInsertionPointAfter(moduleOp); - - return result; -#endif } mlir::LogicalResult mlirGenInclude(mlir::Location location, StringRef filePath, const GenContext &genContext) @@ -1101,12 +1071,6 @@ class MLIRGenImpl return mlir::failure(); } - auto parentModule = theModule; - MLIRCodeLogicHelper mclh(builder, location, compileOptions); - - builder.setInsertionPointToStart(parentModule.getBody()); - mclh.seekLastOp(parentModule.getBody()); - // The shared-lib load + symbol resolution call into LLVM's // sys::DynamicLibrary, which uses std::vector. In debug builds STL // iterators take a global lock that the CRT only initializes via its @@ -1116,8 +1080,7 @@ class MLIRGenImpl // Use the same band as the per-symbol __cctors (LAST) so it runs after // 'initlocks'; it is emitted before them, so it still loads the library // before any LLVMSearchForAddressOfSymbol runs. - builder.create( - location, mlir::FlatSymbolRefAttr::get(builder.getContext(), fullInitGlobalFuncName), builder.getIndexAttr(LAST_GLOBAL_CONSTRUCTOR_PRIORITY)); + addGlobalConstructor(location, fullInitGlobalFuncName); } for (auto declSymbol : symbols) @@ -4773,14 +4736,7 @@ class MLIRGenImpl return mlir::failure(); } - auto parentModule = theModule; - MLIRCodeLogicHelper mclh(builder, location, compileOptions); - - builder.setInsertionPointToStart(parentModule.getBody()); - mclh.seekLastOp(parentModule.getBody()); - - builder.create( - location, mlir::FlatSymbolRefAttr::get(builder.getContext(), fullInitGlobalFuncName), builder.getIndexAttr(LAST_GLOBAL_CONSTRUCTOR_PRIORITY)); + addGlobalConstructor(location, fullInitGlobalFuncName); } } else if (mlir::failed(processDeclaration(item, valClassItem, initFunc, genContext, true))) @@ -19613,15 +19569,7 @@ genContext); return mlir::failure(); } - auto parentModule = theModule; - MLIRCodeLogicHelper mclh(builder, location, compileOptions); - - builder.setInsertionPointToStart(parentModule.getBody()); - mclh.seekLastOp(parentModule.getBody()); - - // priority is lowest to load as first dependencies - builder.create( - location, mlir::FlatSymbolRefAttr::get(builder.getContext(), fullInitGlobalFuncName), builder.getIndexAttr(LAST_GLOBAL_CONSTRUCTOR_PRIORITY)); + addGlobalConstructor(location, fullInitGlobalFuncName); } return mlir::success(); @@ -19714,21 +19662,9 @@ genContext); mlir::LogicalResult createGlobalConstructor(ClassElement classMember, const GenContext &genContext) { auto location = loc(classMember); - - auto parentModule = theModule; - MLIRCodeLogicHelper mclh(builder, location, compileOptions); - auto funcName = getNameOfFunction(classMember, genContext); - { - mlir::OpBuilder::InsertionGuard insertGuard(builder); - - builder.setInsertionPointToStart(parentModule.getBody()); - mclh.seekLastOp(parentModule.getBody()); - - builder.create(location, - FlatSymbolRefAttr::get(builder.getContext(), StringRef(std::get<0>(funcName))), builder.getIndexAttr(LAST_GLOBAL_CONSTRUCTOR_PRIORITY)); - } + addGlobalConstructor(location, std::get<0>(funcName)); return mlir::success(); } @@ -20504,6 +20440,22 @@ genContext); return mlir::success(); } + // RAII scope that redirects theModule and the builder into the temp module + // for speculative evaluation and restores both when it goes out of scope. + class TempModuleScope + { + public: + TempModuleScope(MLIRGenImpl &mlirGenImpl) + : moduleGuard(mlirGenImpl.theModule), insertGuard(mlirGenImpl.builder) + { + mlirGenImpl.builder.setInsertionPointToStart(mlirGenImpl.prepareTempModule()); + } + + private: + MLIRValueGuard moduleGuard; + mlir::OpBuilder::InsertionGuard insertGuard; + }; + mlir::Block* prepareTempModule() { if (tempEntryBlock) @@ -20567,30 +20519,18 @@ genContext); //mlir::ScopedDiagnosticHandler diagHandler(builder.getContext(), [&](mlir::Diagnostic &diag) { //}); - auto location = loc(expr); - - // module - auto savedModule = theModule; + TempModuleScope tempModuleScope(*this); + SymbolTableScopeT varScope(symbolTable); + GenContext evalGenContext(genContext); + evalGenContext.allowPartialResolve = true; + evalGenContext.funcOp = tempFuncOp; + auto result = mlirGen(expr, evalGenContext); + auto initValue = V(result); + if (initValue) { - mlir::OpBuilder::InsertionGuard insertGuard(builder); - - SymbolTableScopeT varScope(symbolTable); - - builder.setInsertionPointToStart(prepareTempModule()); - - GenContext evalGenContext(genContext); - evalGenContext.allowPartialResolve = true; - evalGenContext.funcOp = tempFuncOp; - auto result = mlirGen(expr, evalGenContext); - auto initValue = V(result); - if (initValue) - { - func(initValue); - } + func(initValue); } - - theModule = savedModule; } mlir::Value evaluatePropertyValue(mlir::Location location, mlir::Value exprValue, const std::string &propertyName, const GenContext &genContext) @@ -20599,26 +20539,14 @@ genContext); mlir::ScopedDiagnosticHandler diagHandler(builder.getContext(), [&](mlir::Diagnostic &diag) { }); - mlir::Value initValue; - - // module - auto savedModule = theModule; + TempModuleScope tempModuleScope(*this); - { - mlir::OpBuilder::InsertionGuard insertGuard(builder); - builder.setInsertionPointToStart(prepareTempModule()); - - GenContext evalGenContext(genContext); - evalGenContext.allowPartialResolve = true; - evalGenContext.funcOp = tempFuncOp; - auto result = mlirGenPropertyAccessExpression(location, exprValue, propertyName, evalGenContext); - initValue = V(result); - } - - theModule = savedModule; - - return initValue; - } + GenContext evalGenContext(genContext); + evalGenContext.allowPartialResolve = true; + evalGenContext.funcOp = tempFuncOp; + auto result = mlirGenPropertyAccessExpression(location, exprValue, propertyName, evalGenContext); + return V(result); + } // TODO: rewrite code to get rid of the following method, write method to calculate type of field, we have method mth.getFieldTypeByFieldName mlir::Type evaluateProperty(mlir::Location location, mlir::Value exprValue, const std::string &propertyName, const GenContext &genContext) @@ -20649,31 +20577,16 @@ genContext); mlir::ScopedDiagnosticHandler diagHandler(builder.getContext(), [&](mlir::Diagnostic &diag) { }); - mlir::Type resultType; - - // module - auto savedModule = theModule; + TempModuleScope tempModuleScope(*this); - { - mlir::OpBuilder::InsertionGuard insertGuard(builder); - builder.setInsertionPointToStart(prepareTempModule()); - - GenContext evalGenContext(genContext); - evalGenContext.allowPartialResolve = true; - auto indexVal = builder.create(location, mth.getStructIndexType(), - mth.getStructIndexAttrValue(0)); - auto result = mlirGenElementAccess(location, expression, indexVal, isConditionalAccess, evalGenContext); - auto initValue = V(result); - if (initValue) - { - resultType = initValue.getType(); - } - } - - theModule = savedModule; - - return resultType; - } + GenContext evalGenContext(genContext); + evalGenContext.allowPartialResolve = true; + auto indexVal = builder.create(location, mth.getStructIndexType(), + mth.getStructIndexAttrValue(0)); + auto result = mlirGenElementAccess(location, expression, indexVal, isConditionalAccess, evalGenContext); + auto initValue = V(result); + return initValue ? initValue.getType() : mlir::Type(); + } ValueOrLogicalResult selectFieldsValues(mlir::Location location, SmallVector &values, mlir::Value value, ::llvm::ArrayRef<::mlir::typescript::FieldInfo> fields, bool filterSpecialCases, const GenContext &genContext, bool errorAsWarning = false) From e90e8153d5001312708b1c1702c8f3bb71be5f3d Mon Sep 17 00:00:00 2001 From: ASDAlexander77 Date: Sat, 11 Jul 2026 21:31:43 +0100 Subject: [PATCH 2/2] Refactor MLIRGenContext initialization and address memory management issues --- docs/MLIRGen-refactoring-review.md | 46 ++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/docs/MLIRGen-refactoring-review.md b/docs/MLIRGen-refactoring-review.md index a8003dca8..891b15d26 100644 --- a/docs/MLIRGen-refactoring-review.md +++ b/docs/MLIRGen-refactoring-review.md @@ -126,3 +126,49 @@ Suggested policy: new TODOs must reference an issue number; do a one-time sweep 6. **§6–§8** — opportunistic, alongside other work. Each step is independently shippable and verifiable with the existing `test/tester` suite plus `unittests/MLIRGenTests`. + +--- + +## Addendum — 2026-07-11 scan + +*File is now 26,618 lines. PR [#201](https://github.com/ASDAlexander77/TypeScriptCompiler/pull/201) delivered a first slice of §3: `addGlobalConstructor()` replaced five copy-pasted `GlobalConstructorOp` blocks, `TempModuleScope` (RAII) replaced four manual `theModule` save/restores around temp-module evaluation, `MLIRValueGuard` gained a save-and-set constructor, and the dead non-`MODULE_AS_NAMESPACE` branch in `mlirGen(ModuleDeclaration)` was removed.* + +### A1. `GenContext` initialization and ownership (extends §3) + +`MLIRGenContext.h` — two hazards beyond the traversal-state issue already described: + +- **Uninitialized members.** `GenContext() = default;` leaves ~15 `bool`s and raw pointers indeterminate. Every current instance is value-initialized (`GenContext ctx{};`) or copied, so the bug is latent — but a plain `GenContext ctx;` compiles and produces garbage codegen flags. Fix: default member initializers on every field; then the manual `clearScopeVars()`-style zeroing can shrink. +- **Raw owning pointers with manual delete.** `cleanUps`, `cleanUpOps`, `passResult`, `state` are freed by hand in `clean()`/`cleanState()`, while the struct is copied **48 times** in MLIRGen.cpp; copies share the pointers, so correctness depends on exactly one caller invoking `clean()`. Fix: ownership at the root context only, expressed with `unique_ptr` (copies hold a non-owning pointer), or a small refcounted holder. + +Related measurement: 31 of the 38 `const_cast`s are `const_cast` mutating a parameter declared `const` (§3's diagnosis stands; this is the count). + +### A2. Repeated source-file-switch pattern (new; same family as `TempModuleScope`) + +The trio `MLIRValueGuard vgSourceFile / vgFileName` + assignments appears **9×**: three include/import-loading sites (~751, ~833, ~986) and six generic-instantiation sites (e.g. ~2219) where it is additionally paired with `MLIRNamespaceGuard`. Extract: + +- `SourceFileScope(sourceFile, mainSourceFileName, newFile)` — RAII, one line per site; +- `GenericContextScope` — namespace + source file + file name, for the six instantiation sites. + +### A3. Inference loop guard hides a non-convergence bug (new) + +`resolveGenericParamsFromFunctionCall` (~2622): `if (totalProcessed > params.size() + 100) emitError("loop detected")` is an arbitrary bail-out admitting the parameter-inference fixpoint can cycle (`// TODO: find out the issue`). Deserves a root-cause pass with a reduced test; the guard should become an assertion once understood. + +### A4. Allocation hot spots (new) + +- **Arena interning on lookup paths.** `getFullNamespaceName(StringRef)` (~25926) heap-builds a `std::string` then permanently interns it via `.copy(stringAllocator)` (BumpPtrAllocator, never freed) — on *every* call, including failed lookups. 27 `.copy(stringAllocator)` sites total. Fix: compose lookup keys in a stack `SmallString<128>`; intern only on symbol registration. +- **`GenContext` copy churn.** Each of the 48 copies clones two `llvm::StringMap`s, a `std::string`, and a `NodeArray`. Split rarely-changing parts into a shared immutable block, or reserve copies for sites that actually mutate the maps. +- **`std::function` construction per call.** `StringSwitch>` tables for built-in utility types (`Readonly`/`Partial`/... at ~23255 and duplicated at ~23399) allocate closures on every `getType`; a plain switch is allocation-free. Same theme: the 18 `std::bind` sites — the 5-placeholder `std::bind(&MLIRGenImpl::cast, ...)` is repeated verbatim 4×, and the `anyOrUndefined`/`optionalValueOrUndefined` ternary 2×; lambdas or a small interface passed to `MLIRTypeHelper` (constructor currently takes four `std::function`s) remove both the duplication and the type-erasure overhead. + +### A5. Non-findings + +The full-module scans in `mlirDiscoverAllDependencies` (~734/~774) look expensive but are the deliberate, commented snapshot mechanism for nested discovery — superseded only if §4a (throwaway discovery module) lands. + +### Updated quick wins + +| Item | Effort | Value | +|---|---|---| +| A1 field initializers | ~30 min | removes a landmine class | +| A2 `SourceFileScope` | small, mechanical | −~60 lines, same spirit as PR #201 | +| A4 arena-interning fix | ~30 lines | stops unbounded arena growth on lookups | +| A4 `std::bind`→lambda | mechanical | readability + perf | +| A1 ownership / §3 | dedicated effort | biggest correctness payoff |