Allow Subzero to parse function blocks in parallel.

src/PNaClTranslator.cpp

 //===- subzero/src/PNaClTranslator.cpp - ICE from bitcode -----------------===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 413 matching lines @@
     // Error recover with value that is always acceptable.
     return 1;
   }
 
 private:
   // The translator associated with the parser.
   Ice::Translator &Translator;
   // The exit status that should be set to true if an error occurs.
   Ice::ErrorCode &ErrorStatus;
   // The number of errors reported.
   unsigned NumErrors = 0;

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

Removed this field. It is no longer used.

   // The types associated with each type ID.
   std::vector<ExtendedType> TypeIDValues;
   // The set of functions (prototype and defined).
   Ice::FunctionDeclarationList FunctionDeclarations;
   // The ID of the next possible defined function ID in FunctionDeclarations.
   // FunctionDeclarations is filled first. It's the set of functions (either
   // defined or isproto). Then function definitions are encountered/parsed and
   // NextDefiningFunctionID is incremented to track the next actually-defined
   // function.
   size_t NextDefiningFunctionID = 0;
@@ skipping 127 matching lines @@
   Ice::VariableDeclaration *
   reportGetGlobalVariableByIDError(NaClBcIndexSize_t Index);
 
   // Reports that there is no corresponding ICE type for LLVMTy, and returns
   // Ice::IceType_void.
   Ice::Type convertToIceTypeError(Type *LLVMTy);
 };
 
 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);
   }
   if (Level >= naclbitc::Error &&
       !Translator.getFlags().getAllowErrorRecovery())
     Fatal();
   return true;
 }
 
@@ skipping 60 matching lines @@
   BlockParserBaseClass(const BlockParserBaseClass &) = delete;
   BlockParserBaseClass &operator=(const BlockParserBaseClass &) = delete;
 
 public:
   // Constructor for the top-level module block parser.
   BlockParserBaseClass(unsigned BlockID, TopLevelParser *Context)
       : NaClBitcodeParser(BlockID, Context), Context(Context) {
     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.
   virtual const char *getBlockName() const {
     // If this class is used, it is parsing an unknown block.
     return "unknown";
   }
 
   // Generates an error Message with the Bit address prefixed to it.
   bool ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
                const std::string &Message) final;
 
 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)
       : NaClBitcodeParser(BlockID, EnclosingParser),
         Context(EnclosingParser->Context) {}
 
   // Gets the translator associated with the bitcode parser.
   Ice::Translator &getTranslator() const { return Context->getTranslator(); }
 
   const Ice::ClFlags &getFlags() const { return getTranslator().getFlags(); }
@@ skipping 57 matching lines @@
 bool TopLevelParser::blockError(const std::string &Message) {
   if (BlockParser)
     return BlockParser->Error(Message);
   else
     return Error(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
   // be readable. Hence, replace with simple error. We also use the simple form
   // for unit tests.
   if (getFlags().getGenerateUnitTestMessages()) {
     StrBuf << "Invalid " << getBlockName() << " record: <" << Record.GetCode();
     for (const uint64_t Val : Record.GetValues()) {
       StrBuf << " " << Val;
     }
@@ skipping 566 matching lines @@
   return;
 }
 
 /// Parses function blocks in the bitcode file.
 class FunctionParser : 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()) {}
 
-  bool convertFunction() {
+  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()) {}
+
+  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());
 
       // TODO(kschimpf) Clean up API to add a function signature to a CFG.
       const Ice::FuncSigType &Signature = FuncDecl->getSignature();
 
       Func->setFunctionName(FuncDecl->getName());
       Func->setReturnType(Signature.getReturnType());
       Func->setInternal(FuncDecl->getLinkage() == GlobalValue::InternalLinkage);
       CurrentNode = installNextBasicBlock();
       Func->setEntryNode(CurrentNode);
       for (Ice::Type ArgType : Signature.getArgList()) {
         Func->addArg(getNextInstVar(ArgType));
       }
 
       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;
 
   const char *getBlockName() const override { return "function"; }
 
   Ice::Cfg *getFunc() const { return Func.get(); }
 
   size_t getNumGlobalIDs() const { return CachedNumGlobalValueIDs; }
 
