Author: Emilio Cota Date: 2025-03-25T21:35:52Z New Revision: 9c438311e573094c76c2b2d2425d625e068f841a
URL: https://github.com/llvm/llvm-project/commit/9c438311e573094c76c2b2d2425d625e068f841a DIFF: https://github.com/llvm/llvm-project/commit/9c438311e573094c76c2b2d2425d625e068f841a.diff LOG: Revert "[mlir] Fix DistinctAttributeUniquer deleting attribute storage when c…" This reverts commit 0aa5ba43a0d11ce8e7f143380ae75fea516b6841. Added: Modified: mlir/include/mlir/IR/MLIRContext.h mlir/lib/IR/AttributeDetail.h mlir/lib/IR/MLIRContext.cpp mlir/lib/Pass/PassCrashRecovery.cpp Removed: mlir/test/Dialect/LLVMIR/add-debuginfo-func-scope-with-crash-reproduction.mlir mlir/test/IR/test-builtin-distinct-attrs-with-crash-reproduction.mlir ################################################################################ diff --git a/mlir/include/mlir/IR/MLIRContext.h b/mlir/include/mlir/IR/MLIRContext.h index ad89d631c8a5f..ef8dab87f131a 100644 --- a/mlir/include/mlir/IR/MLIRContext.h +++ b/mlir/include/mlir/IR/MLIRContext.h @@ -153,14 +153,6 @@ class MLIRContext { disableMultithreading(!enable); } - /// Set the flag specifying if thread-local storage should be used by storage - /// allocators in this context. Note that disabling mutlithreading implies - /// thread-local storage is also disabled. - void disableThreadLocalStorage(bool disable = true); - void enableThreadLocalStorage(bool enable = true) { - disableThreadLocalStorage(!enable); - } - /// Set a new thread pool to be used in this context. This method requires /// that multithreading is disabled for this context prior to the call. This /// allows to share a thread pool across multiple contexts, as well as diff --git a/mlir/lib/IR/AttributeDetail.h b/mlir/lib/IR/AttributeDetail.h index 8fed18140433c..26d40ac3a38f6 100644 --- a/mlir/lib/IR/AttributeDetail.h +++ b/mlir/lib/IR/AttributeDetail.h @@ -24,7 +24,6 @@ #include "llvm/ADT/APFloat.h" #include "llvm/ADT/PointerIntPair.h" #include "llvm/Support/TrailingObjects.h" -#include <mutex> namespace mlir { namespace detail { @@ -402,8 +401,7 @@ class DistinctAttributeUniquer { /// is freed after the destruction of the distinct attribute allocator. class DistinctAttributeAllocator { public: - DistinctAttributeAllocator(bool threadingIsEnabled) - : threadingIsEnabled(threadingIsEnabled), useThreadLocalAllocator(true) {}; + DistinctAttributeAllocator() = default; DistinctAttributeAllocator(DistinctAttributeAllocator &&) = delete; DistinctAttributeAllocator(const DistinctAttributeAllocator &) = delete; @@ -413,49 +411,12 @@ class DistinctAttributeAllocator { /// Allocates a distinct attribute storage using a thread local bump pointer /// allocator to enable synchronization free parallel allocations. DistinctAttrStorage *allocate(Attribute referencedAttr) { - if (!useThreadLocalAllocator && threadingIsEnabled) { - std::scoped_lock<std::mutex> lock(allocatorMutex); - return allocateImpl(referencedAttr); - } - return allocateImpl(referencedAttr); - } - - /// Sets a flag that stores if multithreading is enabled. The flag is used to - /// decide if locking is needed when using a non thread-safe allocator. - void disableMultiThreading(bool disable = true) { - threadingIsEnabled = !disable; - } - - /// Sets a flag to disable using thread local bump pointer allocators and use - /// a single thread-safe allocator. Use this to persist allocated storage - /// beyond the lifetime of a child thread calling this function while ensuring - /// thread-safe allocation. - void disableThreadLocalStorage(bool disable = true) { - useThreadLocalAllocator = !disable; - } - -private: - DistinctAttrStorage *allocateImpl(Attribute referencedAttr) { - return new (getAllocatorInUse().Allocate<DistinctAttrStorage>()) + return new (allocatorCache.get().Allocate<DistinctAttrStorage>()) DistinctAttrStorage(referencedAttr); } - /// If threading is disabled on the owning MLIR context, a normal non - /// thread-local, non-thread safe bump pointer allocator is used instead to - /// prevent use-after-free errors whenever attribute storage created on a - /// crash recover thread is accessed after the thread joins. - llvm::BumpPtrAllocator &getAllocatorInUse() { - if (useThreadLocalAllocator) - return allocatorCache.get(); - return allocator; - } - +private: ThreadLocalCache<llvm::BumpPtrAllocator> allocatorCache; - llvm::BumpPtrAllocator allocator; - std::mutex allocatorMutex; - - bool threadingIsEnabled : 1; - bool useThreadLocalAllocator : 1; }; } // namespace detail } // namespace mlir diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp index ab6a5ac8b76e8..87782e84dd6e4 100644 --- a/mlir/lib/IR/MLIRContext.cpp +++ b/mlir/lib/IR/MLIRContext.cpp @@ -268,8 +268,7 @@ class MLIRContextImpl { public: MLIRContextImpl(bool threadingIsEnabled) - : threadingIsEnabled(threadingIsEnabled), - distinctAttributeAllocator(threadingIsEnabled) { + : threadingIsEnabled(threadingIsEnabled) { if (threadingIsEnabled) { ownedThreadPool = std::make_unique<llvm::DefaultThreadPool>(); threadPool = ownedThreadPool.get(); @@ -597,7 +596,6 @@ void MLIRContext::disableMultithreading(bool disable) { // Update the threading mode for each of the uniquers. impl->affineUniquer.disableMultithreading(disable); impl->attributeUniquer.disableMultithreading(disable); - impl->distinctAttributeAllocator.disableMultiThreading(disable); impl->typeUniquer.disableMultithreading(disable); // Destroy thread pool (stop all threads) if it is no longer needed, or create @@ -719,10 +717,6 @@ bool MLIRContext::isOperationRegistered(StringRef name) { return RegisteredOperationName::lookup(name, this).has_value(); } -void MLIRContext::disableThreadLocalStorage(bool disable) { - getImpl().distinctAttributeAllocator.disableThreadLocalStorage(disable); -} - void Dialect::addType(TypeID typeID, AbstractType &&typeInfo) { auto &impl = context->getImpl(); assert(impl.multiThreadedExecutionContext == 0 && diff --git a/mlir/lib/Pass/PassCrashRecovery.cpp b/mlir/lib/Pass/PassCrashRecovery.cpp index 7ea2a57693217..8c6d865cb31dd 100644 --- a/mlir/lib/Pass/PassCrashRecovery.cpp +++ b/mlir/lib/Pass/PassCrashRecovery.cpp @@ -414,15 +414,6 @@ struct FileReproducerStream : public mlir::ReproducerStream { LogicalResult PassManager::runWithCrashRecovery(Operation *op, AnalysisManager am) { - // Notify the context to disable the use of thread-local storage while the - // pass manager is running in a crash recovery context thread. Re-enable the - // thread local storage upon function exit. This is required to persist any - // attribute storage allocated during passes beyond the lifetime of the - // recovery context thread. - MLIRContext *ctx = getContext(); - ctx->disableThreadLocalStorage(); - auto guard = - llvm::make_scope_exit([ctx]() { ctx->enableThreadLocalStorage(); }); crashReproGenerator->initialize(getPasses(), op, verifyPasses); // Safely invoke the passes within a recovery context. diff --git a/mlir/test/Dialect/LLVMIR/add-debuginfo-func-scope-with-crash-reproduction.mlir b/mlir/test/Dialect/LLVMIR/add-debuginfo-func-scope-with-crash-reproduction.mlir deleted file mode 100644 index b36ed367adb76..0000000000000 --- a/mlir/test/Dialect/LLVMIR/add-debuginfo-func-scope-with-crash-reproduction.mlir +++ /dev/null @@ -1,22 +0,0 @@ -// Test that the enable-debug-info-scope-on-llvm-func pass can create its -// distinct attributes when running in the crash reproducer thread. - -// RUN: mlir-opt --mlir-disable-threading --mlir-pass-pipeline-crash-reproducer=. \ -// RUN: --pass-pipeline="builtin.module(ensure-debug-info-scope-on-llvm-func)" \ -// RUN: --mlir-print-debuginfo %s | FileCheck %s - -// RUN: mlir-opt --mlir-pass-pipeline-crash-reproducer=. \ -// RUN: --pass-pipeline="builtin.module(ensure-debug-info-scope-on-llvm-func)" \ -// RUN: --mlir-print-debuginfo %s | FileCheck %s - -module { - llvm.func @func_no_debug() { - llvm.return loc(unknown) - } loc(unknown) -} loc(unknown) - -// CHECK-LABEL: llvm.func @func_no_debug() -// CHECK: llvm.return loc(#loc -// CHECK: loc(#loc[[LOC:[0-9]+]]) -// CHECK: #di_compile_unit = #llvm.di_compile_unit<id = distinct[{{.*}}]<>, -// CHECK: #di_subprogram = #llvm.di_subprogram<id = distinct[{{.*}}]<> diff --git a/mlir/test/IR/test-builtin-distinct-attrs-with-crash-reproduction.mlir b/mlir/test/IR/test-builtin-distinct-attrs-with-crash-reproduction.mlir deleted file mode 100644 index af6dead31e263..0000000000000 --- a/mlir/test/IR/test-builtin-distinct-attrs-with-crash-reproduction.mlir +++ /dev/null @@ -1,18 +0,0 @@ -// This test verifies that when running with crash reproduction enabled, distinct -// attribute storage is not allocated in thread-local storage. Since crash -// reproduction runs the pass manager in a separate thread, using thread-local -// storage for distinct attributes causes use-after-free errors once the thread -// that runs the pass manager joins. - -// RUN: mlir-opt --mlir-disable-threading --mlir-pass-pipeline-crash-reproducer=. %s -test-distinct-attrs | FileCheck %s -// RUN: mlir-opt --mlir-pass-pipeline-crash-reproducer=. %s -test-distinct-attrs | FileCheck %s - -// CHECK: #[[DIST0:.*]] = distinct[0]<42 : i32> -// CHECK: #[[DIST1:.*]] = distinct[1]<42 : i32> -#distinct = distinct[0]<42 : i32> - -// CHECK: @foo_1 -func.func @foo_1() { - // CHECK: "test.op"() {distinct.input = #[[DIST0]], distinct.output = #[[DIST1]]} - "test.op"() {distinct.input = #distinct} : () -> () -} _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
