diff --git a/tag.bat b/tag.bat index 2e34272c4..888d4a705 100644 --- a/tag.bat +++ b/tag.bat @@ -1,2 +1,2 @@ -git tag -a v0.0-pre-alpha76 -m "pre alpha v0.0-76" +git tag -a v0.0-pre-alpha77 -m "pre alpha v0.0-77" git push origin --tags diff --git a/tag_del.bat b/tag_del.bat index 2c5be7fa4..c377cf5d8 100644 --- a/tag_del.bat +++ b/tag_del.bat @@ -1,2 +1,2 @@ -git push --delete origin v0.0-pre-alpha76 -git tag -d v0.0-pre-alpha76 +git push --delete origin v0.0-pre-alpha77 +git tag -d v0.0-pre-alpha77 diff --git a/tslang/include/TypeScript/TypeScriptOps.td b/tslang/include/TypeScript/TypeScriptOps.td index df2de387c..241dea6ba 100644 --- a/tslang/include/TypeScript/TypeScriptOps.td +++ b/tslang/include/TypeScript/TypeScriptOps.td @@ -1503,14 +1503,17 @@ def TypeScript_LogicalBinaryOp : TypeScript_Op<"LogicalBinary"> { let hasCanonicalizer = 1; } -def TypeScript_CastOp : TypeScript_Op<"Cast", [DeclareOpInterfaceMethods, Pure]> { +def TypeScript_CastOp : TypeScript_Op<"Cast", [ + DeclareOpInterfaceMethods, + DeclareOpInterfaceMethods +]> { let description = [{ Example: ts.cast %v : i32 to f16 }]; let arguments = (ins AnyType:$in); let results = (outs AnyType:$res); - + let hasCanonicalizer = 1; let hasVerifier = 1; @@ -1530,7 +1533,7 @@ def TypeScript_SafeCastOp : TypeScript_Op<"SafeCast", [ let hasFolder = 1; } -def TypeScript_BoxOp : TypeScript_Op<"Box", [Pure]> { +def TypeScript_BoxOp : TypeScript_Op<"Box", [DeclareOpInterfaceMethods]> { let description = [{ Example: %ti = ts.typeof %1 @@ -1538,7 +1541,7 @@ def TypeScript_BoxOp : TypeScript_Op<"Box", [Pure]> { }]; let arguments = (ins AnyType:$in, TypeScript_String:$typeInfo); let results = (outs TypeScript_Any:$res); - + } def TypeScript_UnboxOp : TypeScript_Op<"Unbox", [Pure]> { @@ -1551,6 +1554,11 @@ def TypeScript_UnboxOp : TypeScript_Op<"Unbox", [Pure]> { } +// Safe as Pure only because CreateUnionInstanceOpLowering calls CastLogicHelper::castLLVMTypes +// (bitcast/struct-GEP only) directly, never CastLogicHelper::cast() - the dispatcher whose +// castToArrayType branch is what made CastOp/BoxOp unsafely Pure (each called ch.MemoryAlloc +// under the hood). If this op's lowering is ever changed to route through cast() instead, +// re-audit whether it can still be Pure. def TypeScript_CreateUnionInstanceOp : TypeScript_Op<"CreateUnionInstance", [Pure]> { let description = [{ Example: diff --git a/tslang/lib/TypeScript/GCPass.cpp b/tslang/lib/TypeScript/GCPass.cpp index acb1efc09..723cfef8a 100644 --- a/tslang/lib/TypeScript/GCPass.cpp +++ b/tslang/lib/TypeScript/GCPass.cpp @@ -153,6 +153,47 @@ class GCPass : public mlir::PassWrapper } funcOp->setAttr(SymbolTable::getSymbolAttrName(), mlir::StringAttr::get(funcOp->getContext(), newName)); + + markAsAllocatorIfNeeded(newName, funcOp); + } + + // GC_malloc/GC_malloc_atomic/GC_memalign are fresh-pointer-per-call allocators, just like + // plain malloc. Unlike malloc, they aren't libc names LLVM's TargetLibraryInfo recognizes, + // so without explicit markings, GVN/EarlyCSE at -O3 see two calls with identical arguments + // (e.g. GC_malloc(0) for two different empty array fields) and no intervening memory + // clobber, and fold them into one shared allocation - aliasing fields that must stay + // distinct. memory_effects alone (writing unmodeled "other" memory) is not enough to stop + // GVN's call-CSE, which special-cases allocator-shaped functions via the `allockind` LLVM + // attribute (the same mechanism TargetLibraryInfo uses internally for malloc/calloc). Since + // GC_malloc isn't a recognized libc name, we must attach `allockind("alloc")` explicitly so + // each call is treated as returning a distinct, non-aliasing pointer. + void markAsAllocatorIfNeeded(StringRef newName, LLVM::LLVMFuncOp funcOp) + { + if (newName != "GC_malloc" && newName != "GC_malloc_atomic" && newName != "GC_memalign") + { + return; + } + + auto *context = funcOp->getContext(); + auto memoryEffects = LLVM::MemoryEffectsAttr::get(context, LLVM::ModRefInfo::Mod, LLVM::ModRefInfo::NoModRef, + LLVM::ModRefInfo::NoModRef, LLVM::ModRefInfo::NoModRef, + LLVM::ModRefInfo::NoModRef, LLVM::ModRefInfo::NoModRef); + funcOp.setMemoryEffectsAttr(memoryEffects); + + // AllocFnKind::Alloc = 1<<0, Zeroed = 1<<4. GC_malloc/GC_malloc_atomic zero-fill; + // GC_memalign (GC_memalign) does not guarantee zeroing, so only mark Alloc for it. + uint64_t allocKind = newName == "GC_memalign" ? /*Alloc*/ 1 : /*Alloc|Zeroed*/ 1 | (1 << 4); + auto kindEntry = mlir::ArrayAttr::get( + context, {mlir::StringAttr::get(context, "allockind"), + mlir::StringAttr::get(context, std::to_string(allocKind))}); + + llvm::SmallVector passthrough; + if (auto existing = funcOp.getPassthroughAttr()) + { + passthrough.append(existing.begin(), existing.end()); + } + passthrough.push_back(kindEntry); + funcOp.setPassthroughAttr(mlir::ArrayAttr::get(context, passthrough)); } void renameCall(StringRef name, LLVM::CallOp callOp) @@ -186,6 +227,7 @@ class GCPass : public mlir::PassWrapper LLVMCodeHelper ch(memSetCallOp, rewriter, nullptr, tsContext.compileOptions); auto i8PtrTy = th.getPtrType(); auto gcInitFuncOp = ch.getOrInsertFunction("GC_malloc_atomic", th.getFunctionType(th.getPtrType(), mlir::ArrayRef{th.getI64Type()})); + markAsAllocatorIfNeeded("GC_malloc_atomic", gcInitFuncOp); } void injectInit(LLVM::LLVMFuncOp funcOp) diff --git a/tslang/lib/TypeScript/LowerToLLVM.cpp b/tslang/lib/TypeScript/LowerToLLVM.cpp index 8e9c5d066..9494f50d2 100644 --- a/tslang/lib/TypeScript/LowerToLLVM.cpp +++ b/tslang/lib/TypeScript/LowerToLLVM.cpp @@ -5515,6 +5515,29 @@ class GCMakeDescriptorOpLowering : public TsLlvmPatterngetContext(); + + // AllocFnKind::Alloc = 1<<0; not marking Zeroed since GC_malloc_explicitly_typed's + // contents are then explicitly initialized by the caller. + auto kindEntry = mlir::ArrayAttr::get( + context, {mlir::StringAttr::get(context, "allockind"), mlir::StringAttr::get(context, "1")}); + + llvm::SmallVector passthrough; + if (auto existing = funcOp.getPassthroughAttr()) + { + passthrough.append(existing.begin(), existing.end()); + } + passthrough.push_back(kindEntry); + funcOp.setPassthroughAttr(mlir::ArrayAttr::get(context, passthrough)); +} + class GCNewExplicitlyTypedOpLowering : public TsLlvmPattern { public: @@ -5550,6 +5573,13 @@ class GCNewExplicitlyTypedOpLowering : public TsLlvmPattern(loc, gcMallocExplicitlyTypedFunc, ValueRange{sizeOfTypeValue, transformed.getTypeDescr()}); rewriter.replaceOpWithNewOp(op, resultType, value.getResult()); diff --git a/tslang/lib/TypeScript/TypeScriptOps.cpp b/tslang/lib/TypeScript/TypeScriptOps.cpp index c7b73eccd..29a9b59b9 100644 --- a/tslang/lib/TypeScript/TypeScriptOps.cpp +++ b/tslang/lib/TypeScript/TypeScriptOps.cpp @@ -14,6 +14,7 @@ #include "mlir/IR/PatternMatch.h" #include "mlir/IR/TypeUtilities.h" #include "mlir/Interfaces/FunctionImplementation.h" +#include "mlir/Interfaces/SideEffectInterfaces.h" #include "llvm/ADT/TypeSwitch.h" #include "llvm/ADT/MapVector.h" @@ -821,6 +822,40 @@ bool mlir_ts::CastOp::areCastCompatible(TypeRange inputs, TypeRange outputs) return true; } +// Casting a ConstArrayType (e.g. the `[]` empty-array literal sentinel) to a heap-backed +// ArrayType materializes a fresh GC allocation during lowering (see +// CastLogicHelper::castToArrayType's `byValue` branch, which calls MemoryAlloc). Reporting no +// effects here (as the previous blanket `Pure` trait did) let generic CSE - which runs on the +// `ts` dialect before this op is ever lowered to the actual allocation call - treat two such +// casts with the same (identical, CSE'd) constant operand as redundant and merge them into one, +// silently aliasing what should be two distinct backing arrays. All other CastOp shapes are true +// value-preserving casts with no allocation, so they keep reporting no effects. +void mlir_ts::CastOp::getEffects(SmallVectorImpl> &effects) +{ + if (isa(getIn().getType()) && isa(getRes().getType())) + { + auto result = cast(getRes()); + effects.emplace_back(MemoryEffects::Allocate::get(), result); + effects.emplace_back(MemoryEffects::Write::get(), result); + } +} + +//===----------------------------------------------------------------------===// +// BoxOp +//===----------------------------------------------------------------------===// + +// BoxOp's lowering (AnyLogic::castToAny) unconditionally allocates a fresh "any" container via +// MemoryAlloc on every execution - it is not a value-preserving reinterpretation like a real +// cast. Reporting it as Pure (as before) would let CSE merge two structurally-identical Box ops +// into one, aliasing what must be two distinct boxed values - the same bug fixed for CastOp +// above. Always report the effect: unlike CastOp, every BoxOp shape allocates. +void mlir_ts::BoxOp::getEffects(SmallVectorImpl> &effects) +{ + auto result = cast(getRes()); + effects.emplace_back(MemoryEffects::Allocate::get(), result); + effects.emplace_back(MemoryEffects::Write::get(), result); +} + //===----------------------------------------------------------------------===// // DialectCastOp //===----------------------------------------------------------------------===// diff --git a/tslang/test/tester/CMakeLists.txt b/tslang/test/tester/CMakeLists.txt index 080b49797..7ba8d94bb 100644 --- a/tslang/test/tester/CMakeLists.txt +++ b/tslang/test/tester/CMakeLists.txt @@ -442,6 +442,7 @@ add_test(NAME test-compile-load-store-decorators COMMAND test-runner "${PROJECT_ add_test(NAME test-compile-reference-null-bug COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00reference_null_bug.ts") add_test(NAME test-compile-reference-index-bug COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00reference_index_bug.ts") add_test(NAME test-compile-uint-compare-bug COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/00uint_compare_bug.ts") +add_test(NAME test-compile-gc-malloc-cse-o3 COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/gc_malloc_cse_o3.ts") add_test(NAME test-jit-00-print COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00print.ts") add_test(NAME test-jit-00-assert COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00assert.ts") @@ -772,6 +773,7 @@ add_test(NAME test-jit-load-store-decorators COMMAND test-runner -jit "${PROJECT add_test(NAME test-jit-reference-null-bug COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00reference_null_bug.ts") add_test(NAME test-jit-reference-index-bug COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00reference_index_bug.ts") add_test(NAME test-jit-uint-compare-bug COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/00uint_compare_bug.ts") +add_test(NAME test-jit-gc-malloc-cse-o3 COMMAND test-runner -jit "${PROJECT_SOURCE_DIR}/test/tester/tests/gc_malloc_cse_o3.ts") # tesing include add_test(NAME test-compile-include-global-var COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/tests/include_global_var.ts" "${PROJECT_SOURCE_DIR}/test/tester/tests/define_global_var.ts") diff --git a/tslang/test/tester/tests/gc_malloc_cse_o3.ts b/tslang/test/tester/tests/gc_malloc_cse_o3.ts new file mode 100644 index 000000000..2230cd369 --- /dev/null +++ b/tslang/test/tester/tests/gc_malloc_cse_o3.ts @@ -0,0 +1,49 @@ +// Regression test for: at -O3, two GC_malloc(0) calls with identical +// arguments (allocating two distinct empty array fields in the same +// constructor) were incorrectly CSE'd into a single shared allocation, +// aliasing the two fields. Growing one field's backing array then +// corrupted/orphaned the other field's pointer. +// +// Compile at --opt --opt_level=3 to exercise the bug; passes trivially +// at --opt_level=0. + +class TwoArrays { + a: string[]; + b: string[]; + + constructor() { + this.a = []; + this.b = []; + } + + addA(v: string) { + this.a.push(v); + } + + addB(v: string) { + this.b.push(v); + } +} + +function main() { + const t = new TwoArrays(); + + // Grow `a` several times; if `a` and `b` alias the same backing + // store (the bug), this corrupts `b`'s view of its own array. + t.addA("a1"); + t.addA("a2"); + t.addA("a3"); + t.addA("a4"); + + t.addB("b1"); + + assert(t.a.length == 4, "a.length should be 4"); + assert(t.b.length == 1, "b.length should be 1 (aliasing bug would corrupt this)"); + assert(t.a[0] == "a1"); + assert(t.a[3] == "a4"); + assert(t.b[0] == "b1"); + + print("done."); +} + +main();