diff --git a/tslang/docs/interface-vtable-simplification-design.md b/tslang/docs/interface-vtable-simplification-design.md index d559bd2c7..3c680313b 100644 --- a/tslang/docs/interface-vtable-simplification-design.md +++ b/tslang/docs/interface-vtable-simplification-design.md @@ -1,13 +1,13 @@ # Interface vtable simplification: design -Status: **PR 1 (§4 + §5) implemented, full suite green (716/716)**. §3 -(constant vtables for capture-free literal methods) not yet started. Follow-up -to PR #251 (heap-allocated patched vtable) and PR #252 (canonical slot -numbering, `fix-interface-vtable-index-mismatch@be2c9620`). All anchors below -verified by code inspection on that branch, except where PR 1's implementation -notes (end of §4) correct them. Goal: remove the last *runtime-patched* vtable -path for the common case, make slot numbering single-sourced, and fix one -latent cast-order miscompile found during review. +Status: **PR 1 (§4 + §5) and PR 2 (§3) both implemented, full suite green +(718/718)**. Follow-up to PR #251 (heap-allocated patched vtable) and PR #252 +(canonical slot numbering, `fix-interface-vtable-index-mismatch@be2c9620`). +All anchors below verified by code inspection at the time each PR started; +see each section's "Implementation notes" subsection for what changed, or was +found to be wrong, while building it. Goal: remove the last *runtime-patched* +vtable path for the common case, make slot numbering single-sourced, and fix +latent bugs found during review and implementation. ## 1. Current architecture: what a vtable slot means @@ -120,6 +120,78 @@ the #251 heap machinery becomes dead on the main path (kept for the fallback); the vtable global can be const-qualified; cross-module behavior stops depending on which module happened to run a cast. +### Implementation notes (PR 2, as landed) + +Two corrections to the plan above, both found by testing rather than +inspection - the "captures need a fallback" premise was actually wrong in the +*helpful* direction (one fewer case to handle), and a second, unrelated +distinction turned out to be load-bearing that the plan didn't mention at all. + +**Captures don't need the fallback.** Rereading `addObjectFuncFieldInfo` +(`MLIRGenImpl.h`) shows the func-typed field's value is +`FlatSymbolRefAttr(funcName)` unconditionally - captures-with-methods land in +`methodInfosWithCaptures` *in addition to*, not *instead of*, the same +symbol-valued field. What captures actually add is a separate `.captured` +data field the (single, shared) lifted function reads via `this` at call time +(`mlirGenObjectLiteralCaptures`) - not a different function per instance. So +the side table (`objectLiteralMethodSymbolsMap`, keyed on the object literal's +own `ObjectStorageType`, populated in `addObjectFuncFieldInfo` for every +method, captures or not) needed no captures-awareness at all. The imported- +object-type fallback (side-table miss, since there's no local `funcOp` to +name) is the only case actually exercised in practice today - no test yet +covers it explicitly (see §7's test matrix, still open). + +**A vtable-identity subtlety this plan didn't anticipate, but had to solve +to key the side table at all:** vtable globals are named by +`hash_value(objectType)` (`interfaceVTableNameForObject`) - structural, not +nominal. The worry this raises - two differently-implemented, same-shape +literals colliding on one vtable global and a baked-in constant only being +correct for one of them - does not happen, verified by compiling a +counterexample (`{count:0,inc(){...+1}}` vs `{count:0,inc(){...+100}}`): +they get different vtable globals. Why: a method field's `FunctionType` +carries `this` as an explicit first parameter typed `object>`, +and that symbol embeds the literal's own source location - so the "same +shape" premise never actually holds once the method's own signature is +counted. This is also why the side table has to be keyed on that *nested* +`ObjectStorageType` (recovered at the vtable-build site by reading +`FunctionType.getInputs().front()` off the object's own field, cast to +`ObjectType`, `.getStorageType()`), not on the boxed `ObjectType` directly, +since the boxed type wraps a converted plain `TupleType` (`mlirGen(ObjectLiteralExpression)`, +`convertConstTupleTypeToTupleType`), a *different* MLIR `Type` value than +`oli.objThis` captured when the field was added, even though one is derived +from the other. + +**The bug that actually broke the regression suite (22 failures on the first +pass): `PropertySignature` with a function type is not a `MethodSignature`.** +`interface Something { toString: () => string; }` categorizes `toString` as +an interface **field** (`InterfaceInfo::fields`), not a method +(`InterfaceInfo::methods`) - `getVirtualTable`'s `methodsAsFields=true` only +governs how the *object's own* func-typed field gets resolved into the local +`virtualTable` snapshot; it collapses every entry to `isField=true` +regardless of which list the *interface* actually declared the member in. The +access site, however, dispatches on the interface's real categorization +(`InterfaceMembers` tries `findField` before `findMethod`) - so +`toString`'s access always goes through `InterfaceFieldAccess` +(`thisVal + slotValue`, offset semantics), never `InterfaceMethodAccess` +(raw function-pointer slot, `BoundFunctionType`), no matter how the object +stores it. The first version of this change substituted a constant +`SymbolRefOp` into *any* func-typed slot, including `toString`'s - handing a +raw function pointer to code that adds it to `thisVal` expecting an offset, +producing a garbage call target. Crash confirmed via WinDbg +(`WinDbgX.exe -g -c ".dump /ma ..."` then offline `!analyze -v`; see +[[windbg-available-for-debugging]]): faulting instruction `call qword ptr +[rax+rdx]`, both registers holding `thisVal`/slot-sum garbage. Fix: gate the +symbol substitution on `newInterfacePtr->findMethod(fieldName)` actually +finding the member in the interface's own (extends-inclusive) method list, +not merely on the vtable entry being func-typed +(`mlirGenObjectVirtualTableDefinitionForInterface`). Regression test: +`00interface_function_typed_field.ts` - `00interface_object.ts` (pre-existing) +already covered this pattern and is what surfaced the bug, but a minimal, +purpose-labeled test makes the invariant explicit for future readers. + +718/718 tests green (716 from PR 1 + this section's new regression test +1 +already counted, plus the incidental fix above). + ## 4. Change 2: a single writer for `virtualIndex` Slot numbering is currently embodied in four places: diff --git a/tslang/lib/TypeScript/MLIRGenImpl.h b/tslang/lib/TypeScript/MLIRGenImpl.h index 176466941..9f49e5dd6 100644 --- a/tslang/lib/TypeScript/MLIRGenImpl.h +++ b/tslang/lib/TypeScript/MLIRGenImpl.h @@ -7423,6 +7423,21 @@ class MLIRGenImpl oli.values.push_back(mlir::FlatSymbolRefAttr::get(builder.getContext(), funcName)); oli.fieldInfos.push_back({fieldId, type, false, mlir_ts::AccessLevel::Public}); + // record (this literal's object-storage type, field name) -> the lifted function's + // symbol so a later interface vtable build (mlirGenObjectVirtualTableDefinitionForInterface) + // can emit a constant SymbolRefOp for this method slot instead of the runtime + // load-from-object patch - see docs/interface-vtable-simplification-design.md §3. + // oli.objThis.getStorageType() is the SAME (hash-consed) ObjectStorageType that ends + // up embedded as the "this" parameter of this field's own FunctionType, which is how + // the vtable builder recovers this key later even though by then it only has the + // BOXED object type (whose top-level storage is a plain TupleType, not this + // ObjectStorageType directly - see the design doc's §3 implementation notes for why + // that indirection is necessary, not optional). Captures are fine here too: a + // captures-bearing method's field value is still this same compile-time-constant + // funcName (the per-instance data lives in a separate `.captured` field the function + // reads via `this`, not in a different function per instance). + objectLiteralMethodSymbolsMap[oli.objThis.getStorageType()][fieldId] = funcName; + if (getCaptureVarsMap().find(funcName) != getCaptureVarsMap().end()) { oli.methodInfosWithCaptures.push_back({funcName, oli.fieldInfos.size() - 1}); @@ -10852,6 +10867,43 @@ class MLIRGenImpl llvm::ScopedHashTable fullNameInterfacesMap; + // (object literal's ObjectStorageType -> (field name -> lifted function symbol)) for + // capture-free-or-not object-literal methods; see addObjectFuncFieldInfo's doc comment + // and docs/interface-vtable-simplification-design.md §3. Not namespace-scoped: an + // ObjectStorageType's symbol already embeds the literal's source location, so it's + // globally unique regardless of which namespace declared it. + mlir::DenseMap> objectLiteralMethodSymbolsMap; + + // fieldType is a method-as-field's FunctionType, whose first input is the object literal's + // own "this" type (ObjectType wrapping the ObjectStorageType key used in + // objectLiteralMethodSymbolsMap - see addObjectFuncFieldInfo). Returns the empty string if + // fieldType isn't a function, "this" isn't an ObjectType (e.g. a class instance, or an + // imported object type reconstructed from a @dllimport declaration with no local funcOp), + // or no method was ever registered for this exact (object, field) pair. + std::string lookupObjectLiteralMethodSymbol(mlir::Type fieldType, mlir::Attribute fieldId) + { + auto funcType = dyn_cast(fieldType); + if (!funcType || funcType.getInputs().empty()) + { + return {}; + } + + auto thisObjectType = dyn_cast(funcType.getInputs().front()); + if (!thisObjectType) + { + return {}; + } + + auto symbolsForObject = objectLiteralMethodSymbolsMap.find(thisObjectType.getStorageType()); + if (symbolsForObject == objectLiteralMethodSymbolsMap.end()) + { + return {}; + } + + auto symbolIt = symbolsForObject->second.find(fieldId); + return symbolIt == symbolsForObject->second.end() ? std::string{} : symbolIt->second; + } + llvm::ScopedHashTable fullNameGenericInterfacesMap; llvm::ScopedHashTable fullNameGlobalsMap; diff --git a/tslang/lib/TypeScript/MLIRGenInterfaces.cpp b/tslang/lib/TypeScript/MLIRGenInterfaces.cpp index a98dc73c5..b295a5b1c 100644 --- a/tslang/lib/TypeScript/MLIRGenInterfaces.cpp +++ b/tslang/lib/TypeScript/MLIRGenInterfaces.cpp @@ -212,6 +212,37 @@ namespace mlirgen return mlir::failure(); } + // methods this literal's own funcName is known for were already baked into + // globalVTableRefValue as constant SymbolRefOps by + // mlirGenObjectVirtualTableDefinitionForInterface (see there, and + // docs/interface-vtable-simplification-design.md §3) - only the rest (an + // imported object type reconstructed from a @dllimport declaration, with no + // local funcOp to name) still need their function pointer read out of the + // actual object `in` at cast time. + llvm::SmallVector methodsNeedingPatch; + for (auto& method : newInterfacePtr->methods) + { + auto fieldId = builder.getStringAttr(method.name); + auto index = mth.getFieldIndexByFieldName(storeType, fieldId); + if (index == -1) + { + return mlir::failure(); + } + + auto fieldInfo = mth.getFieldInfoByIndex(storeType, index); + if (lookupObjectLiteralMethodSymbol(fieldInfo.type, fieldId).empty()) + { + methodsNeedingPatch.push_back(&method); + } + } + + if (methodsNeedingPatch.empty()) + { + // every method slot is already a compile-time constant - no per-cast + // heap allocation needed, same footing as a method-less interface. + return globalVTableRefValue; + } + // match VTable // 1) clone vtable onto the GC heap (NOT a stack VariableOp): this per-object // patched vtable is pointed at by the resulting interface value, and that @@ -226,8 +257,9 @@ namespace mlirgen builder.create(location, valueVTable, heapVTable); auto varVTable = builder.create(location, globalVTableRefValue.getType(), heapVTable); - for (auto& method : newInterfacePtr->methods) + for (auto* methodPtr : methodsNeedingPatch) { + auto& method = *methodPtr; auto index = mth.getFieldIndexByFieldName(storeType, builder.getStringAttr(method.name)); if (index == -1) { @@ -246,13 +278,13 @@ namespace mlirgen auto methodRefVT = builder.create(location, fieldInfoVT.type, varVTable, method.virtualIndex); LLVM_DEBUG(llvm::dbgs() << "\n!!\n\t vtable method: " << method.name - << "\n\t vtable method ref: " << V(methodRefVT) << "\n\n";); - - builder.create(location, methodRefVT, methodRef); - } + << "\n\t vtable method ref: " << V(methodRefVT) << "\n\n";); + + builder.create(location, methodRefVT, methodRef); + } // patched VTable - return V(varVTable); + return V(varVTable); } return globalVTableRefValue; @@ -301,9 +333,56 @@ namespace mlirgen { if (methodOrField.isField) { - auto nullObj = builder.create(location, getNullType()); + // an object-literal method is resolved via the FIELD path too + // (methodsAsFields=true, getInterfaceVirtualTableForObject) since it's + // literally stored as a func-typed field on the object - but if it's + // capture-free-or-not-relevant (its funcName is compile-time constant + // regardless of captures, see addObjectFuncFieldInfo), we already know + // exactly which function it is and don't need to derive an object-relative + // offset for it at all: emit the function symbol directly, same as the + // class vtable path (mlirGenClassVirtualTableDefinitionForInterface) does + // for its own (isField=false) methods. This makes the slot a genuine + // compile-time constant, skipping mlirGenCreateInterfaceVTableForObject's + // per-cast heap-clone-and-patch entirely for such methods (see there). + // See docs/interface-vtable-simplification-design.md §3. + // + // Gate on the INTERFACE's own categorization, not merely "is this + // vtable entry func-typed": `methodsAsFields=true` makes every entry + // here isField=true regardless of whether the interface declared it as + // a MethodSignature (`inc(): void`) or a PropertySignature with a + // function type (`toString: () => string`) - only the former is read + // through InterfaceMethodAccess (BoundFunctionType access, raw funcptr + // slot semantics) at the access site; the latter goes through + // InterfaceFieldAccess, which computes thisVal + slotValue expecting an + // OFFSET. Substituting a raw function pointer into a + // PropertySignature-with-function-type's slot corrupts that + // computation (crashes on the resulting garbage address). + std::string methodFuncName; if (!methodOrField.isMissing) { + auto fieldNameAttr = dyn_cast(methodOrField.fieldInfo.id); + if (fieldNameAttr && newInterfacePtr->findMethod(fieldNameAttr.getValue())) + { + methodFuncName = lookupObjectLiteralMethodSymbol(methodOrField.fieldInfo.type, methodOrField.fieldInfo.id); + } + } + + if (!methodFuncName.empty()) + { + auto methodConstName = builder.create( + location, mlir::cast(methodOrField.fieldInfo.type), + mlir::FlatSymbolRefAttr::get(builder.getContext(), methodFuncName)); + auto methodConstNameRef = cast(location, mlir_ts::RefType::get(methodOrField.fieldInfo.type), + methodConstName, genContext); + + vtableValue = builder.create( + location, virtTuple, methodConstNameRef, vtableValue, + MLIRHelper::getStructIndex(builder, fieldIndex)); + } + else if (!methodOrField.isMissing) + { + auto nullObj = builder.create(location, getNullType()); + // TODO: test cast result auto objectNull = cast(location, objectType, nullObj, genContext, true); if (!objectNull) diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index 5f7fa107e..b21619ae3 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -285,6 +285,7 @@ add_test(NAME test-compile-00-interface-conjunction COMMAND test-runner "${PROJE add_test(NAME test-compile-00-interface-partial COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_partial.ts") add_test(NAME test-compile-00-interface-optional COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_optional.ts") add_test(NAME test-compile-00-interface-optional-cast-order COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_optional_cast_order.ts") +add_test(NAME test-compile-00-interface-function-typed-field COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_function_typed_field.ts") add_test(NAME test-compile-00-interface-generic COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_generic.ts") add_test(NAME test-compile-00-interface-new COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_new.ts") add_test(NAME test-compile-00-interface-indexer COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_indexer.ts") @@ -636,6 +637,7 @@ add_test(NAME test-jit-00-interface-conjunction COMMAND test-runner -jit "${PROJ add_test(NAME test-jit-00-interface-partial COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_partial.ts") add_test(NAME test-jit-00-interface-optional COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_optional.ts") add_test(NAME test-jit-00-interface-optional-cast-order COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_optional_cast_order.ts") +add_test(NAME test-jit-00-interface-function-typed-field COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_function_typed_field.ts") add_test(NAME test-jit-00-interface-generic COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_generic.ts") add_test(NAME test-jit-00-interface-new COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_new.ts") add_test(NAME test-jit-00-interface-indexer COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00interface_indexer.ts") diff --git a/tslang/test/tester/tests/00interface_function_typed_field.ts b/tslang/test/tester/tests/00interface_function_typed_field.ts new file mode 100644 index 000000000..d297d05ff --- /dev/null +++ b/tslang/test/tester/tests/00interface_function_typed_field.ts @@ -0,0 +1,33 @@ +// regression test: an interface member declared as a PropertySignature with a +// function type (`toString: () => string`) is categorized as a FIELD by the +// interface itself (InterfaceInfo::fields, not ::methods), even though the +// object literal implementing it stores it as a func-typed field internally +// (methodsAsFields, getInterfaceVirtualTableForObject). The access site +// (InterfaceFieldAccess) computes thisVal + slotValue expecting an OFFSET, +// unlike a real MethodSignature (`inc(): void`, InterfaceMethodAccess, raw +// function-pointer slot semantics). mlirGenObjectVirtualTableDefinitionForInterface +// must gate its constant-symbol vtable optimization (see +// docs/interface-vtable-simplification-design.md §3) on the interface's own +// method/field categorization, not merely "is this slot func-typed" - doing +// otherwise crashed the JIT with 0xC0000005 (call through a garbage address +// computed as thisVal + a raw function pointer misread as an offset). + +interface Formattable { + prefix: string; + toString: () => string; +} + +function main() { + const a = { + prefix: "A: ", + toString() { + return this.prefix + "hello"; + }, + }; + + const iface: Formattable = a; + print(iface.toString()); + assert(iface.toString() == "A: hello"); + + print("done."); +}