Refactor - CodeExtractor : Move check for valid block to static utility and add assumption checks.

[rriddle]

Summary

Move the static function, checking to see if a block is valid, to the CodeExtractor class a public utility. This lets you actually check to see if a block is valid before trying to extract.

[rriddle]

Moving assumptions to later patch

[silvas]

River pinged me about this offline. I committed it in r276846

/Users/rriddle/Desktop/llvm/llvm/include/llvm/Transforms/Utils/CodeExtractor.h

class CodeExtractor {
     const bool AggregateArgs;
 
     // Bits of intermediate state computed at various phases of extraction.
     SetVector<BasicBlock *> Blocks;
     unsigned NumExitBlocks;
     Type *RetTy;
 
   public:
+
+    /// \brief Check to see if a block is valid for extraction.
+    ///
+    /// Blocks containing EHPads, allocas, invokes, or vastarts are not valid.
+    static bool CodeExtractor::isBlockValidForExtraction(const BasicBlock &BB);
+
     /// \brief Create a code extractor for a single basic block.
     ///
     /// In this formation, we don't require a dominator tree. The given basic
     /// block is set up for extraction.
     CodeExtractor(BasicBlock *BB, bool AggregateArgs = false);
 
     /// \brief Create a code extractor for a sequence of blocks.
     ///
private:
 
     void emitCallAndSwitchStatement(Function *newFunction,
                                     BasicBlock *newHeader,
                                     ValueSet &inputs,
                                     ValueSet &outputs);
   };
 }
 
-#endif
+#endif
 No newline at end of file

/Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Utils/CodeExtractor.cpp

 // for functions produced by the code extractor. This is useful when converting
 // extracted functions to pthread-based code, as only one argument (void*) can
 // be passed in to pthread_create().
 static cl::opt<bool>
 AggregateArgsOpt("aggregate-extracted-args", cl::Hidden,
                  cl::desc("Aggregate arguments to code-extracted functions"));
 
 /// \brief Test whether a block is valid for extraction.
