[llvm-commits] Patch for simplifying regions
Vu Le
vmle at ucdavis.edu
Wed Feb 2 09:46:37 PST 2011
Hi Andreas,
On Wed, Feb 2, 2011 at 1:09 AM, Andreas Simbuerger
<simbuerg at googlemail.com>wrote:
> Hi Vu,
>
> thanks for enhancing my patch :-)
>
> Am 02.02.2011 01:37, schrieb Vu Le:
> > Hi Tobias,
> >
> > On Tue, Feb 1, 2011 at 2:02 PM, Tobias Grosser
> > <grosser at fim.uni-passau.de <mailto:grosser at fim.uni-passau.de>> wrote:
> >
> > 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?
> >
> >
> > This is is Andreas' code. Do you mean if the region is modified, we just
> > print CR->getNameStr()?
>
> I thought it would be nice to have the output of both the entering and
> exiting nodes (if changed) and the region namestr (which shows entry &
> exit block). That's why I formatted this myself.
>
> >
> >
> >
> > +
> > + O<< "\n";
> > + }
> > +}
> > +
> > +void RegionSimplify::getAnalysisUsage(AnalysisUsage&AU) const {
> > + AU.addPreserved<DominatorTree> ();
> > + AU.addRequired<RegionInfo> ();
> >
> > Does this transformation preserve RegionInfo?
> >
> >
> > Yes, I think RegionInfo is also preserved.
>
> There would be no point of putting effort in updating the RegionInfo if
> we don't preserve it. If I missed to add it, sorry ;-).
>
> We need to update RegionInfo to reflect the changes.
Otherwise when you visit the next region, your pass will break.
> >
> >
> >
> > +}
> > +
> > +// 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.
> >
> >
> > I thought that we should not split regions whose entry is the function
> > entry. But I was wrong. We can split those. But I don't quite understand
> > what you mean.
> >
> >
> > +
> > + 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?
> >
> > I guess yes. The different here is if we use splitBlockPredecessors,
> > Entry is split into NewNode->Entry. The entry node of R is not still
> Entry.
> >
> > We only need to update regions whose exit is Entry.
> > Their exit must be changed into NewNode.
> > I'm not quite sure how to do that.
>
Do you have any idea how to do that?
Update Entry is simpler since it belongs to the region.
Exit node does not.
> >
> >
> > +
> > + 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.
> >
> >
> > Why would we do that?
> > Suppose another region X has entry oldExit.
> > In region R, oldExit is split into (NewExit->oldExit).
> > R is now the smallest region containing NewExit.
> >
> > I think it's still OK if the entry of X is oldExit.
> >
> >
> > +}
> > +
> > +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.
> >
> >
> > OK.
> >
> >
> >
> > +
> > + 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.
> >
> > OK.
> >
>
> I thought it would be a lot cleaner if we have the edge creation
> separated from the update code.
>
> >
> >
> > +
> > + 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.
> >
> > I tested with mysql code. Do you know how to verify that our pass
> > preserves the semantics of the program?
> >
> > Tobi
> >
> >
>
> Again I have to thank you a lot for finishing my broken patch :-)
>
> Cheers,
> Andreas
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110202/26676e59/attachment.html>
More information about the llvm-commits
mailing list