BitcodeReader: Change mechanics of BlockAddress forward references, NFC

Now that we can reliably handle forward references to `BlockAddress`
(r214563), change the mechanics to simplify predicting use-list order.

Previously, we created dummy `GlobalVariable`s to represent block
addresses.  After every function was materialized, we'd go through any
forward references to its blocks and RAUW them with a proper
`BlockAddress` constant.  This causes some (potentially a lot of)
unnecessary use-list churn, since any constant expression that it's a
part of will need to be rematerialized as well.

Instead, pre-construct a `BasicBlock` immediately -- without attaching
it to its (empty) `Function` -- and use that to construct a
`BlockAddress`.  This constant will not have to be regenerated.  When
the function body is parsed, hook this pre-constructed basic block up
in the right place using `BasicBlock::insertInto()`.

Both before and after this change, the IR is temporarily in an invalid
state that gets resolved when `materializeForwardReferencedFunctions()`
gets called.

This is a prep commit that's part of PR5680, but the only functionality
change is the reduction of churn in the constant pool.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@214570 91177308-0d34-0410-b5e6-96231b3b80d8

lib/Bitcode/Reader/BitcodeReader.cpp

@@ -38,8 +38,8 @@ std::error_code BitcodeReader::materializeForwardReferencedFunctions() {
   // Prevent recursion.
   WillMaterializeAllForwardRefs = true;
 
-  while (!BlockAddrFwdRefs.empty()) {
+  while (!BasicBlockFwdRefs.empty()) {
-    Function *F = BlockAddrFwdRefs.begin()->first;
+    Function *F = BasicBlockFwdRefs.begin()->first;
     assert(F && "Expected valid function");
     // Check for a function that isn't materializable to prevent an infinite
     // loop.  When parsing a blockaddress stored in a global variable, there
@@ -71,7 +71,7 @@ void BitcodeReader::FreeState() {
   DeferredFunctionInfo.clear();
   MDKindMap.clear();
 
-  assert(BlockAddrFwdRefs.empty() && "Unresolved blockaddress fwd references");
+  assert(BasicBlockFwdRefs.empty() && "Unresolved blockaddress fwd references");
 }
 
 //===----------------------------------------------------------------------===//
@@ -1612,24 +1612,26 @@ std::error_code BitcodeReader::ParseConstants() {
 
       // If the function is already parsed we can insert the block address right
       // away.
+      BasicBlock *BB;
+      unsigned BBID = Record[2];
+      if (!BBID)
+        // Invalid reference to entry block.
+        return Error(BitcodeError::InvalidID);
       if (!Fn->empty()) {
         Function::iterator BBI = Fn->begin(), BBE = Fn->end();
-        for (size_t I = 0, E = Record[2]; I != E; ++I) {
+        for (size_t I = 0, E = BBID; I != E; ++I) {
           if (BBI == BBE)
             return Error(BitcodeError::InvalidID);
           ++BBI;
         }
-        V = BlockAddress::get(Fn, BBI);
+        BB = BBI;
       } else {
         // Otherwise insert a placeholder and remember it so it can be inserted
         // when the function is parsed.
-        GlobalVariable *FwdRef = new GlobalVariable(*Fn->getParent(),
+        BB = BasicBlock::Create(Context);
-                                                    Type::getInt8Ty(Context),
+        BasicBlockFwdRefs[Fn].emplace_back(BBID, BB);
-                                            false, GlobalValue::InternalLinkage,
-                                                    nullptr, "");
-        BlockAddrFwdRefs[Fn].push_back(std::make_pair(Record[2], FwdRef));
-        V = FwdRef;
       }
+      V = BlockAddress::get(Fn, BB);
       break;
     }
     }
@@ -2367,15 +2369,45 @@ std::error_code BitcodeReader::ParseFunctionBody(Function *F) {
     switch (BitCode) {
     default: // Default behavior: reject
       return Error(BitcodeError::InvalidValue);
-    case bitc::FUNC_CODE_DECLAREBLOCKS:     // DECLAREBLOCKS: [nblocks]
+    case bitc::FUNC_CODE_DECLAREBLOCKS: {   // DECLAREBLOCKS: [nblocks]
       if (Record.size() < 1 || Record[0] == 0)
         return Error(BitcodeError::InvalidRecord);
       // Create all the basic blocks for the function.
       FunctionBBs.resize(Record[0]);
-      for (unsigned i = 0, e = FunctionBBs.size(); i != e; ++i)
+
-        FunctionBBs[i] = BasicBlock::Create(Context, "", F);
+      // See if anything took the address of blocks in this function.
+      auto BBFRI = BasicBlockFwdRefs.find(F);
+      if (BBFRI == BasicBlockFwdRefs.end()) {
+        for (unsigned i = 0, e = FunctionBBs.size(); i != e; ++i)
+          FunctionBBs[i] = BasicBlock::Create(Context, "", F);
+      } else {
+        auto &BBRefs = BBFRI->second;
+        std::sort(BBRefs.begin(), BBRefs.end(),
+                  [](const std::pair<unsigned, BasicBlock *> &LHS,
+                     const std::pair<unsigned, BasicBlock *> &RHS) {
+          return LHS.first < RHS.first;
+        });
+        unsigned R = 0, RE = BBRefs.size();
+        for (unsigned I = 0, E = FunctionBBs.size(); I != E; ++I)
+          if (R != RE && BBRefs[R].first == I) {
+            assert(I != 0 && "Invalid reference to entry block");
+            BasicBlock *BB = BBRefs[R++].second;
+            BB->insertInto(F);
+            FunctionBBs[I] = BB;
+          } else {
+            FunctionBBs[I] = BasicBlock::Create(Context, "", F);
+          }
+        // Check for invalid basic block references.
+        if (R != RE)
+          return Error(BitcodeError::InvalidID);
+
+        // Erase from the table.
+        BasicBlockFwdRefs.erase(BBFRI);
+      }
+
       CurBB = FunctionBBs[0];
       continue;
+    }
 
     case bitc::FUNC_CODE_DEBUG_LOC_AGAIN:  // DEBUG_LOC_AGAIN
       // This record indicates that the last instruction is at the same
@@ -3210,25 +3242,6 @@ OutOfRecordLoop:
   // FIXME: Check for unresolved forward-declared metadata references
   // and clean up leaks.
 
-  // See if anything took the address of blocks in this function.  If so,
-  // resolve them now.
-  DenseMap<Function*, std::vector<BlockAddrRefTy> >::iterator BAFRI =
-    BlockAddrFwdRefs.find(F);
-  if (BAFRI != BlockAddrFwdRefs.end()) {
-    std::vector<BlockAddrRefTy> &RefList = BAFRI->second;
-    for (unsigned i = 0, e = RefList.size(); i != e; ++i) {
-      unsigned BlockIdx = RefList[i].first;
-      if (BlockIdx >= FunctionBBs.size())
-        return Error(BitcodeError::InvalidID);
-
-      GlobalVariable *FwdRef = RefList[i].second;
-      FwdRef->replaceAllUsesWith(BlockAddress::get(F, FunctionBBs[BlockIdx]));
-      FwdRef->eraseFromParent();
-    }
-
-    BlockAddrFwdRefs.erase(BAFRI);
-  }
-
   // Trim the value list down to the size it was before we parsed this function.
   ValueList.shrinkTo(ModuleValueListSize);
   MDValueList.shrinkTo(ModuleMDValueListSize);
@@ -3351,7 +3364,7 @@ std::error_code BitcodeReader::MaterializeModule(Module *M) {
 
   // Check that all block address forward references got resolved (as we
   // promised above).
-  if (!BlockAddrFwdRefs.empty())
+  if (!BasicBlockFwdRefs.empty())
     return Error(BitcodeError::NeverResolvedFunctionFromBlockAddress);
 
   // Upgrade any intrinsic calls that slipped through (should not happen!) and

lib/Bitcode/Reader/BitcodeReader.h

@@ -179,10 +179,10 @@ class BitcodeReader : public GVMaterializer {
   /// stream.
   DenseMap<Function*, uint64_t> DeferredFunctionInfo;
 
-  /// BlockAddrFwdRefs - These are blockaddr references to basic blocks.  These
+  /// These are basic blocks forward-referenced by block addresses.  They are
-  /// are resolved lazily when functions are loaded.
+  /// inserted lazily into functions when they're loaded.
-  typedef std::pair<unsigned, GlobalVariable*> BlockAddrRefTy;
+  typedef std::pair<unsigned, BasicBlock *> BasicBlockRefTy;
-  DenseMap<Function*, std::vector<BlockAddrRefTy> > BlockAddrFwdRefs;
+  DenseMap<Function *, std::vector<BasicBlockRefTy>> BasicBlockFwdRefs;
 
   /// UseRelativeIDs - Indicates that we are using a new encoding for
   /// instruction operands where most operands in the current