[llvm-commits] [PATCH] Add SeSeRegionInfo transform pass
Vu Le
vmle at ucdavis.edu
Mon Jan 31 15:05:35 PST 2011
Hi Tobias and Andreas,
I'm looking at Andreas' SeseRegionInfo.cpp
I think after we split the entry and exit node, we have to update
the DominatorTree since Region is based on it.
I applied the patch and came up with the problem described below.
Suppose I have a region like this. BB1 is the Entry and BB3 is the exit
\ /
BB1
/ \
| BB2
\ /
BB3
We have to split the entry BB1, since BB1 does not have a single entry edge.
The result looks like this.
\ /
BB1
|
BB1.new
/ \
| BB2
\ /
BB3
The new entry node is BB1.new.
BB1 is split into BB1->BB1.new.
We then use DT->splitBlock(BB1.new) to update the DominatorTree.
(This is similar to CodeExtractor::severSplitPHINode)
But that is where we have problems.
DominatorTree::splitBlock(NewBB) make an assumption that
NewBB has only one successor.
But in our case, BB1.new has 2 successors, BB2 and BB3.
I do think that is not a valid assumption.
Tell me what you guys think, and I'll fix this.
Thanks a lot.
Vu
> On 01/14/2011 07:06 AM, Andreas Simbuerger wrote:
> > Hi,
> >
> > this patch adds a small transform pass that converts a refined region
> > into a simple region by adding the required edges at the region entry /
>
> > exit.
>
> Thanks Andreas,
>
> > Basically it should update the RegionInfo analysis correctly, but
> > sometimes it fails to do so.
> > This requires some work on the RegionInfo::splitBlock methods.
>
> > I'll try to figure it out and send a patch for that at a later time.
> >
> > Test cases will follow in a separate patch.
>
> Here my review. Let's commit it as soon as splitBlock() is enhanced and
>
> we have got some test cases.
>
> > 0001-Add-SeSeRegionInfo-transform-pass.patch
> >
> >
> > From cf305190c65a72505408d663b0e4bf01bb7afa74 Mon Sep 17 00:00:00 2001
> > From: Andreas Simbuerger<simbuerg at ... <http://gmane.org/get-address.php?address=simbuerg%2djTxsZArgI4PmFSIgyKwhw7NAH6kLmebB%40public.gmane.org>>
>
> > Date: Fri, 14 Jan 2011 10:01:19 +0100
> > Subject: [PATCH] Add SeSeRegionInfo transform pass.
>
> As this is a transformation pass, I believe calling it "Info" is
> misleading. What about "SimplifyRegions" or "SimpleRegions"?
>
> > This adds a single entry / single exit edge transformation pass.
> > The RegionInfo analysis constructs a region tree that contains
> > of 2 types of regions:
> > * Refined regions
> > * Simple regions
>
> >
> > While simple regions only have two transitions inside
> > (1 entry, 1 exit), refined regions don't have that property.
> > However, refined regions can be transformed into simple regions
> > by merging multiple edges into a new split BasicBlock and forming
>
> > a new edge.
> > ---
> > include/llvm/InitializePasses.h | 1 +
> > include/llvm/LinkAllPasses.h | 1 +
> > include/llvm/Transforms/Scalar.h | 6 +
> > lib/Transforms/Scalar/CMakeLists.txt | 1 +
>
> > lib/Transforms/Scalar/Scalar.cpp | 1 +
> > lib/Transforms/Scalar/SeSeRegionInfo.cpp | 167 ++++++++++++++++++++++++++++++
> > 6 files changed, 177 insertions(+), 0 deletions(-)
> > create mode 100644 lib/Transforms/Scalar/SeSeRegionInfo.cpp
>
> >
> > diff --git a/include/llvm/InitializePasses.h b/include/llvm/InitializePasses.h
> > index d86b199..a50db8d 100644
> > --- a/include/llvm/InitializePasses.h
> > +++ b/include/llvm/InitializePasses.h
>
> > @@ -197,6 +197,7 @@ void initializeSROA_DFPass(PassRegistry&);
> > void initializeSROA_SSAUpPass(PassRegistry&);
> > void initializeScalarEvolutionAliasAnalysisPass(PassRegistry&);
> > void initializeScalarEvolutionPass(PassRegistry&);
>
> > +void initializeSeSeRegionInfoPass(PassRegistry&);
> > void initializeSimpleInlinerPass(PassRegistry&);
> > void initializeSimpleRegisterCoalescingPass(PassRegistry&);
> > void initializeSimplifyHalfPowrLibCallsPass(PassRegistry&);
>
> > diff --git a/include/llvm/LinkAllPasses.h b/include/llvm/LinkAllPasses.h
> > index 7dd8ebd..bb7a68a 100644
> > --- a/include/llvm/LinkAllPasses.h
> > +++ b/include/llvm/LinkAllPasses.h
> > @@ -115,6 +115,7 @@ namespace {
>
> > (void) llvm::createRegionViewerPass();
> > (void) llvm::createSCCPPass();
> > (void) llvm::createScalarReplAggregatesPass();
> > + (void) llvm::createSeSeRegionInfoPass();
>
> > (void) llvm::createSimplifyLibCallsPass();
> > (void) llvm::createSimplifyHalfPowrLibCallsPass();
> > (void) llvm::createSingleLoopExtractorPass();
> > diff --git a/include/llvm/Transforms/Scalar.h b/include/llvm/Transforms/Scalar.h
>
> > index 9a87eab..6c63eb8 100644
> > --- a/include/llvm/Transforms/Scalar.h
> > +++ b/include/llvm/Transforms/Scalar.h
> > @@ -349,6 +349,12 @@ Pass *createCorrelatedValuePropagationPass();
> > FunctionPass *createInstructionSimplifierPass();
>
> > extern char&InstructionSimplifierID;
> >
> > +//===----------------------------------------------------------------------===//
> > +//
> > +// SeSeRegionInfo - Convert refined regions to simple regions.
>
> > +//
> > +Pass *createSeSeRegionInfoPass();
> > +
> > } // End llvm namespace
> >
> > #endif
> > diff --git a/lib/Transforms/Scalar/CMakeLists.txt b/lib/Transforms/Scalar/CMakeLists.txt
>
> > index 106fb8f..836c22c 100644
> > --- a/lib/Transforms/Scalar/CMakeLists.txt
> > +++ b/lib/Transforms/Scalar/CMakeLists.txt
> > @@ -26,6 +26,7 @@ add_llvm_library(LLVMScalarOpts
> > SCCP.cpp
> > Scalar.cpp
>
> > ScalarReplAggregates.cpp
> > + SeSeRegionInfo.cpp
> > SimplifyCFGPass.cpp
> > SimplifyHalfPowrLibCalls.cpp
> > SimplifyLibCalls.cpp
> > diff --git a/lib/Transforms/Scalar/Scalar.cpp b/lib/Transforms/Scalar/Scalar.cpp
>
> > index 1d0ca7d..7ab83b9 100644
> > --- a/lib/Transforms/Scalar/Scalar.cpp
> > +++ b/lib/Transforms/Scalar/Scalar.cpp
> > @@ -53,6 +53,7 @@ void llvm::initializeScalarOpts(PassRegistry&Registry) {
> > initializeRegToMemPass(Registry);
>
> > initializeSCCPPass(Registry);
> > initializeIPSCCPPass(Registry);
> > + initializeSeSeRegionInfoPass(Registry);
> > initializeSROA_DFPass(Registry);
> > initializeSROA_SSAUpPass(Registry);
>
> > initializeCFGSimplifyPassPass(Registry);
> > diff --git a/lib/Transforms/Scalar/SeSeRegionInfo.cpp b/lib/Transforms/Scalar/SeSeRegionInfo.cpp
> > new file mode 100644
> > index 0000000..6005e64
> > --- /dev/null
>
> > +++ b/lib/Transforms/Scalar/SeSeRegionInfo.cpp
> > @@ -0,0 +1,167 @@
> > +//===- 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
> > +// into simple regions.
> > +//
> > +//===----------------------------------------------------------------------===//
>
> > +
> > +#include "llvm/Instructions.h"
> > +#include "llvm/ADT/Statistic.h"
> > +#include "llvm/Analysis/RegionPass.h"
> > +#include "llvm/Analysis/RegionInfo.h"
>
> > +#include "llvm/Transforms/Utils/BasicBlockUtils.h"
> > +
> > +#define DEBUG_TYPE "sese-regions"
> > +
> > +using namespace llvm;
> > +
> > +STATISTIC(NumEntries, "The # of created entry edges");
>
> > +STATISTIC(NumExits, "The # of created exit edges");
> > +
> > +namespace {
> > + class SeSeRegionInfo : public RegionPass {
> > + bool modified;
> > + Region *CR;
> > + BasicBlock *createSingleEntryEdge(Region *R);
>
> > + BasicBlock *createSingleExitEdge(Region *R);
> > + public:
> > + static char ID;
> > + explicit SeSeRegionInfo() : RegionPass(ID) {
> > + initializeSeSeRegionInfoPass(*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(SeSeRegionInfo, "sese-regions",
> I believe sese is difficult to understand. Can we use simple-regions for
> this?
>
> > + "Transform refined regions into simple regions", false, false)
>
> > +
> > +char SeSeRegionInfo::ID = 0;
> > +namespace llvm {
> > + Pass *createSeSeRegionInfoPass() {
> > + return new SeSeRegionInfo();
> > + }
> > +}
> > +
> > +void SeSeRegionInfo::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";
> > +
> > + O<< "\n";
> > + }
> > +}
> > +
> > +void SeSeRegionInfo::getAnalysisUsage(AnalysisUsage&AU) const {
>
> > + AU.setPreservesAll();
> Is this correct?
>
>
>
> > + AU.addRequired<RegionInfo>();
> > +}
> > +
> > +// createSingleEntryEdge - Split the entry BasicBlock of the given
> > +// region after the last PHINode to form a single entry edge.
> > +// This does not update RegionInfo analysis.
>
> > +BasicBlock *SeSeRegionInfo::createSingleEntryEdge(Region *R) {
> > + BasicBlock *BB = R->getEntry();
> > + BasicBlock::iterator SplitIt = BB->begin();
> > +
> > + while (isa<PHINode>(SplitIt))
>
> > + ++SplitIt;
> > +
> > + BasicBlock *newBB = SplitBlock(BB, SplitIt, this);
>
> > +
> > + for (BasicBlock::iterator PI = BB->begin(); isa<PHINode>(PI); ++PI) {
> > + PHINode *PN = cast<PHINode>(PI);
> > + PHINode *NPN =
> > + PHINode::Create(PN->getType(), PN->getName()+".ph", newBB->begin());
>
> > +
> > + for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE; ++PI) {
> > + if (R->contains(*PI)) {
> > + Value *V = PN->removeIncomingValue(*PI, false);
> > + NPN->addIncoming(V, *PI);
>
> > + }
> > + }
> You can get rid of a set of parenthesis.
>
> > + PN->replaceAllUsesWith(NPN);
> > + NPN->addIncoming(PN,BB);
> > + }
> > +
> > + for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE; ++PI)
>
> > + if (R->contains(*PI))
> > + (*PI)->getTerminator()->replaceUsesOfWith(BB, newBB);
> > +
> > + return newBB;
> > +}
> > +
> > +// createSingleExitEdge - Split the exit BasicBlock of the given region
>
> > +// to form a single exit edge.
> > +// This does not update RegionInfo analysis.
> > +BasicBlock *SeSeRegionInfo::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(), ".region", this);
>
> > +}
> > +
> > +bool SeSeRegionInfo::runOnRegion(Region *R, RGPassManager&RGM) {
> > + RegionInfo *RI =&getAnalysis<RegionInfo>();
> > + modified = false;
> > +
> > + CR = R;
>
> > + if (!R->isTopLevelRegion()) {
> > + BasicBlock *nBB;
> > + BasicBlock *oBB;
> Can you use more descriptive variable names?
>
> > +
> > + BasicBlock *enteringBlock = R->getEnteringBlock();
>
> > + if (!enteringBlock) {
> You can probably move the R->getEnteringBlock() into the if condition.
> enteringBlock is not used anywhere else.
>
> > + oBB = R->getEntry();
> > + nBB = createSingleEntryEdge(R);
>
> > +
> > + RI->splitBlock(nBB, oBB);
> > +
> > + modified |= true;
> > + ++NumEntries;
> > + }
> > +
> > + BasicBlock *exitingBlock = R->getExitingBlock();
> > + if (!exitingBlock) {
>
> Dito.
>
> > + oBB = R->getExit();
> > + nBB = createSingleExitEdge(R);
> > +
> > + RI->splitBlock(nBB, oBB);
> > +
> > + modified |= true;
> > + ++NumExits;
>
> > + }
> > + }
> > +
> > + return modified;
> > +}
> > -- 1.7.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110131/fd651dab/attachment.html>
More information about the llvm-commits
mailing list