[CodeExtractor] Remove a bunch of unneeded constructors

[davide]

Summary

Found while debugging a partial inliner crash.
As a follow up, I'd like to make CodeExtractor always requiring a DominatorTree (as, it seems, pretty much every consumer pass one anyway). As you're doing some work on the partial inliner, I wanted to make sure you're OK with these changes before submitting, hence this review thread.

[davidxl]

lgtm.

by the way, what is your PM setup for partial inlining?

[davide]

In D32305#732439, @davidxl wrote:

lgtm.

by the way, what is your PM setup for partial inlining?

Unfortunately, once I start playing with it, I hit this crash and I started debugging, so I didn't had yet time to put at different places in the pipeline.
You have already tried some configurations? Aside, I noticed partial inlining isn't using BB frequency informations (which I found odd, but probably it's because nobody cared).
I imagine at least a simple heuristic where you split functions comparing as GCC does, IIRC (or at least, used to do at some point).
There was some initial work on this, but, alas, it's unfinished https://reviews.llvm.org/D22744

[davide]

In D32305#732492, @davide wrote:

In D32305#732439, @davidxl wrote:

lgtm.

by the way, what is your PM setup for partial inlining?

Unfortunately, once I start playing with it, I hit this crash and I started debugging, so I didn't had yet time to put at different places in the pipeline.
You have already tried some configurations? Aside, I noticed partial inlining isn't using BB frequency informations (which I found odd, but probably it's because nobody cared).
I imagine at least a simple heuristic where you split functions comparing as GCC does, IIRC (or at least, used to do at some point).

s/comparing/looking at the frequencies of the BBs compared to the frequency of the entry block/
I mean https://gcc.gnu.org/ml/gcc-patches/2010-06/msg02143.html

There was some initial work on this, but, alas, it's unfinished https://reviews.llvm.org/D22744

llvm/trunk/include/llvm/Transforms/Utils/CodeExtractor.h

template <typename T> class ArrayRef;
 
   public:
 
     /// \brief Check to see if a block is valid for extraction.
     ///
     /// Blocks containing EHPads, allocas, invokes, or vastarts are not valid.
     static bool 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,
-                  BlockFrequencyInfo *BFI = nullptr,
-                  BranchProbabilityInfo *BPI = nullptr);
-
     /// \brief Create a code extractor for a sequence of blocks.
     ///
     /// Given a sequence of basic blocks where the first block in the sequence
     /// dominates the rest, prepare a code extractor object for pulling this
     /// sequence out into its new function. When a DominatorTree is also given,
     /// extra checking and transformations are enabled.
     CodeExtractor(ArrayRef<BasicBlock *> BBs, DominatorTree *DT = nullptr,
                   bool AggregateArgs = false, BlockFrequencyInfo *BFI = nullptr,
                   BranchProbabilityInfo *BPI = nullptr);
 
     /// \brief Create a code extractor for a loop body.
     ///
     /// Behaves just like the generic code sequence constructor, but uses the
     /// block sequence of the loop.
     CodeExtractor(DominatorTree &DT, Loop &L, bool AggregateArgs = false,
                   BlockFrequencyInfo *BFI = nullptr,
                   BranchProbabilityInfo *BPI = nullptr);
 
-    /// \brief Create a code extractor for a region node.
-    ///
-    /// Behaves just like the generic code sequence constructor, but uses the
-    /// block sequence of the region node passed in.
-    CodeExtractor(DominatorTree &DT, const RegionNode &RN,
-                  bool AggregateArgs = false, BlockFrequencyInfo *BFI = nullptr,
-                  BranchProbabilityInfo *BPI = nullptr);
-
     /// \brief Perform the extraction, returning the new function.
     ///
     /// Returns zero when called on a CodeExtractor instance where isEligible
     /// returns false.
     Function *extractCodeRegion();
 
     /// \brief Test whether this code extractor is eligible.
     ///

llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp

 }
 
 /// \brief Helper to call buildExtractionBlockSet with an ArrayRef.
 static SetVector<BasicBlock *>
 buildExtractionBlockSet(ArrayRef<BasicBlock *> BBs) {
   return buildExtractionBlockSet(BBs.begin(), BBs.end());
 }
 
-/// \brief Helper to call buildExtractionBlockSet with a RegionNode.
-static SetVector<BasicBlock *>
-buildExtractionBlockSet(const RegionNode &RN) {
-  if (!RN.isSubRegion())
-    // Just a single BasicBlock.
-    return buildExtractionBlockSet(RN.getNodeAs<BasicBlock>());
-
-  const Region &R = *RN.getNodeAs<Region>();
-
-  return buildExtractionBlockSet(R.block_begin(), R.block_end());
-}
-
-CodeExtractor::CodeExtractor(BasicBlock *BB, bool AggregateArgs,
-                             BlockFrequencyInfo *BFI,
-                             BranchProbabilityInfo *BPI)
-    : DT(nullptr), AggregateArgs(AggregateArgs || AggregateArgsOpt), BFI(BFI),
-      BPI(BPI), Blocks(buildExtractionBlockSet(BB)), NumExitBlocks(~0U) {}
-
 CodeExtractor::CodeExtractor(ArrayRef<BasicBlock *> BBs, DominatorTree *DT,
                              bool AggregateArgs, BlockFrequencyInfo *BFI,
                              BranchProbabilityInfo *BPI)
     : DT(DT), AggregateArgs(AggregateArgs || AggregateArgsOpt), BFI(BFI),
       BPI(BPI), Blocks(buildExtractionBlockSet(BBs)), NumExitBlocks(~0U) {}
 
 CodeExtractor::CodeExtractor(DominatorTree &DT, Loop &L, bool AggregateArgs,
                              BlockFrequencyInfo *BFI,
                              BranchProbabilityInfo *BPI)
     : DT(&DT), AggregateArgs(AggregateArgs || AggregateArgsOpt), BFI(BFI),
       BPI(BPI), Blocks(buildExtractionBlockSet(L.getBlocks())),
       NumExitBlocks(~0U) {}
 
-CodeExtractor::CodeExtractor(DominatorTree &DT, const RegionNode &RN,
-                             bool AggregateArgs, BlockFrequencyInfo *BFI,
-                             BranchProbabilityInfo *BPI)
-    : DT(&DT), AggregateArgs(AggregateArgs || AggregateArgsOpt), BFI(BFI),
-      BPI(BPI), Blocks(buildExtractionBlockSet(RN)), NumExitBlocks(~0U) {}
-
 /// definedInRegion - Return true if the specified value is defined in the
 /// extracted region.
 static bool definedInRegion(const SetVector<BasicBlock *> &Blocks, Value *V) {
   if (Instruction *I = dyn_cast<Instruction>(V))
     if (Blocks.count(I->getParent()))
       return true;
   return false;
 }