[llvm-commits] Patch for simplifying regions
Andreas Simbuerger
simbuerg at googlemail.com
Wed Feb 2 01:09:05 PST 2011
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 ;-).
>
>
>
> +}
> +
> +// 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.
>
>
> +
> + 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
More information about the llvm-commits
mailing list