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..056ce082d4b39efedbe50a790af1bdb088b11c0d 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -221,7 +221,7 @@ private:
 class BlockParserBaseClass;
 
 // Top-level class to read PNaCl bitcode files, and translate to ICE.
-class TopLevelParser : public NaClBitcodeParser {
+class TopLevelParser final : public NaClBitcodeParser {
   TopLevelParser() = delete;
   TopLevelParser(const TopLevelParser &) = delete;
   TopLevelParser &operator=(const TopLevelParser &) = delete;
@@ -244,14 +244,11 @@ public:
   /// Generates error with given Message, occurring at BitPosition within the
   /// bitcode file. Always returns true.
   bool ErrorAt(naclbitc::ErrorLevel Level, uint64_t BitPosition,
-               const std::string &Message) final;
+               const std::string &Message) override;
 
   /// Generates error message with respect to the current block parser.
   bool blockError(const std::string &Message);
 
-  /// Returns the number of errors found while parsing the bitcode file.
-  unsigned getNumErrors() const { return NumErrors; }
-
   /// Changes the size of the type list to the given size.
   void resizeTypeIDValues(size_t NewSize) { TypeIDValues.resize(NewSize); }
 
@@ -428,10 +425,12 @@ public:
 private:
   // The translator associated with the parser.
   Ice::Translator &Translator;
+
+  // ErrorStatus should only be updated while this lock is locked.
+  Ice::GlobalLockType ErrorReportingLock;
   // The exit status that should be set to true if an error occurs.
   Ice::ErrorCode &ErrorStatus;
-  // The number of errors reported.
-  unsigned NumErrors = 0;
+
   // The types associated with each type ID.
   std::vector<ExtendedType> TypeIDValues;
   // The set of functions (prototype and defined).
@@ -579,9 +578,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();
+  {
+    std::unique_lock<Ice::GlobalLockType> L(ErrorReportingLock);

[Jim Stichnoth, 2016/03/25 04:31:04]

name this "_" instead of "L"?

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

Done.

+    ErrorStatus.assign(Ice::EC_Bitcode);
+  }
   { // Lock while printing out error message.
     Ice::OstreamLocker L(Context);
     raw_ostream &OldErrStream = setErrStream(Context->getStrError());
@@ -589,8 +590,10 @@ bool TopLevelParser::ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
     setErrStream(OldErrStream);
   }
   if (Level >= naclbitc::Error &&
-      !Translator.getFlags().getAllowErrorRecovery())
+      !Translator.getFlags().getAllowErrorRecovery()) {
+    Context->waitForWorkerThreads();
     Fatal();
+  }
   return true;
 }
 
@@ -664,6 +667,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.
@@ -674,11 +682,13 @@ public:
 
   // Generates an error Message with the Bit address prefixed to it.
   bool ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
-               const std::string &Message) final;
+               const std::string &Message) override;
 
 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 +766,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
@@ -812,7 +823,7 @@ void BlockParserBaseClass::ProcessRecord() {
 }
 
 // Class to parse a types block.