-static bool isBlockValidForExtraction(const BasicBlock &BB) {
+bool CodeExtractor::isBlockValidForExtraction(const BasicBlock &BB) {
   // Landing pads must be in the function where they were inserted for cleanup.
   if (BB.isEHPad())
     return false;
 
   // Don't hoist code containing allocas, invokes, or vastarts.
   for (BasicBlock::const_iterator I = BB.begin(), E = BB.end(); I != E; ++I) {
     if (isa<AllocaInst>(I) || isa<InvokeInst>(I))
       return false;
static SetVector<BasicBlock *> buildExtractionBlockSet(IteratorT BBBegin,
   assert(BBBegin != BBEnd);
 
   // Loop over the blocks, adding them to our set-vector, and aborting with an
   // empty set if we encounter invalid blocks.
   do {
     if (!Result.insert(*BBBegin))
       llvm_unreachable("Repeated basic blocks in extraction input");
 
-    if (!isBlockValidForExtraction(**BBBegin)) {
+    if (!CodeExtractor::isBlockValidForExtraction(**BBBegin)) {
       Result.clear();
       return Result;
     }
   } while (++BBBegin != BBEnd);
 
 #ifndef NDEBUG
   for (SetVector<BasicBlock *>::iterator I = std::next(Result.begin()),
                                          E = Result.end();
Function *CodeExtractor::constructFunction(const ValueSet &inputs,
   // Create the new function
   Function *newFunction = Function::Create(funcType,
                                            GlobalValue::InternalLinkage,
                                            oldFunction->getName() + "_" +
                                            header->getName(), M);
   // If the old function is no-throw, so is the new one.
   if (oldFunction->doesNotThrow())
     newFunction->setDoesNotThrow();
-  
   newFunction->getBasicBlockList().push_back(newRootNode);
 
   // Create an iterator to name all of the arguments we inserted.
   Function::arg_iterator AI = newFunction->arg_begin();
 
   // Rewrite all users of the inputs in the extracted region to use the
   // arguments (or appropriate addressing into struct) instead.
   for (unsigned i = 0, e = inputs.size(); i != e; ++i) {

 /// the call instruction, splitting any PHI nodes in the header block as
 /// necessary.
 void CodeExtractor::
 emitCallAndSwitchStatement(Function *newFunction, BasicBlock *codeReplacer,
                            ValueSet &inputs, ValueSet &outputs) {
   // Emit a call to the new function, passing in: *pointer to struct (if
   // aggregating parameters), or plan inputs and allocated memory for outputs
   std::vector<Value*> params, StructValues, ReloadOutputs, Reloads;
-  
   LLVMContext &Context = newFunction->getContext();
 
   // Add inputs as params, or to be filled into the struct
   for (Value *input : inputs)
     if (AggregateArgs)
       StructValues.push_back(input);
     else
       params.push_back(input);
for (unsigned i = 0, e = TI->getNumSuccessors(); i != e; ++i)
                                           FirstOut+out);
                 GetElementPtrInst *GEP = GetElementPtrInst::Create(
                     StructArgTy, &*OAI, Idx, "gep_" + outputs[out]->getName(),
                     NTRet);
                 new StoreInst(outputs[out], GEP, NTRet);
               } else {
                 new StoreInst(outputs[out], &*OAI, NTRet);
               }
+              assert(DT->dominates(cast<Instruction>(outputs[out]), OldTarget) &&
+                         "Code Extractor depends on critical edges being broken");
             }
             // Advance output iterator even if we don't emit a store
             if (!AggregateArgs) ++OAI;
           }
         }
 
         // rewrite the original branch instruction with this new target
         TI->setSuccessor(i, NewTarget);
case 0:
     if (OldFnRetTy->isVoidTy()) {
       ReturnInst::Create(Context, nullptr, TheSwitch);  // Return void
     } else if (OldFnRetTy == TheSwitch->getCondition()->getType()) {
       // return what we have
       ReturnInst::Create(Context, TheSwitch->getCondition(), TheSwitch);
     } else {
       // Otherwise we must have code extracted an unwind or something, just
       // return whatever we want.
-      ReturnInst::Create(Context, 
+      ReturnInst::Create(Context,
                          Constant::getNullValue(OldFnRetTy), TheSwitch);
     }
 
     TheSwitch->eraseFromParent();
     break;
   case 1:
     // Only a single destination, change the switch into an unconditional
     // branch.
Function *CodeExtractor::extractCodeRegion() {
 
   // If we have any return instructions in the region, split those blocks so
   // that the return is not in the region.
   splitReturnBlocks();
 
   Function *oldFunction = header->getParent();
 
   // This takes place of the original loop
-  BasicBlock *codeReplacer = BasicBlock::Create(header->getContext(), 
+  BasicBlock *codeReplacer = BasicBlock::Create(header->getContext(),
                                                 "codeRepl", oldFunction,
                                                 header);
 
   // The new function needs a root node because other nodes can branch to the
   // head of the region, but the entry node of a function cannot have preds.
-  BasicBlock *newFuncRoot = BasicBlock::Create(header->getContext(), 
+  BasicBlock *newFuncRoot = BasicBlock::Create(header->getContext(),
                                                "newFuncRoot");
   newFuncRoot->getInstList().push_back(BranchInst::Create(header));
 
   // Find inputs to, outputs from the code region.
   findInputsOutputs(inputs, outputs);
 
   SmallPtrSet<BasicBlock *, 1> ExitBlocks;
   for (BasicBlock *Block : Blocks)
for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i)
         PN->setIncomingBlock(i, newFuncRoot);
   }
 
   // Look at all successors of the codeReplacer block.  If any of these blocks
   // had PHI nodes in them, we need to update the "from" block to be the code
   // replacer, not the original block in the extracted region.
   std::vector<BasicBlock*> Succs(succ_begin(codeReplacer),
                                  succ_end(codeReplacer));
-  for (unsigned i = 0, e = Succs.size(); i != e; ++i)
+  for (unsigned i = 0, e = Succs.size(); i != e; ++i) {
     for (BasicBlock::iterator I = Succs[i]->begin(); isa<PHINode>(I); ++I) {
       PHINode *PN = cast<PHINode>(I);
       std::set<BasicBlock*> ProcessedPreds;
-      for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i)
-        if (Blocks.count(PN->getIncomingBlock(i))) {
-          if (ProcessedPreds.insert(PN->getIncomingBlock(i)).second)
+      for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i) {
+        BasicBlock *IncomingBlock = PN->getIncomingBlock(i);
+        if (Blocks.count(IncomingBlock)) {
+          if (ProcessedPreds.insert(IncomingBlock).second) {
+            assert(PN->getBasicBlockIndex(codeReplacer) < 0
+                   && "Code Extractor depends on critical edges being broken");
             PN->setIncomingBlock(i, codeReplacer);
+          }
           else {
             // There were multiple entries in the PHI for this block, now there
             // is only one, so remove the duplicated entries.
             PN->removeIncomingValue(i, false);
             --i; --e;
           }
         }
       }
+    }
+  }
 
   //cerr << "NEW FUNCTION: " << *newFunction;
   //  verifyFunction(*newFunction);
 
   //  cerr << "OLD FUNCTION: " << *oldFunction;
   //  verifyFunction(*oldFunction);
 
-  DEBUG(if (verifyFunction(*newFunction)) 
+  DEBUG(if (verifyFunction(*newFunction))
         report_fatal_error("verifyFunction failed!"));
   return newFunction;
 }
 No newline at end of file