Subzero: Add locking to prepare for multithreaded translation.

src/IceGlobalContext.h

 //===- subzero/src/IceGlobalContext.h - Global context defs -----*- C++ -*-===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file declares aspects of the compilation that persist across
 // multiple functions.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SUBZERO_SRC_ICEGLOBALCONTEXT_H
 #define SUBZERO_SRC_ICEGLOBALCONTEXT_H
 
 #include <memory>
+#include <mutex>
 
 #include "IceDefs.h"
 #include "IceClFlags.h"
 #include "IceIntrinsics.h"
 #include "IceRNG.h"
 #include "IceTimerTree.h"
 #include "IceTypes.h"
 
 namespace Ice {
 
 class ClFlags;
 class FuncSigType;
 
-// This class 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);
-
-private:
-  uint32_t InstructionsEmitted;
-  uint32_t RegistersSaved;
-  uint32_t FrameBytes;
-  uint32_t Spills;
-  uint32_t Fills;
-};
-
-// 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;
 
+  // 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);
+
+  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;
+  };
+
+  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,
                 VerboseMask Mask, TargetArch Arch, OptLevel Opt,
                 IceString TestPrefix, const ClFlags &Flags);
   ~GlobalContext();
 
   // Returns true if any of the specified options in the verbose mask
   // are set.  If the argument is omitted, it checks if any verbose
   // options at all are set.
   VerboseMask getVerbose() const { return VMask; }
   bool isVerbose(VerboseMask Mask = IceV_All) const { return VMask & Mask; }
   void setVerbose(VerboseMask Mask) { VMask = Mask; }
   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; }
 
   // When emitting assembly, we allow a string to be prepended to
   // names of translated functions.  This makes it easier to create an
   // execution test against a reference translator like llc, with both
   // translators using the same bitcode as input.
   IceString getTestPrefix() const { return TestPrefix; }
   IceString mangleName(const IceString &Name) const;
@@ skipping 11 matching lines @@
   Constant *getConstantDouble(double Value);
   // Returns a symbolic constant.
   Constant *getConstantSym(RelocOffsetT Offset, const IceString &Name,
                            bool SuppressMangling);
   // Returns an undef.
   Constant *getConstantUndef(Type Ty);
   // Returns a zero value.
   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.
   FunctionDeclaration *newFunctionDeclaration(const FuncSigType *Signature,
                                               unsigned CallingConv,
                                               unsigned Linkage, bool IsProto);
 
   // Returns a new global variable declaration, allocated in an
   // internal memory pool.  Ownership of the function is maintained by
   // this class instance.
   VariableDeclaration *newVariableDeclaration();
 
   const ClFlags &getFlags() const { return Flags; }
 
   bool isIRGenerationDisabled() const {
     return ALLOW_DISABLE_IR_GEN ? getFlags().DisableIRGeneration : false;
   }
 
   // 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; }
 
   // TODO(wala,stichnot): Make the RNG play nicely with multithreaded
   // translation.
   RandomNumberGenerator &getRNG() { return RNG; }
 
   ELFObjectWriter *getObjectWriter() const { return ObjectWriter.get(); }
 
   // 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();
   }
 
   // These are predefined TimerStackIdT values.
   enum TimerStackKind {
     TSK_Default = 0,
     TSK_Funcs,
     TSK_Num
   };
 
+  TimerStackIdT newTimerStackID(const IceString &Name);
   TimerIdT getTimerID(TimerStackIdT StackID, const IceString &Name);
-  TimerStackIdT newTimerStackID(const IceString &Name);
   void pushTimer(TimerIdT ID, TimerStackIdT StackID = TSK_Default);
   void popTimer(TimerIdT ID, TimerStackIdT StackID = TSK_Default);
   void resetTimer(TimerStackIdT StackID);
   void setTimerName(TimerStackIdT StackID, const IceString &NewName);
   void dumpTimers(TimerStackIdT StackID = TSK_Default,
                   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
 
   ArenaAllocator<> Allocator;
   VerboseMask VMask;
   std::unique_ptr<class ConstantPool> ConstPool;
   Intrinsics IntrinsicsInfo;
   const TargetArch Arch;
   const OptLevel Opt;
   const IceString TestPrefix;
   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;
 };
 
 // Helper class to push and pop a timer marker.  The constructor
 // pushes a marker, and the destructor pops it.  This is for
 // convenient timing of regions of code.
 class TimerMarker {
   TimerMarker(const TimerMarker &) = delete;
@@ skipping 14 matching lines @@
     if (ALLOW_DUMP && Active)
       Ctx->popTimer(ID);
   }
 
 private:
   TimerIdT ID;
   GlobalContext *const Ctx;
   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