Allow Subzero to parse function blocks in parallel.

src/PNaClTranslator.cpp

Index: src/PNaClTranslator.cpp
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index caa0c57c6a05052f1d2926d63039102baba66301..edd0c1ac9e01480f45e5abd5ef373614d6365da5 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -579,11 +579,11 @@ private:
 
 bool TopLevelParser::ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
                              const std::string &Message) {
-  ErrorStatus.assign(Ice::EC_Bitcode);
-  ++NumErrors;
-  Ice::GlobalContext *Context = Translator.getContext();
-  { // Lock while printing out error message.
+  { // Lock while printing out error message and updating state.
+    Ice::GlobalContext *Context = Translator.getContext();
     Ice::OstreamLocker L(Context);
+    ErrorStatus.assign(Ice::EC_Bitcode);

[John, 2016/03/24 17:32:35]

Are you using the OstreamLocker to prevent concurrent assignments to
ErrorStatus? If so, I would suggest that instead a new mutex is added to protect
it.

[Karl, 2016/03/24 22:35:25]

Yes I was using the OstreamLocker. I did it because this is the only place where
these variables get updated. However, I agree that is a dangerous choice in that
others may not note. Changing.

+    ++NumErrors;
     raw_ostream &OldErrStream = setErrStream(Context->getStrError());
     NaClBitcodeParser::ErrorAt(Level, Bit, Message);
     setErrStream(OldErrStream);
@@ -664,6 +664,11 @@ public:
     Context->setBlockParser(this);
   }
 
+  BlockParserBaseClass(unsigned BlockID, BlockParserBaseClass *EnclosingParser,
+                       NaClBitstreamCursor &Cursor)
+      : NaClBitcodeParser(BlockID, EnclosingParser, Cursor),
+        Context(EnclosingParser->Context) {}
+
   ~BlockParserBaseClass() override { Context->setBlockParser(nullptr); }
 
   // Returns the printable name of the type of block being parsed.
@@ -679,6 +684,8 @@ public:
 protected:
   // The context parser that contains the decoded state.
   TopLevelParser *Context;
+  // True if ErrorAt has been called in this block.
+  bool BlockHasError = false;
 
   // Constructor for nested block parsers.
   BlockParserBaseClass(unsigned BlockID, BlockParserBaseClass *EnclosingParser)
@@ -756,6 +763,7 @@ bool TopLevelParser::blockError(const std::string &Message) {
 // Generates an error Message with the bit address prefixed to it.
 bool BlockParserBaseClass::ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
                                    const std::string &Message) {
+  BlockHasError = true;
   std::string Buffer;
   raw_string_ostream StrBuf(Buffer);
   // Note: If dump routines have been turned off, the error messages will not
@@ -1342,22 +1350,30 @@ class FunctionParser : public BlockParserBaseClass {
   FunctionParser &operator=(const FunctionParser &) = delete;
 
 public:
-  FunctionParser(unsigned BlockID, BlockParserBaseClass *EnclosingParser)
+  FunctionParser(unsigned BlockID, BlockParserBaseClass *EnclosingParser,
+                 NaClBcIndexSize_t FcnId)
       : BlockParserBaseClass(BlockID, EnclosingParser),
         Timer(Ice::TimerStack::TT_parseFunctions, getTranslator().getContext()),
-        Func(nullptr), FcnId(Context->getNextFunctionBlockValueID()),
-        FuncDecl(Context->getFunctionByID(FcnId)),
+        Func(nullptr), FuncDecl(Context->getFunctionByID(FcnId)),
+        CachedNumGlobalValueIDs(Context->getNumGlobalIDs()),
+        NextLocalInstIndex(Context->getNumGlobalIDs()) {}
+
+  FunctionParser(unsigned BlockID, BlockParserBaseClass *EnclosingParser,
+                 NaClBcIndexSize_t FcnId, NaClBitstreamCursor &Cursor)
+      : BlockParserBaseClass(BlockID, EnclosingParser, Cursor),
+        Timer(Ice::TimerStack::TT_parseFunctions, getTranslator().getContext()),
+        Func(nullptr), FuncDecl(Context->getFunctionByID(FcnId)),
         CachedNumGlobalValueIDs(Context->getNumGlobalIDs()),
         NextLocalInstIndex(Context->getNumGlobalIDs()) {}
 
-  bool convertFunction() {
+  std::unique_ptr<Ice::Cfg> parseFunction(uint32_t SeqNumber) {
     bool ParserResult;
+    Ice::GlobalContext *Ctx = getTranslator().getContext();
     {
-      Ice::TimerMarker T(getTranslator().getContext(), FuncDecl->getName());
+      Ice::TimerMarker T(Ctx, FuncDecl->getName());
       // Note: The Cfg is created, even when IR generation is disabled. This is
       // done to install a CfgLocalAllocator for various internal containers.
-      Func = Ice::Cfg::create(getTranslator().getContext(),
-                              getTranslator().getNextSequenceNumber());
+      Func = Ice::Cfg::create(Ctx, SeqNumber);
 
       Ice::CfgLocalAllocatorScope _(Func.get());
 
@@ -1374,20 +1390,12 @@ public:
       }
 
       ParserResult = ParseThisBlock();
-
-      // Note: Once any errors have been found, we turn off all translation of
-      // all remaining functions. This allows successive parsing errors to be
-      // reported, without adding extra checks to the translator for such
-      // parsing errors.
-    }
-    if (Context->getNumErrors() == 0 && Func) {
-      getTranslator().translateFcn(std::move(Func));
-      // The translator now has ownership of Func.
-    } else {
-      Func.reset();
     }
 
-    return ParserResult;
+    if (ParserResult || BlockHasError)
+      Func->setError("Unable to parse function");
+
+    return std::move(Func);

[John, 2016/03/24 17:32:35]

no need to move it here -- Func is an xvalue here -- because of the return -- so
it will be moved. (and according to the "Effective Modern C++11" book, having
std::move here may also lead to worse code.

[Karl, 2016/03/24 22:35:26]

The compiler disagrees with you!

[John, 2016/03/25 04:55:22]

Ah, Func is a class member... That's one of the worst features the stupid llvm
coding style... <rant />

Does Func need to be a class member? It seems it doesn't. If it doesn't, make it
a local -- then std::move is not needed.

[Karl, 2016/03/29 17:35:02]

It uses many instance fields and methods of the FunctionParser, so yes, it
should be a class member.

   }
 
   ~FunctionParser() final = default;
@@ -1434,8 +1442,6 @@ private:
   NaClBcIndexSize_t DeclaredNumberBbs = 0;
   // The basic block being built.
   Ice::CfgNode *CurrentNode = nullptr;
-  // The ID for the function.
-  NaClBcIndexSize_t FcnId;
   // The corresponding function declaration.
   Ice::FunctionDeclaration *FuncDecl;
   // Holds the dividing point between local and global absolute value indices.
@@ -2984,10 +2990,9 @@ public:
       : BlockParserBaseClass(BlockID, Context),
         Timer(Ice::TimerStack::TT_parseModule,
               Context->getTranslator().getContext()) {}
-
-  ~ModuleParser() override = default;
-
+  ~ModuleParser() final = default;

[John, 2016/03/24 17:32:35]

marking the destructor "final" is a pretty strong evidence that the class should
be final instead.

[Karl, 2016/03/24 22:35:25]

Moved final keywords (where appropriate) to the class.

   const char *getBlockName() const override { return "module"; }
+  NaClBitstreamReader &getReader() { return Record.GetReader(); }
 
 private:
   Ice::TimerMarker Timer;
@@ -3015,7 +3020,10 @@ private:
   }
   bool ParseBlock(unsigned BlockID) override;
 
-  void ExitBlock() override { installGlobalNamesAndGlobalVarInitializers(); }
+  void ExitBlock() override {
+    installGlobalNamesAndGlobalVarInitializers();
+    Context->getTranslator().getContext()->waitForWorkerThreads();
+  }
 
   void ProcessRecord() override;
 };
@@ -3060,6 +3068,33 @@ void ModuleValuesymtabParser::setBbName(NaClBcIndexSize_t Index,
   reportUnableToAssign("Basic block", Index, Name);
 }
 
+class CfgParserWorkItem : public Ice::OptWorkItem {
+  CfgParserWorkItem() = delete;
+  CfgParserWorkItem(const CfgParserWorkItem &) = delete;
+  CfgParserWorkItem &operator=(const CfgParserWorkItem &) = delete;
+
+public:
+  CfgParserWorkItem(unsigned BlockID, NaClBcIndexSize_t FcnId,
+                    ModuleParser *ModParser, uint64_t StartBit,
+                    uint32_t SeqNumber)
+      : BlockID(BlockID), FcnId(FcnId), ModParser(ModParser),
+        StartBit(StartBit), SeqNumber(SeqNumber) {}
+  std::unique_ptr<Ice::Cfg> getParsedCfg() final {
+    NaClBitstreamCursor Cursor(ModParser->getReader());
+    Cursor.JumpToBit(StartBit);
+    FunctionParser Parser(BlockID, ModParser, FcnId, Cursor);
+    return Parser.parseFunction(SeqNumber);
+  }
+  ~CfgParserWorkItem() final {}

[John, 2016/03/24 17:32:35]

make the class final instead?

[Karl, 2016/03/24 22:35:25]

Done.

+
+private:
+  unsigned BlockID;

[John, 2016/03/24 17:32:35]

can these be const?

[Karl, 2016/03/24 22:35:26]

Done, except that ModParser can't be constant (Constructor FunctionParser()
requires it.

+  NaClBcIndexSize_t FcnId;
+  ModuleParser *ModParser;
+  uint64_t StartBit;
+  uint32_t SeqNumber;
+};
+
 bool ModuleParser::ParseBlock(unsigned BlockID) {
   switch (BlockID) {
   case naclbitc::BLOCKINFO_BLOCK_ID:
@@ -3088,8 +3123,24 @@ bool ModuleParser::ParseBlock(unsigned BlockID) {
   }
   case naclbitc::FUNCTION_BLOCK_ID: {
     installGlobalNamesAndGlobalVarInitializers();
-    FunctionParser Parser(BlockID, this);
-    return Parser.convertFunction();
+    Ice::GlobalContext *Ctx = Context->getTranslator().getContext();
+    uint32_t SeqNumber = Context->getTranslator().getNextSequenceNumber();
+    NaClBcIndexSize_t FcnId = Context->getNextFunctionBlockValueID();
+    if (Ctx->getFlags().getParseParallel()) {
+      uint64_t StartBit = Record.GetCursor().GetCurrentBitNo();
+      if (SkipBlock())
+        return true;
+      Ice::GlobalContext *Ctx = Context->getTranslator().getContext();
+      Ctx->optQueueBlockingPush(Ice::makeUnique<CfgParserWorkItem>(
+          BlockID, FcnId, this, StartBit, SeqNumber));
+      return false;
+    } else {
+      FunctionParser Parser(BlockID, this, FcnId);
+      std::unique_ptr<Ice::Cfg> Func = Parser.parseFunction(SeqNumber);
+      bool Failed = Func->hasError();
+      getTranslator().translateFcn(std::move(Func));
+      return Failed && !getTranslator().getFlags().getAllowErrorRecovery();
+    }
   }
   default:
     return BlockParserBaseClass::ParseBlock(BlockID);