[llvm-commits] Patch for simplifying regions
Vu Le
vmle at ucdavis.edu
Tue Feb 1 16:37:58 PST 2011
Hi Tobias,
On Tue, Feb 1, 2011 at 2:02 PM, Tobias Grosser <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()?
>
> +
>> + 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.
>
> +}
>> +
>> +// 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.
>
> +
>> + 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110201/f21f745a/attachment.html>
More information about the llvm-commits
mailing list