-class TypesParser : public BlockParserBaseClass {
+class TypesParser final : public BlockParserBaseClass {
   TypesParser() = delete;
   TypesParser(const TypesParser &) = delete;
   TypesParser &operator=(const TypesParser &) = delete;
@@ -1016,7 +1027,7 @@ void TypesParser::ProcessRecord() {
 
 /// Parses the globals block (i.e. global variable declarations and
 /// corresponding initializers).
-class GlobalsParser : public BlockParserBaseClass {
+class GlobalsParser final : public BlockParserBaseClass {
   GlobalsParser() = delete;
   GlobalsParser(const GlobalsParser &) = delete;
   GlobalsParser &operator=(const GlobalsParser &) = delete;
@@ -1032,7 +1043,7 @@ public:
     Context->getGlobalVariablesPool()->willNotBeEmitted(DummyGlobalVar);
   }
 
-  ~GlobalsParser() final = default;
+  ~GlobalsParser() override = default;
 
   const char *getBlockName() const override { return "globals"; }
 
@@ -1336,28 +1347,36 @@ void ValuesymtabParser::ProcessRecord() {
 }
 
 /// Parses function blocks in the bitcode file.
-class FunctionParser : public BlockParserBaseClass {
+class FunctionParser final : public BlockParserBaseClass {
   FunctionParser() = delete;
   FunctionParser(const FunctionParser &) = delete;
   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,23 +1393,15 @@ 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);
   }
 
-  ~FunctionParser() final = default;
+  ~FunctionParser() override = default;
 
   const char *getBlockName() const override { return "function"; }
 
@@ -1434,8 +1445,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.
@@ -1453,7 +1462,7 @@ private:
 
   void ProcessRecord() override;
 
-  void EnterBlock(unsigned NumWords) final {
+  void EnterBlock(unsigned NumWords) override {
     // Note: Bitstream defines words as 32-bit values.
     NumBytesDefiningFunction = NumWords * sizeof(uint32_t);
     // We know that all records are minimally defined by a two-bit abreviation.
@@ -2770,7 +2779,7 @@ void FunctionParser::ProcessRecord() {
 }
 
 /// Parses constants within a function block.
-class ConstantsParser : public BlockParserBaseClass {
+class ConstantsParser final : public BlockParserBaseClass {
   ConstantsParser() = delete;
   ConstantsParser(const ConstantsParser &) = delete;
   ConstantsParser &operator=(const ConstantsParser &) = delete;
@@ -2887,7 +2896,7 @@ void ConstantsParser::ProcessRecord() {
 }
 
 // Parses valuesymtab blocks appearing in a function block.
-class FunctionValuesymtabParser : public ValuesymtabParser {
+class FunctionValuesymtabParser final : public ValuesymtabParser {
   FunctionValuesymtabParser() = delete;
   FunctionValuesymtabParser(const FunctionValuesymtabParser &) = delete;
   void operator=(const FunctionValuesymtabParser &) = delete;
@@ -2905,10 +2914,10 @@ private:
     return reinterpret_cast<FunctionParser *>(GetEnclosingParser());
   }
 
-  const char *getTableKind() const final { return "Function"; }
+  const char *getTableKind() const override { return "Function"; }
 
-  void setValueName(NaClBcIndexSize_t Index, StringType &Name) final;
-  void setBbName(NaClBcIndexSize_t Index, StringType &Name) final;
+  void setValueName(NaClBcIndexSize_t Index, StringType &Name) override;
+  void setBbName(NaClBcIndexSize_t Index, StringType &Name) override;
 
   // Reports that the assignment of Name to the value associated with index is
   // not possible, for the given Context.
@@ -2974,7 +2983,7 @@ bool FunctionParser::ParseBlock(unsigned BlockID) {
 }
 
 /// Parses the module block in the bitcode file.
-class ModuleParser : public BlockParserBaseClass {
+class ModuleParser final : public BlockParserBaseClass {
   ModuleParser() = delete;
   ModuleParser(const ModuleParser &) = delete;
   ModuleParser &operator=(const ModuleParser &) = delete;
@@ -2984,10 +2993,9 @@ public:
       : BlockParserBaseClass(BlockID, Context),
         Timer(Ice::TimerStack::TT_parseModule,
               Context->getTranslator().getContext()) {}
-
   ~ModuleParser() override = default;
-
   const char *getBlockName() const override { return "module"; }
+  NaClBitstreamReader &getReader() const { return Record.GetReader(); }
 
 private:
   Ice::TimerMarker Timer;
@@ -3015,7 +3023,10 @@ private:
   }
   bool ParseBlock(unsigned BlockID) override;
 
-  void ExitBlock() override { installGlobalNamesAndGlobalVarInitializers(); }
+  void ExitBlock() override {
+    installGlobalNamesAndGlobalVarInitializers();
+    Context->getTranslator().getContext()->waitForWorkerThreads();
+  }
 
   void ProcessRecord() override;
 };
@@ -3035,9 +3046,9 @@ public:
 
 private:
   Ice::TimerMarker Timer;
-  const char *getTableKind() const final { return "Module"; }
-  void setValueName(NaClBcIndexSize_t Index, StringType &Name) final;
-  void setBbName(NaClBcIndexSize_t Index, StringType &Name) final;
+  const char *getTableKind() const override { return "Module"; }
+  void setValueName(NaClBcIndexSize_t Index, StringType &Name) override;
+  void setBbName(NaClBcIndexSize_t Index, StringType &Name) override;
 };
 
 void ModuleValuesymtabParser::setValueName(NaClBcIndexSize_t Index,
@@ -3060,6 +3071,34 @@ void ModuleValuesymtabParser::setBbName(NaClBcIndexSize_t Index,
   reportUnableToAssign("Basic block", Index, Name);
 }
 
+class CfgParserWorkItem final : 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() override {
+    NaClBitstreamCursor Cursor(ModParser->getReader());
+    Cursor.JumpToBit(StartBit);
+    FunctionParser Parser(BlockID, const_cast<ModuleParser *>(ModParser), FcnId,
+                          Cursor);
+    return Parser.parseFunction(SeqNumber);
+  }
+  ~CfgParserWorkItem() override = default;
+
+private:
+  const unsigned BlockID;
+  const NaClBcIndexSize_t FcnId;
+  const ModuleParser *ModParser;
+  const uint64_t StartBit;
+  const uint32_t SeqNumber;
+};
+
 bool ModuleParser::ParseBlock(unsigned BlockID) {
   switch (BlockID) {
   case naclbitc::BLOCKINFO_BLOCK_ID:
@@ -3088,8 +3127,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);
@@ -3184,11 +3239,13 @@ void PNaClTranslator::translate(const std::string &IRFilename,
   // Read header and verify it is good.
   NaClBitcodeHeader Header;
   if (Header.Read(MemObj.get())) {
+    getContext()->waitForWorkerThreads();
     llvm::report_fatal_error("Invalid PNaCl bitcode header");
   }
   if (!Header.IsSupported()) {
     getContext()->getStrError() << Header.Unsupported();
     if (!Header.IsReadable()) {
+      getContext()->waitForWorkerThreads();
       llvm::report_fatal_error("Invalid PNaCl bitcode header");
     }
   }
@@ -3200,18 +3257,21 @@ void PNaClTranslator::translate(const std::string &IRFilename,
   TopLevelParser Parser(*this, InputStream, ErrorStatus);
   while (!InputStream.AtEndOfStream()) {
     if (Parser.Parse()) {
+      getContext()->waitForWorkerThreads();
       ErrorStatus.assign(EC_Bitcode);
       return;
     }
   }
 
   if (!Parser.parsedModuleBlock()) {
+    getContext()->waitForWorkerThreads();
     std::string Buffer;
     raw_string_ostream StrBuf(Buffer);
     StrBuf << IRFilename << ": Does not contain a module!";
     llvm::report_fatal_error(StrBuf.str());
   }
   if (InputStreamFile.getBitcodeBytes().getExtent() % 4 != 0) {
+    getContext()->waitForWorkerThreads();
     llvm::report_fatal_error("Bitcode stream should be a multiple of 4 bytes");
   }
 }