Subzero: Add locking to prepare for multithreaded translation.

src/IceGlobalContext.h

Index: src/IceGlobalContext.h
diff --git a/src/IceGlobalContext.h b/src/IceGlobalContext.h
index cf2478fcb5c7d013401c67dace21b06432178592..a81abaed98dc439110b6b89ff32a4545b00bd62e 100644
--- a/src/IceGlobalContext.h
+++ b/src/IceGlobalContext.h
@@ -16,6 +16,7 @@
 #define SUBZERO_SRC_ICEGLOBALCONTEXT_H
 
 #include <memory>
+#include <mutex>
 
 #include "IceDefs.h"
 #include "IceClFlags.h"
@@ -29,37 +30,48 @@ namespace Ice {
 class ClFlags;
 class FuncSigType;
 
-// This class collects rudimentary statistics during translation.
-class CodeStats {
-  CodeStats(const CodeStats &) = delete;
-  CodeStats &operator=(const CodeStats &) = default;
+class GlobalContext {
+  GlobalContext(const GlobalContext &) = delete;
+  GlobalContext &operator=(const GlobalContext &) = delete;
 
-public:
-  CodeStats()
+  // CodeStats collects rudimentary statistics during translation.
+  class CodeStats {
+    CodeStats(const CodeStats &) = delete;
+    CodeStats &operator=(const CodeStats &) = default;
+
+  public:
+    CodeStats()
       : InstructionsEmitted(0), RegistersSaved(0), FrameBytes(0), Spills(0),
         Fills(0) {}
-  void reset() { *this = CodeStats(); }
-  void updateEmitted(uint32_t InstCount) { InstructionsEmitted += InstCount; }
-  void updateRegistersSaved(uint32_t Num) { RegistersSaved += Num; }
-  void updateFrameBytes(uint32_t Bytes) { FrameBytes += Bytes; }
-  void updateSpills() { ++Spills; }
-  void updateFills() { ++Fills; }
-  void dump(const IceString &Name, Ostream &Str);
+    void reset() { *this = CodeStats(); }
+    void updateEmitted(uint32_t InstCount) { InstructionsEmitted += InstCount; }
+    void updateRegistersSaved(uint32_t Num) { RegistersSaved += Num; }
+    void updateFrameBytes(uint32_t Bytes) { FrameBytes += Bytes; }
+    void updateSpills() { ++Spills; }
+    void updateFills() { ++Fills; }
+    void dump(const IceString &Name, Ostream &Str);
+
+  private:
+    uint32_t InstructionsEmitted;
+    uint32_t RegistersSaved;
+    uint32_t FrameBytes;
+    uint32_t Spills;
+    uint32_t Fills;
+  };
 
-private:
-  uint32_t InstructionsEmitted;
-  uint32_t RegistersSaved;
-  uint32_t FrameBytes;
-  uint32_t Spills;
-  uint32_t Fills;
-};
+  // ThreadContext contains thread-local data.  This data can be
+  // combined/reduced as needed after all threads complete.
+  class ThreadContext {
+    ThreadContext(const ThreadContext &) = delete;
+    ThreadContext &operator=(const ThreadContext &) = delete;
+  public:
+    ThreadContext() {}
+    CodeStats StatsFunction;
+    std::vector<TimerStack> Timers;
+  };
 
-// TODO: Accesses to all non-const fields of GlobalContext need to
-// be synchronized, especially the constant pool, the allocator, and
-// the output streams.
-class GlobalContext {
-  GlobalContext(const GlobalContext &) = delete;
-  GlobalContext &operator=(const GlobalContext &) = delete;
+  typedef std::recursive_mutex GlobalLockType;
+  typedef std::recursive_mutex StrLockType;

[JF, 2015/01/15 01:31:01]

Why recursive? I'd leave a TODO here to remove the heavy stuff and just rely on
tsan.

[Jim Stichnoth, 2015/01/15 07:16:29]

(I moved this typedef down to where it's actually used, to keep all the stream
locking documentation together.)

I want to be able to enforce a discipline where a thread grabs the stream lock,
then does a whole bunch of work where far-off callees grab the stream and do
something with it, and finally the thread releases the stream lock.  In that
sense, the mutex should be non-recursive.  When someone grabs the stream, I want
to validate that they are already holding the stream lock.  The easiest way I
could think to do that is with the bool IsStrLocked field.  But IsStrLocked can
only be safely read while the mutex is held, so for safety the check ends up
being made while the mutex is recursively locked twice.

I added a big comment to try to explain this.

I plan to use tsan in any case, but since these streams are so integral to
development yet so unused in production, I'd prefer the developer to get
immediate feedback on stream locking protocol violations.

 
 public:
   GlobalContext(Ostream *OsDump, Ostream *OsEmit, ELFStreamer *ELFStreamer,
@@ -76,8 +88,38 @@ public:
   void addVerbose(VerboseMask Mask) { VMask |= Mask; }
   void subVerbose(VerboseMask Mask) { VMask &= ~Mask; }
 
-  Ostream &getStrDump() { return *StrDump; }
-  Ostream &getStrEmit() { return *StrEmit; }
+  // The dump and emit streams need to be used by only one thread at a
+  // time.  This is done by exclusively reserving the streams via
+  // lockStr() and unlockStr().  The OstreamLocker class can be used
+  // to conveniently manage this.
+  void lockStr() {
+    StrLock.lock();
+    assert(!IsStrLocked);
+    IsStrLocked = true;
+  }
+  void unlockStr() {
+    assert(IsStrLocked);
+    IsStrLocked = false;
+    StrLock.unlock();
+  }
+  // Test whether we are already holding StrLock, by doing a
+  // try_lock() and if it succeeds, checking that we didn't
+  // recursively lock it.
+  bool isStrLocked() {
+    if (!StrLock.try_lock())
+      return false;
+    bool WasLocked = IsStrLocked;
+    StrLock.unlock();
+    return WasLocked;
+  }
+  Ostream &getStrDump() {
+    assert(isStrLocked());
+    return *StrDump;
+  }
+  Ostream &getStrEmit() {
+    assert(isStrLocked());
+    return *StrEmit;
+  }
 
   TargetArch getTargetArch() const { return Arch; }
   OptLevel getOptLevel() const { return Opt; }
@@ -109,7 +151,7 @@ public:
   Constant *getConstantZero(Type Ty);
   // getConstantPool() returns a copy of the constant pool for
   // constants of a given type.
-  ConstantList getConstantPool(Type Ty) const;
+  ConstantList getConstantPool(Type Ty);
   // Returns a new function declaration, allocated in an internal
   // memory pool.  Ownership of the function is maintained by this
   // class instance.
@@ -129,7 +171,10 @@ public:
   }
 
   // Allocate data of type T using the global allocator.
-  template <typename T> T *allocate() { return Allocator.Allocate<T>(); }
+  template <typename T> T *allocate() {
+    std::lock_guard<GlobalLockType> L(GlobalLock);
+    return Allocator.Allocate<T>();
+  }
 
   const Intrinsics &getIntrinsicsInfo() const { return IntrinsicsInfo; }
 
@@ -142,37 +187,42 @@ public:
   // Reset stats at the beginning of a function.
   void resetStats() {
     if (ALLOW_DUMP)
-      StatsFunction.reset();
+      TLS->StatsFunction.reset();
   }
   void dumpStats(const IceString &Name, bool Final = false);
   void statsUpdateEmitted(uint32_t InstCount) {
-    if (!ALLOW_DUMP)
+    if (!ALLOW_DUMP || !getFlags().DumpStats)
       return;
-    StatsFunction.updateEmitted(InstCount);
+    TLS->StatsFunction.updateEmitted(InstCount);
+    std::lock_guard<GlobalLockType> L(GlobalLock);
     StatsCumulative.updateEmitted(InstCount);
   }
   void statsUpdateRegistersSaved(uint32_t Num) {
-    if (!ALLOW_DUMP)
+    if (!ALLOW_DUMP || !getFlags().DumpStats)
       return;
-    StatsFunction.updateRegistersSaved(Num);
+    TLS->StatsFunction.updateRegistersSaved(Num);
+    std::lock_guard<GlobalLockType> L(GlobalLock);
     StatsCumulative.updateRegistersSaved(Num);
   }
   void statsUpdateFrameBytes(uint32_t Bytes) {
-    if (!ALLOW_DUMP)
+    if (!ALLOW_DUMP || !getFlags().DumpStats)
       return;
-    StatsFunction.updateFrameBytes(Bytes);
+    TLS->StatsFunction.updateFrameBytes(Bytes);
+    std::lock_guard<GlobalLockType> L(GlobalLock);
     StatsCumulative.updateFrameBytes(Bytes);
   }
   void statsUpdateSpills() {
-    if (!ALLOW_DUMP)
+    if (!ALLOW_DUMP || !getFlags().DumpStats)
       return;
-    StatsFunction.updateSpills();
+    TLS->StatsFunction.updateSpills();
+    std::lock_guard<GlobalLockType> L(GlobalLock);
     StatsCumulative.updateSpills();
   }
   void statsUpdateFills() {
-    if (!ALLOW_DUMP)
+    if (!ALLOW_DUMP || !getFlags().DumpStats)
       return;
-    StatsFunction.updateFills();
+    TLS->StatsFunction.updateFills();
+    std::lock_guard<GlobalLockType> L(GlobalLock);
     StatsCumulative.updateFills();
   }
 
@@ -183,8 +233,8 @@ public:
     TSK_Num
   };
 
-  TimerIdT getTimerID(TimerStackIdT StackID, const IceString &Name);
   TimerStackIdT newTimerStackID(const IceString &Name);
+  TimerIdT getTimerID(TimerStackIdT StackID, const IceString &Name);
   void pushTimer(TimerIdT ID, TimerStackIdT StackID = TSK_Default);
   void popTimer(TimerIdT ID, TimerStackIdT StackID = TSK_Default);
   void resetTimer(TimerStackIdT StackID);
@@ -193,6 +243,15 @@ public:
                   bool DumpCumulative = true);
 
 private:
+  // GlobalLock is the default coarse-grain lock for accessing members
+  // of GlobalContext.  As contention becomes an issue, more
+  // fine-grain locks can be added.
+  GlobalLockType GlobalLock;
+  // StrLock is a global lock on the dump and emit output streams.
+  // IsStrLocked is used to validate the locking protocol.
+  StrLockType StrLock;
+  bool IsStrLocked;

[JF, 2015/01/15 01:31:01]

If the intent is to detect races then IsStrLocked should be atomic.

[Jim Stichnoth, 2015/01/15 07:16:29]

I changed it so that IsStrLocked is only read or written while StrLock is held. 
I.e., StrLock guards IsStrLocked, StrDump, and StrEmit.  I think it should be
thread-safe now, but please check...

[JF, 2015/01/15 16:28:39]

The goal of IsStrLocked is to guarantee thread-safety, so using a straight bool
is thread-safe as long as the lock is held. If you assume that it's used in a
thread-unsafe way then you have to use an atomic. When you do get everything
right the atomic is indeed redundant.

[Jim Stichnoth, 2015/01/20 16:22:53]

I removed the IsStrLocked field.  The stream lock is now only acquired for
bracketing purposes, and acquiring the stream itself doesn't do any locking or
checking.

+
   Ostream *StrDump; // Stream for dumping / diagnostics
   Ostream *StrEmit; // Stream for code emission
 
@@ -206,11 +265,15 @@ private:
   const ClFlags &Flags;
   RandomNumberGenerator RNG;
   std::unique_ptr<ELFObjectWriter> ObjectWriter;
-  CodeStats StatsFunction;
   CodeStats StatsCumulative;
   std::vector<TimerStack> Timers;
   std::vector<GlobalDeclaration *> GlobalDeclarations;
 
+  std::vector<ThreadContext *> AllThreadContexts;
+  // Each thread has its own TLS pointer which is also held in
+  // AllThreadContexts.
+  thread_local static ThreadContext *TLS;
+
   // Private helpers for mangleName()
   typedef llvm::SmallVector<char, 32> ManglerVector;
   void incrementSubstitutions(ManglerVector &OldName) const;
@@ -245,6 +308,21 @@ private:
   bool Active;
 };
 
+// Helper class for locking the streams and then automatically
+// unlocking them.
+class OstreamLocker {
+private:
+  OstreamLocker(const OstreamLocker &) = delete;
+  OstreamLocker &operator=(const OstreamLocker &) = delete;

[JF, 2015/01/15 01:31:01]

OStreamLocker() = delete;

[Jim Stichnoth, 2015/01/15 07:16:29]

Done.

+
+public:
+  explicit OstreamLocker(GlobalContext *Ctx) : Ctx(Ctx) { Ctx->lockStr(); }
+  ~OstreamLocker() { Ctx->unlockStr(); }
+
+private:
+  GlobalContext *const Ctx;
+};
+
 } // end of namespace Ice
 
 #endif // SUBZERO_SRC_ICEGLOBALCONTEXT_H