Subzero: Emit functions and global initializers in a separate thread.

src/IceGlobalContext.h

Index: src/IceGlobalContext.h
diff --git a/src/IceGlobalContext.h b/src/IceGlobalContext.h
index e3e0810cb02824af79d25403db76e50e9aedcc9b..3aa01e2495f4be14fec6ed56709c1500a1552592 100644
--- a/src/IceGlobalContext.h
+++ b/src/IceGlobalContext.h
@@ -31,6 +31,7 @@ namespace Ice {
 
 class ClFlags;
 class ConstantPool;
+class EmitterWorkItem;
 class FuncSigType;
 
 // LockedPtr is a way to provide automatically locked access to some object.
@@ -276,6 +277,12 @@ public:
   void resetTimer(TimerStackIdT StackID);
   void setTimerName(TimerStackIdT StackID, const IceString &NewName);
 
+  // This is the first work item sequence number that the parser
+  // produces, and correspondingly the first sequence number that the
+  // emitter thread will wait for.  Start numbering at 1 to leave room
+  // for a sentinel, in case e.g. we wish to inject items with a
+  // special sequence number that may be executed out of order.
+  static uint32_t getFirstSequenceNumber() { return 1; }
   // Adds a newly parsed and constructed function to the Cfg work
   // queue.  Notifies any idle workers that a new function is
   // available for translating.  May block if the work queue is too
@@ -289,6 +296,10 @@ public:
   // Notifies that no more work will be added to the work queue.
   void cfgQueueNotifyEnd() { CfgQ.notifyEnd(); }
 
+  void emitQueueBlockingPush(EmitterWorkItem *Item);
+  EmitterWorkItem *emitQueueBlockingPop();
+  void emitQueueNotifyEnd() { EmitQ.notifyEnd(); }
+
   void startWorkerThreads() {
     size_t NumWorkers = getFlags().NumTranslationThreads;
     auto Timers = getTimers();
@@ -300,18 +311,29 @@ public:
           &GlobalContext::translateFunctionsWrapper, this, WorkerTLS));
     }
     if (NumWorkers) {
-      // TODO(stichnot): start a new thread for the emitter queue worker.
+      ThreadContext *WorkerTLS = new ThreadContext();
+      Timers->initInto(WorkerTLS->Timers);
+      AllThreadContexts.push_back(WorkerTLS);
+      EmitterThreads.push_back(
+          std::thread(&GlobalContext::emitterWrapper, this, WorkerTLS));
     }
   }
 
   void waitForWorkerThreads() {
     cfgQueueNotifyEnd();
-    // TODO(stichnot): call end() on the emitter work queue.
     for (std::thread &Worker : TranslationThreads) {
       Worker.join();
     }
     TranslationThreads.clear();
-    // TODO(stichnot): join the emitter thread.
+
+    // Only notify the emit queue to end after all the translation
+    // threads have ended.
+    emitQueueNotifyEnd();
+    for (std::thread &Worker : EmitterThreads) {
+      Worker.join();
+    }
+    EmitterThreads.clear();
+
     if (ALLOW_DUMP) {
       auto Timers = getTimers();
       for (ThreadContext *TLS : AllThreadContexts)
@@ -334,6 +356,15 @@ public:
   // Translate functions from the Cfg queue until the queue is empty.
   void translateFunctions();
 
+  // Emitter thread startup routine.
+  void emitterWrapper(ThreadContext *MyTLS) {
+    ICE_TLS_SET_FIELD(TLS, MyTLS);
+    emitItems();
+  }
+  // Emit functions and global initializers from the emitter queue
+  // until the queue is empty.
+  void emitItems();
+
   // Utility function to match a symbol name against a match string.
   // This is used in a few cases where we want to take some action on
   // a particular function or symbol based on a command-line argument,
@@ -391,6 +422,7 @@ private:
   RandomNumberGenerator RNG; // TODO(stichnot): Move into Cfg.
   std::unique_ptr<ELFObjectWriter> ObjectWriter;
   BoundedProducerConsumerQueue<Cfg> CfgQ;

[JF, 2015/02/08 00:29:47]

Maybe this should now be the OptimizationQ?

[Jim Stichnoth, 2015/02/08 17:11:23]

Done.

+  BoundedProducerConsumerQueue<EmitterWorkItem> EmitQ;
 
   LockedPtr<ArenaAllocator<>> getAllocator() {
     return LockedPtr<ArenaAllocator<>>(&Allocator, &AllocLock);
@@ -407,6 +439,7 @@ private:
 
   std::vector<ThreadContext *> AllThreadContexts;
   std::vector<std::thread> TranslationThreads;
+  std::vector<std::thread> EmitterThreads;

[JF, 2015/02/08 00:29:47]

These could just be SmallVector or even std::array, since you won't spin up more
than ~16 threads. Reduces fragmentation since you'll have less allocations!

[Jim Stichnoth, 2015/02/08 17:11:23]

16 threads?  I've forgotten how to count that low.

   // Each thread has its own TLS pointer which is also held in
   // AllThreadContexts.
   ICE_TLS_DECLARE_FIELD(ThreadContext *, TLS);
@@ -471,6 +504,59 @@ private:
   GlobalContext *const Ctx;
 };
 
+class EmitterWorkItem {

[JF, 2015/02/08 00:29:47]

This is getting to be a pretty big file, it's probably worth moving this to its
own file.

[Jim Stichnoth, 2015/02/08 17:11:23]

Good idea, moved this into the new IceThreading.h, along with the blocking queue
stuff from IceUtils.h.

+  EmitterWorkItem(const EmitterWorkItem &) = delete;
+  EmitterWorkItem &operator=(const EmitterWorkItem &) = delete;

[JF, 2015/02/08 00:29:47]

EmitterWorkItem() = delete;

[Jim Stichnoth, 2015/02/08 17:11:23]

Done.

+
+public:
+  enum ItemKind {
+    WI_Nop,         // Placeholder to maintain sequence numbers in case there
+                    // is a translation error.
+    WI_GlobalInits, // A list of global initializers.
+    WI_Asm,         // An already-assembled function that needs to be emitted,
+                    // either as low-level asm text or as an ELF binary.
+    WI_Cfg          // A Cfg that needs to be emitted as "readable" assembly.

[JF, 2015/02/08 00:29:47]

I'm not sure I get the different between asm and cfg.

[Jim Stichnoth, 2015/02/08 17:11:23]

Added more comments that hopefully clarify.

[JF, 2015/02/08 21:15:04]

Yeah, though I'm wary of having a debugging feature that always ships with
subzero. Does WI_Cfg and related functions become a nop in release mode?

[Jim Stichnoth, 2015/02/10 07:51:46]

I added a report_fatal_error() call to GlobalContext::emitItems() in case a
WI_Cfg item somehow makes it onto the queue in a release build.

+  };
+  // Constructor for a Nop work item.
+  explicit EmitterWorkItem(uint32_t Seq)
+      : Sequence(Seq), Kind(WI_Nop), GlobalInits(nullptr), Function(nullptr),
+        RawFunc(nullptr) {}
+  // Constructor for a GlobalInits work item.
+  EmitterWorkItem(uint32_t Seq, VariableDeclarationList *D)
+      : Sequence(Seq), Kind(WI_GlobalInits), GlobalInits(D), Function(nullptr),
+        RawFunc(nullptr) {}
+  // Constructor for an Asm work item.
+  EmitterWorkItem(uint32_t Seq, Assembler *A)
+      : Sequence(Seq), Kind(WI_Asm), GlobalInits(nullptr), Function(A),
+        RawFunc(nullptr) {}
+  // Constructor for a Cfg work item.
+  EmitterWorkItem(uint32_t Seq, Cfg *F)
+      : Sequence(Seq), Kind(WI_Cfg), GlobalInits(nullptr), Function(nullptr),
+        RawFunc(F) {}
+  uint32_t getSequenceNumber() const { return Sequence; }
+  ItemKind getKind() const { return Kind; }
+  VariableDeclarationList *getGlobalInits() const {
+    assert(getKind() == WI_GlobalInits);
+    return GlobalInits;
+  }
+  Assembler *getAsm() const {
+    assert(getKind() == WI_Asm);
+    return Function;
+  }
+  Cfg *getCfg() const {
+    assert(getKind() == WI_Cfg);
+    return RawFunc;
+  }
+  ~EmitterWorkItem();

[JF, 2015/02/08 00:29:47]

Define inline, since this should do anything.

[Jim Stichnoth, 2015/02/08 17:11:23]

Tried that originally, but it's getting into icky territory (which I'm guessing
unique_ptr won't solve):

make: *** [build/Release+Asserts/assembler_ia32.o] Error 1
In file included from src/IceCfgNode.cpp:16:
In file included from src/IceCfg.h:21:
src/IceGlobalContext.h:553:3: error: deleting pointer to incomplete type
'Ice::Cfg' may cause undefined behavior [-Werror,-Wdelete-incomplete]
  delete RawFunc;
  ^      ~~~~~~~
src/IceDefs.h:49:7: note: forward declaration of 'Ice::Cfg'
class Cfg;
      ^
1 error generated.

[JF, 2015/02/08 21:15:04]

Oh yeah, include order would do that, and unique_ptr would have the same issue.

+
+private:
+  const uint32_t Sequence;
+  const ItemKind Kind;
+  VariableDeclarationList *const GlobalInits;
+  Assembler *const Function;
+  Cfg *const RawFunc;

[JF, 2015/02/08 00:29:47]

3 x unique_ptr?

[Jim Stichnoth, 2015/02/10 07:51:46]

Yeah, I think so, after Karl's CL lands...

+};
+
 } // end of namespace Ice
 
 #endif // SUBZERO_SRC_ICEGLOBALCONTEXT_H