@@ skipping 26 matching lines @@
   // the number of bytes defining the function block.
   uint64_t MaxRecordsInBlock = 0;
   // The corresponding ICE function defined by the function block.
   std::unique_ptr<Ice::Cfg> Func;
   // The index to the current basic block being built.
   NaClBcIndexSize_t CurrentBbIndex = 0;
   // The number of basic blocks declared for the function block.
   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.
   size_t CachedNumGlobalValueIDs;
   // Holds operands local to the function block, based on indices defined in
   // the bitcode file.
   Ice::OperandList LocalOperands;
   // Holds the index within LocalOperands corresponding to the next instruction
   // that generates a value.
   NaClBcIndexSize_t NextLocalInstIndex;
@@ skipping 1528 matching lines @@
 class ModuleParser : public BlockParserBaseClass {
   ModuleParser() = delete;
   ModuleParser(const ModuleParser &) = delete;
   ModuleParser &operator=(const ModuleParser &) = delete;
 
 public:
   ModuleParser(unsigned BlockID, TopLevelParser *Context)
       : 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;
   // True if we have already installed names for unnamed global declarations,
   // and have generated global constant initializers.
   bool GlobalDeclarationNamesAndInitializersInstalled = false;
   // True if we have already processed the symbol table for the module.
   bool FoundValuesymtab = false;
 
   // Generates names for unnamed global addresses (i.e. functions and global
   // variables). Then lowers global variable declaration initializers to the
   // target. May be called multiple times. Only the first call will do the
   // installation.
   void installGlobalNamesAndGlobalVarInitializers() {
     if (!GlobalDeclarationNamesAndInitializersInstalled) {
       Context->installGlobalNames();
       Context->createValueIDs();
       Context->verifyFunctionTypeSignatures();
       std::unique_ptr<Ice::VariableDeclarationList> Globals =
           Context->getGlobalVariables();
       if (Globals)
         getTranslator().lowerGlobals(std::move(Globals));
       GlobalDeclarationNamesAndInitializersInstalled = true;
     }
   }
   bool ParseBlock(unsigned BlockID) override;
 
-  void ExitBlock() override { installGlobalNamesAndGlobalVarInitializers(); }
+  void ExitBlock() override {
+    installGlobalNamesAndGlobalVarInitializers();
+    Context->getTranslator().getContext()->waitForWorkerThreads();
+  }
 
   void ProcessRecord() override;
 };
 
 class ModuleValuesymtabParser : public ValuesymtabParser {
   ModuleValuesymtabParser() = delete;
   ModuleValuesymtabParser(const ModuleValuesymtabParser &) = delete;
   void operator=(const ModuleValuesymtabParser &) = delete;
 
 public:
@@ skipping 24 matching lines @@
   }
 
   Decl->setName(StringRef(Name.data(), Name.size()));
 }
 
 void ModuleValuesymtabParser::setBbName(NaClBcIndexSize_t Index,
                                         StringType &Name) {
   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:
     return NaClBitcodeParser::ParseBlock(BlockID);
   case naclbitc::TYPE_BLOCK_ID_NEW: {
     TypesParser Parser(BlockID, this);
     return Parser.ParseThisBlock();
   }
   case naclbitc::GLOBALVAR_BLOCK_ID: {
     GlobalsParser Parser(BlockID, this);
     return Parser.ParseThisBlock();
   }
   case naclbitc::VALUE_SYMTAB_BLOCK_ID: {
     if (FoundValuesymtab)
       Fatal("Duplicate valuesymtab in module");
 
     // If we have already processed a function block (i.e. we have already
     // installed global names and variable initializers) we can no longer accept
     // the value symbol table. Names have already been generated.
     if (GlobalDeclarationNamesAndInitializersInstalled)
       Fatal("Module valuesymtab not allowed after function blocks");
 
     FoundValuesymtab = true;
     ModuleValuesymtabParser Parser(BlockID, this);
     return Parser.ParseThisBlock();
   }
   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);
   }
 }
 
 void ModuleParser::ProcessRecord() {
   const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
   switch (Record.GetCode()) {
   case naclbitc::MODULE_CODE_VERSION: {
@@ skipping 107 matching lines @@
     raw_string_ostream StrBuf(Buffer);
     StrBuf << IRFilename << ": Does not contain a module!";
     llvm::report_fatal_error(StrBuf.str());
   }
   if (InputStreamFile.getBitcodeBytes().getExtent() % 4 != 0) {
     llvm::report_fatal_error("Bitcode stream should be a multiple of 4 bytes");
   }
 }
 
 } // end of namespace Ice