[llvm-commits] Patch for simplifying regions

[Tobias Grosser]

On 02/01/2011 02:03 PM, Vu Le wrote:
> Hi Tobias, Andreas,
> This is my patch for regionsimplify pass.
> I rename the file to RegionSimplify, change the pass name to
> -regionsimplify to conform with those of Loop.
>
> I also update the RegionInfo and DominatorTree whenever we split the
> entry or exit.
> Please give me the feedbacks.
> Thanks.
> Vu

Hi Vu,

thanks for submitting this patch. I added my comments inline.

> region-simplify.patch
>
>
> diff --git a/include/llvm/InitializePasses.h b/include/llvm/InitializePasses.h
> index 2a17c38..a3c1eaa 100644
> --- a/include/llvm/InitializePasses.h
> +++ b/include/llvm/InitializePasses.h
> @@ -192,6 +192,7 @@ void initializeRegionInfoPass(PassRegistry&);
>   void initializeRegionOnlyPrinterPass(PassRegistry&);
>   void initializeRegionOnlyViewerPass(PassRegistry&);
>   void initializeRegionPrinterPass(PassRegistry&);
> +void initializeRegionSimplifyPass(PassRegistry&);
>   void initializeRegionViewerPass(PassRegistry&);
>   void initializeRegisterCoalescerAnalysisGroup(PassRegistry&);
>   void initializeRenderMachineFunctionPass(PassRegistry&);
> diff --git a/include/llvm/LinkAllPasses.h b/include/llvm/LinkAllPasses.h
> index 69e1bd9..ea1faec 100644
> --- a/include/llvm/LinkAllPasses.h
> +++ b/include/llvm/LinkAllPasses.h
> @@ -114,7 +114,8 @@ namespace {
>         (void) llvm::createRegionInfoPass();
>         (void) llvm::createRegionOnlyPrinterPass();
>         (void) llvm::createRegionOnlyViewerPass();
> -      (void) llvm::createRegionPrinterPass();
> +      (void) llvm::createRegionPrinterPass();
Any need to change this line?

> +      (void) llvm::createRegionSimplifyPass();
>         (void) llvm::createRegionViewerPass();
>         (void) llvm::createSCCPPass();
>         (void) llvm::createScalarReplAggregatesPass();
> diff --git a/include/llvm/Transforms/Scalar.h b/include/llvm/Transforms/Scalar.h
> index 6f2a38e..e3ca06a 100644
> --- a/include/llvm/Transforms/Scalar.h
> +++ b/include/llvm/Transforms/Scalar.h
> @@ -349,6 +349,12 @@ Pass *createCorrelatedValuePropagationPass();
>   FunctionPass *createInstructionSimplifierPass();
>   extern char&InstructionSimplifierID;
>
> +//===----------------------------------------------------------------------===//
> +//
> +// RegionSimplify - Simplify refined regions, if possible.
> +//
> +Pass *createRegionSimplifyPass();
> +
>   } // End llvm namespace
>
>   #endif
> diff --git a/lib/Transforms/Scalar/CMakeLists.txt b/lib/Transforms/Scalar/CMakeLists.txt
> index 106fb8f..53fcf69 100644
> --- a/lib/Transforms/Scalar/CMakeLists.txt
> +++ b/lib/Transforms/Scalar/CMakeLists.txt
> @@ -23,6 +23,7 @@ add_llvm_library(LLVMScalarOpts
>     MemCpyOptimizer.cpp
>     Reassociate.cpp
>     Reg2Mem.cpp
> +  RegionSimplify.cpp
>     SCCP.cpp
>     Scalar.cpp
>     ScalarReplAggregates.cpp
> diff --git a/lib/Transforms/Scalar/RegionSimplify.cpp b/lib/Transforms/Scalar/RegionSimplify.cpp
> new file mode 100644
> index 0000000..c0b2770
> --- /dev/null
> +++ b/lib/Transforms/Scalar/RegionSimplify.cpp
> @@ -0,0 +1,208 @@
> +//===- SeSeRegionInfo.cpp -------------------------------------------------===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file converts refined regions detected by the RegionInfo analysis
       Convert refined regions ...

> +// into simple regions.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#include "llvm/Instructions.h"
> +#include "llvm/ADT/Statistic.h"
> +#include "llvm/Analysis/Dominators.h"
> +#include "llvm/Analysis/RegionPass.h"
> +#include "llvm/Analysis/RegionInfo.h"
> +#include "llvm/Transforms/Utils/BasicBlockUtils.h"
> +
> +#define DEBUG_TYPE "regionsimplify"
> +
> +using namespace llvm;
> +
> +STATISTIC(NumEntries, "The # of created entry edges");
                           The number of entry edges created

> +STATISTIC(NumExits, "The # of created exit edges");
dito.

> +
> +namespace {
> +class RegionSimplify: public RegionPass {
> +  bool modified;
> +  Region *CR;
> +  BasicBlock *createSingleEntryEdge(Region *R);
> +  BasicBlock *createSingleExitEdge(Region *R);
> +public:
> +  static char ID;
> +  explicit RegionSimplify() :
> +    RegionPass(ID) {
> +    initializeRegionSimplifyPass(*PassRegistry::getPassRegistry());
> +  }
> +
> +  virtual void print(raw_ostream&O, const Module *M) const;
> +
> +  virtual bool runOnRegion(Region *R, RGPassManager&RGM);
> +  virtual void getAnalysisUsage(AnalysisUsage&AU) const;
> +};
> +}
> +
> +INITIALIZE_PASS(RegionSimplify, "regionsimplify",
> +    "Transform refined regions into simple regions", false, false)
> +
> +char RegionSimplify::ID = 0;
> +namespace llvm {
> +Pass *createRegionSimplifyPass() {
> +  return new RegionSimplify();
> +}
> +}
> +
> +void RegionSimplify::print(raw_ostream&O, const Module *M) const {
> +  BasicBlock *enteringBlock;
> +  BasicBlock *exitingBlock;
> +
> +  if (modified) {
> +    enteringBlock = CR->getEnteringBlock();
> +    exitingBlock = CR->getExitingBlock();
> +
> +    O<<  "\nRegion: ["<<  CR->getNameStr()<<  "] Edges:\n";
> +    if (enteringBlock)
> +      O<<  "  Entry: ]"<<  enteringBlock->getNameStr()<<  " =>  "
> +<<  enteringBlock->getNameStr()<<  "]\n";
> +    if (exitingBlock)
> +      O<<  "  Exit: ["<<  exitingBlock->getNameStr()<<  " =>  "
> +<<  exitingBlock->getNameStr()<<  "[\n";
Can you use CR->getNameStr() instead of formatting this yourself?

> +
> +    O<<  "\n";
> +  }
> +}
> +
> +void RegionSimplify::getAnalysisUsage(AnalysisUsage&AU) const {
> +  AU.addPreserved<DominatorTree>  ();
> +  AU.addRequired<RegionInfo>  ();
Does this transformation preserve RegionInfo?

> +}
> +
> +// createSingleEntryEdge - Split the entry BasicBlock of the given
> +// region after the last PHINode to form a single entry edge.
> +// This is similar to CodeExtractor::severSplitPHINodes
> +BasicBlock *RegionSimplify::createSingleEntryEdge(Region *R) {
> +  Function *f = R->getEntry()->getParent();
> +  if (&f->getEntryBlock() == R->getEntry())
> +    return NULL; // Entry node is the function's entry blocks
Why do you need a special case for this? AS the entry node as never any 
predecessors, I would imaging there would automatically be no splitting.

> +
> +  BasicBlock *oldEntry = R->getEntry();
> +  PHINode *PN = dyn_cast<PHINode>  (oldEntry->begin());
> +  if (!PN)
> +    return NULL; // No PHI nodes.
> +
> +  BasicBlock::iterator AfterPHIs = oldEntry->getFirstNonPHI();
> +  BasicBlock *newEntry = oldEntry->splitBasicBlock(AfterPHIs,
> +      oldEntry->getName() + ".simregentry");
'simregionentry' sounds wrong. What about '.single_region_entry', 
'singleentry', ...?

> +
> +  // Okay, update dominator sets.
// Update dominator tree.

> +  if (DominatorTree *DT = getAnalysisIfAvailable<DominatorTree>()) {
> +    succ_iterator secondSucc = succ_begin(newEntry) + 1;
> +    if (secondSucc == succ_end(newEntry)) //newEntry has 1 successor
> +      DT->splitBlock(newEntry);
> +    else { // newEntry has more than 1 successor, update DT manually
> +      // oldEntry dominates newEntry.
> +      // newEntry node dominates all other nodes dominated by oldEntry.
> +      DomTreeNode *OldNode = DT->getNode(oldEntry);
> +      if (OldNode) { // don't bother if oldEntry doesn't dominates any node
> +        std::vector<DomTreeNode *>  Children;
> +        for (DomTreeNode::iterator I = OldNode->begin(), E = OldNode->end(); I
> +            != E; ++I)
> +          Children.push_back(*I);
> +
> +        DomTreeNode *NewNode = DT->addNewBlock(newEntry, oldEntry);
> +        for (std::vector<DomTreeNode *>::iterator I = Children.begin(), E =
> +            Children.end(); I != E; ++I)
> +          DT->changeImmediateDominator(*I, NewNode);
> +      }
> +    }
> +  }
> +
> +  // Loop over all of the predecessors of the old entry that are in the region,
> +  // changing them to branch to the new entry instead of the old one
> +  for (pred_iterator PI = pred_begin(oldEntry), PE = pred_end(oldEntry); PI
> +      != PE; ++PI) {
> +    if (R->contains(*PI)) {
> +      TerminatorInst *TI = (*PI)->getTerminator();
> +      TI->replaceUsesOfWith(oldEntry, newEntry);
> +    }
> +  }
> +  // just have to update the PHI nodes now, inserting PHI nodes into NewBB.
> +  for (BasicBlock::iterator PI = oldEntry->begin(); isa<PHINode>  (PI); ++PI) {
> +    PHINode *PN = cast<PHINode>  (PI);
> +    // Create a new PHI node in the new region, which has an incoming value
> +    // from oldEntry of PN.
> +    PHINode *NewPN = PHINode::Create(PN->getType(), PN->getName() + ".ph",
> +        newEntry->begin());
> +
> +    NewPN->addIncoming(PN, oldEntry);
> +
> +    // Loop over all of the incoming value in PN, moving them to NewPN if they
> +    // are from the region.
> +    for (unsigned i = 0; i != PN->getNumIncomingValues(); ++i) {
> +      BasicBlock *HasValToPN = PN->getIncomingBlock(i);
> +      if (R->contains(HasValToPN)) {
> +        NewPN->addIncoming(PN->getIncomingValue(i), HasValToPN);
> +        PN->removeIncomingValue(i);
> +        --i;
> +      }
> +    }
> +  }
Do you think we can use splitBlockPredecessors to simplify all this?

> +
> +  return newEntry;
> +}
> +
> +// createSingleExitEdge - Split the exit BasicBlock of the given region
> +// to form a single exit edge.
> +// This does not update RegionInfo analysis.
> +BasicBlock *RegionSimplify::createSingleExitEdge(Region *R) {
> +  BasicBlock *BB = R->getExit();
> +
> +  SmallVector<BasicBlock*, 4>  Preds;
> +  for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE; ++PI)
> +    if (R->contains(*PI))
> +      Preds.push_back(*PI);
> +
> +  return SplitBlockPredecessors(BB, Preds.data(), Preds.size(), ".simregexit",
> +      this);
I propose to update RegionInfo here. Copying the setRegionFor from 
runOnRegion will not be sufficient. You need to update all regions whose 
entry node was the old exit node of this region.

> +}
> +
> +bool RegionSimplify::runOnRegion(Region *R, RGPassManager&RGM) {
> +  RegionInfo *RI =&getAnalysis<RegionInfo>  ();
> +  modified = false;
> +
> +  CR = R;
> +  if (!R->isTopLevelRegion()) {
> +    BasicBlock *newBB;
> +    BasicBlock *oldBB;
> +
> +    if (!(R->getEnteringBlock())) {
> +      oldBB = R->getEntry();
> +
> +      newBB = createSingleEntryEdge(R);
> +      if (newBB) { // update RegionInfo only if we split entry successfully
> +        RI->splitBlock(newBB, oldBB);
I would put this into createSingleEntryEdge(), as you also update the 
dominance information there.

> +
> +        modified |= true;
> +        ++NumEntries;
> +      }
> +    }
> +
> +    if (!(R->getExitingBlock())) {
> +      oldBB = R->getExit();
> +      newBB = createSingleExitEdge(R);
> +
> +      RI->setRegionFor(newBB, R);
I would update the RI in the createSingleExitEdge as you also update the 
regioninto there.


> +
> +      modified |= true;
> +      ++NumExits;
> +    }
> +  }
> +
> +  return modified;
> +}
> diff --git a/lib/Transforms/Scalar/Scalar.cpp b/lib/Transforms/Scalar/Scalar.cpp
> index bf9ca6d..5d18f22 100644
> --- a/lib/Transforms/Scalar/Scalar.cpp
> +++ b/lib/Transforms/Scalar/Scalar.cpp
> @@ -51,6 +51,7 @@ void llvm::initializeScalarOpts(PassRegistry&Registry) {
>     initializeMemCpyOptPass(Registry);
>     initializeReassociatePass(Registry);
>     initializeRegToMemPass(Registry);
> +  initializeRegionSimplifyPass(Registry);
>     initializeSCCPPass(Registry);
>     initializeIPSCCPPass(Registry);
>     initializeSROA_DTPass(Registry);

Furthermore, you should add a couple of test cases for the different 
branches.

Tobi