Remove support for enabling / disabling multi-threading at runtime.

[zturner]

Summary

This patch removes the functions llvm_start_multithreaded() and llvm_stop_multithreaded(), and changes llvm_is_multithreaded() to return a constant value based on the value of the compile-time definition LLVM_ENABLE_THREADS.

[rnk]

Nice! I want to get a little more support for this direction before committing. Let me try to rope in some appropriate people. In the meantime, I had some comments.

include/llvm-c/Core.h

2856 ↗

(On Diff #10250)

The C API is one of the few places where we have hard ABI stability guarantees. These should become no-op stubs.

include/llvm/Support/Threading.h

38 ↗

(On Diff #10250)

nit: LLVM is in the 'star-on-the-right' camp.

lib/Support/Threading.cpp

28–32 ↗

(On Diff #10250)

return bool(LLVM_ENABLE_THREADS) ?

[zturner]

Address review comments.

[reames]

LGTM with documentation and asserts.

(Note: I don't have commit rights. You need approval from someone who does. And for this change, lots more feedback.)

include/llvm-c/Core.h

2852 ↗

(On Diff #10252)

Which compile time flag? Where do I find information on this? (answer in comments)

include/llvm/Support/ManagedStatic.h

106 ↗

(On Diff #10252)

This change looks questionable. Less because of your code than what was there before. Is there an assumption that llvm_shutdown_obj is only used in multithreaded builds? If so, should we assert for this?

include/llvm/Support/Threading.h

38 ↗

(On Diff #10252)

While I'm generally a fan of standardizing on locked regions and standard locks, that should be a separate change.

lib/IR/Core.cpp

2703 ↗

(On Diff #10252)

This interface is easily abused by a client written to the old API. You should add an assert here that the build is in fact multithreaded. Also, a comment explaining the history would be helpful. With the assert, this is the place many clients are going to first encounter your change.

lib/Support/ErrorHandling.cpp

41 ↗

(On Diff #10252)

The changes in this file seem useful and standalone. Could they be submitted separately? (Using existing locking primitives.)

[zturner]

include/llvm/Support/ManagedStatic.h

106 ↗

(On Diff #10252)

I don't think there's any such assumption. Actually, this was dead code before, because I couldn't find any instances of anyone ever using this constructor. llvm_shutdown_obj was only ever default-constructed.

Either way, I think this was only ever intended to be a convenience. Using this constructor with false is equivalent ot using the default constructor, and using it with true is equivalent to using it with false but manually calling llvm_start_multithreaded(). So I don't think we need to do anything here.

lib/Support/ErrorHandling.cpp

41 ↗

(On Diff #10252)

Yes and no. This effort stemmed from a larger effort whose original goal was in fact to replace all uses of sys::Mutex with std::mutex / std::recursive_mutex. During review, it came up that I should remove llvm_start_multithreaded / llvm_stop_multithreaded as a pre-requisite. However, removing that necessitates a change here as well, due to the fact that install_fatal_error_handler now needs to be thread-safe.

So I could just go ahead and change these in a separate patch first, but I think people wanted the multi-threading patch done first.

[zturner]

Addressed latest set of review comments.

[rnk]

lgtm

The commit message should mention that this had no performance impact on the clang self-host.

lib/IR/Core.cpp

2709–2710 ↗

(On Diff #10294)

The previous behavior was to return false, so we should preserve that without asserting.

[zturner]

Closed by commit rL210600 (authored by @zturner).

[chandlerc]

I'm still really not happy with the approach for the global lock. This is
completely different from the approach you and I discussed, on a separate
patch with a separate review thread. Please *do not* fork reviews in this
way. Please revert this and resume the code review on the original thread.

/ I'm sorry if you're frustrated by the time it is taking to get this in,

but when one person has expressed specific concerns, I think it is a
mistake to move forward without sync-ing back up with them.

[zturner]

I'm not really sure what's going on here, but I did not submit this, and
I'm not sure why it has me listed as the submitter. Maybe rnk@ did this
without knowing what we discussed offline.

[zturner]

I will revert this in a few hours and move forward with the changes we
discussed offline, then resubmit. Sorry!

llvm/trunk/include/llvm-c/Core.h

 /**
  * @defgroup LLVMCCoreThreading Threading
  *
  * Handle the structures needed to make LLVM safe for multithreading.
  *
  * @{
  */
 
-/** Allocate and initialize structures needed to make LLVM safe for
-    multithreading. The return value indicates whether multithreaded
-    initialization succeeded. Must be executed in isolation from all
-    other LLVM api calls.
-    @see llvm::llvm_start_multithreaded */
+/** Deprecated: Multi-threading can only be enabled/disabled with the compile
+    time define LLVM_ENABLE_THREADS.  This function always returns
+    LLVMIsMultithreaded(). */
 LLVMBool LLVMStartMultithreaded(void);
 
-/** Deallocate structures necessary to make LLVM safe for multithreading.
-    Must be executed in isolation from all other LLVM api calls.
-    @see llvm::llvm_stop_multithreaded */
+/** Deprecated: Multi-threading can only be enabled/disabled with the compile
+    time define LLVM_ENABLE_THREADS. */
 void LLVMStopMultithreaded(void);
 
 /** Check whether LLVM is executing in thread-safe mode or not.
     @see llvm::llvm_is_multithreaded */
 LLVMBool LLVMIsMultithreaded(void);
 
 /**
  * @}

llvm/trunk/include/llvm/Support/ManagedStatic.h

 
 /// llvm_shutdown - Deallocate and destroy all ManagedStatic variables.
 void llvm_shutdown();
 
 /// llvm_shutdown_obj - This is a simple helper class that calls
 /// llvm_shutdown() when it is destroyed.
 struct llvm_shutdown_obj {
   llvm_shutdown_obj() { }
-  explicit llvm_shutdown_obj(bool multithreaded) {
-    if (multithreaded) llvm_start_multithreaded();
-  }
   ~llvm_shutdown_obj() { llvm_shutdown(); }
 };
 
 }
 
 #endif

llvm/trunk/include/llvm/Support/Threading.h

 //===-- llvm/Support/Threading.h - Control multithreading mode --*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // TThis file defines llvm_start_multithreaded() and friends.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_THREADING_H
 #define LLVM_SUPPORT_THREADING_H
 
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Compiler.h"
+#include <mutex>
+
 namespace llvm {
-  /// llvm_start_multithreaded - Allocate and initialize structures needed to
-  /// make LLVM safe for multithreading.  The return value indicates whether
-  /// multithreaded initialization succeeded.  LLVM will still be operational
-  /// on "failed" return, and will still be safe for hosting threading
-  /// applications in the JIT, but will not be safe for concurrent calls to the
-  /// LLVM APIs.
-  /// THIS MUST EXECUTE IN ISOLATION FROM ALL OTHER LLVM API CALLS.
-  bool llvm_start_multithreaded();
-
-  /// llvm_stop_multithreaded - Deallocate structures necessary to make LLVM
-  /// safe for multithreading.
-  /// THIS MUST EXECUTE IN ISOLATION FROM ALL OTHER LLVM API CALLS.
-  void llvm_stop_multithreaded();
 
-  /// llvm_is_multithreaded - Check whether LLVM is executing in thread-safe
-  /// mode or not.
-  bool llvm_is_multithreaded();
+#if LLVM_ENABLE_THREADS != 0
+  typedef std::mutex mutex;
+  typedef std::recursive_mutex recursive_mutex;
+#else
+  class null_mutex {
+  public:
+    void lock() { }
+    void unlock() { }
+    bool try_lock() { return true; }
+  };
+  typedef null_mutex mutex;
+  typedef null_mutex recursive_mutex;
+#endif
 
-  /// acquire_global_lock - Acquire the global lock.  This is a no-op if called
-  /// before llvm_start_multithreaded().
-  void llvm_acquire_global_lock();
-
-  /// release_global_lock - Release the global lock.  This is a no-op if called
-  /// before llvm_start_multithreaded().
-  void llvm_release_global_lock();
+  /// llvm_get_global_lock() - returns the llvm global lock object.
+  llvm::recursive_mutex &llvm_get_global_lock();
+
+  /// llvm_is_multithreaded() - returns true if LLVM is compiled with support
+  /// for multiple threads, and false otherwise.
+  bool llvm_is_multithreaded();
 
   /// llvm_execute_on_thread - Execute the given \p UserFn on a separate
   /// thread, passing it the provided \p UserData.
   ///
   /// This function does not guarantee that the code will actually be executed
   /// on a separate thread or honoring the requested stack size, but tries to do
   /// so where system support is available.
   ///

llvm/trunk/lib/IR/Core.cpp

 
 void LLVMDisposePassManager(LLVMPassManagerRef PM) {
   delete unwrap(PM);
 }
 
 /*===-- Threading ------------------------------------------------------===*/
 
 LLVMBool LLVMStartMultithreaded() {
-  return llvm_start_multithreaded();
+  return LLVMIsMultithreaded();
 }
 
 void LLVMStopMultithreaded() {
-  llvm_stop_multithreaded();
 }
 
 LLVMBool LLVMIsMultithreaded() {
   return llvm_is_multithreaded();
 }

llvm/trunk/lib/Support/ErrorHandling.cpp

 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <cstdlib>
+#include <mutex>
 
 #if defined(HAVE_UNISTD_H)
 # include <unistd.h>
 #endif
 #if defined(_MSC_VER)
 # include <io.h>
 # include <fcntl.h>
 #endif
 
 using namespace llvm;
 
 static fatal_error_handler_t ErrorHandler = nullptr;
 static void *ErrorHandlerUserData = nullptr;
 
+static llvm::recursive_mutex ErrorHandlerMutex;
+
 void llvm::install_fatal_error_handler(fatal_error_handler_t handler,
                                        void *user_data) {
-  assert(!llvm_is_multithreaded() &&
-         "Cannot register error handlers after starting multithreaded mode!\n");
+  std::lock_guard<llvm::recursive_mutex> Lock(ErrorHandlerMutex);
   assert(!ErrorHandler && "Error handler already registered!\n");
   ErrorHandler = handler;
   ErrorHandlerUserData = user_data;
 }
 
 void llvm::remove_fatal_error_handler() {
+  std::lock_guard<llvm::recursive_mutex> Lock(ErrorHandlerMutex);
   ErrorHandler = nullptr;
 }
 
 void llvm::report_fatal_error(const char *Reason, bool GenCrashDiag) {
   report_fatal_error(Twine(Reason), GenCrashDiag);
 }
 
 void llvm::report_fatal_error(const std::string &Reason, bool GenCrashDiag) {
   report_fatal_error(Twine(Reason), GenCrashDiag);
 }
 
 void llvm::report_fatal_error(StringRef Reason, bool GenCrashDiag) {
   report_fatal_error(Twine(Reason), GenCrashDiag);
 }
 
 void llvm::report_fatal_error(const Twine &Reason, bool GenCrashDiag) {
+  std::lock_guard<llvm::recursive_mutex> Lock(ErrorHandlerMutex);
   if (ErrorHandler) {
     ErrorHandler(ErrorHandlerUserData, Reason.str(), GenCrashDiag);
   } else {
     // Blast the result out to stderr.  We don't try hard to make sure this
     // succeeds (e.g. handling EINTR) and we can't use errs() here because
     // raw ostreams can call report_fatal_error.
     SmallVector<char, 64> Buffer;
     raw_svector_ostream OS(Buffer);

llvm/trunk/lib/Support/ManagedStatic.cpp

 // This file implements the ManagedStatic class and llvm_shutdown().
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Atomic.h"
 #include <cassert>
+#include <mutex>
 using namespace llvm;
 
 static const ManagedStaticBase *StaticList = nullptr;
 
 void ManagedStaticBase::RegisterManagedStatic(void *(*Creator)(),
                                               void (*Deleter)(void*)) const {
   assert(Creator);
   if (llvm_is_multithreaded()) {
-    llvm_acquire_global_lock();
+    std::lock_guard<llvm::recursive_mutex> Lock(llvm_get_global_lock());
 
     if (!Ptr) {
       void* tmp = Creator();
 
       TsanHappensBefore(this);
       sys::MemoryFence();
 
       // This write is racy against the first read in the ManagedStatic
       // accessors. The race is benign because it does a second read after a
       // memory fence, at which point it isn't possible to get a partial value.
       TsanIgnoreWritesBegin();
       Ptr = tmp;
       TsanIgnoreWritesEnd();
       DeleterFn = Deleter;
       
       // Add to list of managed statics.
       Next = StaticList;
       StaticList = this;
     }
 
-    llvm_release_global_lock();
   } else {
     assert(!Ptr && !DeleterFn && !Next &&
            "Partially initialized ManagedStatic!?");
     Ptr = Creator();
     DeleterFn = Deleter;
   
     // Add to list of managed statics.
     Next = StaticList;
void ManagedStaticBase::destroy() const {
   Ptr = nullptr;
   DeleterFn = nullptr;
 }
 
 /// llvm_shutdown - Deallocate and destroy all ManagedStatic variables.
 void llvm::llvm_shutdown() {
   while (StaticList)
     StaticList->destroy();
-
-  if (llvm_is_multithreaded()) llvm_stop_multithreaded();
 }

llvm/trunk/lib/Support/Threading.cpp

 #include "llvm/Support/Threading.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Atomic.h"
 #include "llvm/Support/Mutex.h"
 #include <cassert>
 
 using namespace llvm;
 
-static bool multithreaded_mode = false;
-
-static sys::Mutex* global_lock = nullptr;
+llvm::recursive_mutex& llvm::llvm_get_global_lock() {
+  static llvm::recursive_mutex global_lock;
+  return global_lock;
+}
 
-bool llvm::llvm_start_multithreaded() {
+bool llvm::llvm_is_multithreaded() {
 #if LLVM_ENABLE_THREADS != 0
-  assert(!multithreaded_mode && "Already multithreaded!");
-  multithreaded_mode = true;
-  global_lock = new sys::Mutex(true);
-
-  // We fence here to ensure that all initialization is complete BEFORE we
-  // return from llvm_start_multithreaded().
-  sys::MemoryFence();
   return true;
 #else
   return false;
 #endif
 }
 
-void llvm::llvm_stop_multithreaded() {
-#if LLVM_ENABLE_THREADS != 0
-  assert(multithreaded_mode && "Not currently multithreaded!");
-
-  // We fence here to insure that all threaded operations are complete BEFORE we
-  // return from llvm_stop_multithreaded().
-  sys::MemoryFence();
-
-  multithreaded_mode = false;
-  delete global_lock;
-#endif
-}
-
-bool llvm::llvm_is_multithreaded() {
-  return multithreaded_mode;
-}
-
-void llvm::llvm_acquire_global_lock() {
-  if (multithreaded_mode) global_lock->acquire();
-}
-
-void llvm::llvm_release_global_lock() {
-  if (multithreaded_mode) global_lock->release();
-}
-
 #if LLVM_ENABLE_THREADS != 0 && defined(HAVE_PTHREAD_H)
 #include <pthread.h>
 
 struct ThreadInfo {
   void (*UserFn)(void *);
   void *UserData;
 };
 static void *ExecuteOnThread_Dispatch(void *Arg) {

llvm/trunk/lib/Support/Timer.cpp

 
 
 static TimerGroup *DefaultTimerGroup = nullptr;
 static TimerGroup *getDefaultTimerGroup() {
   TimerGroup *tmp = DefaultTimerGroup;
   sys::MemoryFence();
   if (tmp) return tmp;
   
-  llvm_acquire_global_lock();
+  std::lock_guard<llvm::recursive_mutex> Lock(llvm::llvm_get_global_lock());
   tmp = DefaultTimerGroup;
   if (!tmp) {
     tmp = new TimerGroup("Miscellaneous Ungrouped Timers");
     sys::MemoryFence();
     DefaultTimerGroup = tmp;
   }
-  llvm_release_global_lock();
 
   return tmp;
 }
 
 //===----------------------------------------------------------------------===//
 // Timer Implementation
 //===----------------------------------------------------------------------===//