Hi Andreas,<br><br><div class="gmail_quote">On Wed, Feb 2, 2011 at 1:09 AM, Andreas Simbuerger <span dir="ltr"><<a href="mailto:simbuerg@googlemail.com">simbuerg@googlemail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Hi Vu,<br>
<br>
thanks for enhancing my patch :-)<br>
<br>
Am 02.02.2011 01:37, schrieb Vu Le:<br>
<div class="im">> Hi Tobias,<br>
><br>
> On Tue, Feb 1, 2011 at 2:02 PM, Tobias Grosser<br>
</div><div><div></div><div class="h5">> <<a href="mailto:grosser@fim.uni-passau.de">grosser@fim.uni-passau.de</a> <mailto:<a href="mailto:grosser@fim.uni-passau.de">grosser@fim.uni-passau.de</a>>> wrote:<br>
><br>
> On 02/01/2011 02:03 PM, Vu Le wrote:<br>
><br>
> Hi Tobias, Andreas,<br>
> This is my patch for regionsimplify pass.<br>
> I rename the file to RegionSimplify, change the pass name to<br>
> -regionsimplify to conform with those of Loop.<br>
><br>
> I also update the RegionInfo and DominatorTree whenever we split the<br>
> entry or exit.<br>
> Please give me the feedbacks.<br>
> Thanks.<br>
> Vu<br>
><br>
><br>
> Hi Vu,<br>
><br>
> thanks for submitting this patch. I added my comments inline.<br>
><br>
> region-simplify.patch<br>
><br>
><br>
> diff --git a/include/llvm/InitializePasses.h<br>
> b/include/llvm/InitializePasses.h<br>
> index 2a17c38..a3c1eaa 100644<br>
> --- a/include/llvm/InitializePasses.h<br>
> +++ b/include/llvm/InitializePasses.h<br>
> @@ -192,6 +192,7 @@ void initializeRegionInfoPass(PassRegistry&);<br>
> void initializeRegionOnlyPrinterPass(PassRegistry&);<br>
> void initializeRegionOnlyViewerPass(PassRegistry&);<br>
> void initializeRegionPrinterPass(PassRegistry&);<br>
> +void initializeRegionSimplifyPass(PassRegistry&);<br>
> void initializeRegionViewerPass(PassRegistry&);<br>
> void initializeRegisterCoalescerAnalysisGroup(PassRegistry&);<br>
> void initializeRenderMachineFunctionPass(PassRegistry&);<br>
> diff --git a/include/llvm/LinkAllPasses.h<br>
> b/include/llvm/LinkAllPasses.h<br>
> index 69e1bd9..ea1faec 100644<br>
> --- a/include/llvm/LinkAllPasses.h<br>
> +++ b/include/llvm/LinkAllPasses.h<br>
> @@ -114,7 +114,8 @@ namespace {<br>
> (void) llvm::createRegionInfoPass();<br>
> (void) llvm::createRegionOnlyPrinterPass();<br>
> (void) llvm::createRegionOnlyViewerPass();<br>
> - (void) llvm::createRegionPrinterPass();<br>
> + (void) llvm::createRegionPrinterPass();<br>
><br>
> Any need to change this line?<br>
><br>
> + (void) llvm::createRegionSimplifyPass();<br>
> (void) llvm::createRegionViewerPass();<br>
> (void) llvm::createSCCPPass();<br>
> (void) llvm::createScalarReplAggregatesPass();<br>
> diff --git a/include/llvm/Transforms/Scalar.h<br>
> b/include/llvm/Transforms/Scalar.h<br>
> index 6f2a38e..e3ca06a 100644<br>
> --- a/include/llvm/Transforms/Scalar.h<br>
> +++ b/include/llvm/Transforms/Scalar.h<br>
> @@ -349,6 +349,12 @@ Pass *createCorrelatedValuePropagationPass();<br>
> FunctionPass *createInstructionSimplifierPass();<br>
> extern char&InstructionSimplifierID;<br>
><br>
> +//===----------------------------------------------------------------------===//<br>
> +//<br>
> +// RegionSimplify - Simplify refined regions, if possible.<br>
> +//<br>
> +Pass *createRegionSimplifyPass();<br>
> +<br>
> } // End llvm namespace<br>
><br>
> #endif<br>
> diff --git a/lib/Transforms/Scalar/CMakeLists.txt<br>
> b/lib/Transforms/Scalar/CMakeLists.txt<br>
> index 106fb8f..53fcf69 100644<br>
> --- a/lib/Transforms/Scalar/CMakeLists.txt<br>
> +++ b/lib/Transforms/Scalar/CMakeLists.txt<br>
> @@ -23,6 +23,7 @@ add_llvm_library(LLVMScalarOpts<br>
> MemCpyOptimizer.cpp<br>
> Reassociate.cpp<br>
> Reg2Mem.cpp<br>
> + RegionSimplify.cpp<br>
> SCCP.cpp<br>
> Scalar.cpp<br>
> ScalarReplAggregates.cpp<br>
> diff --git a/lib/Transforms/Scalar/RegionSimplify.cpp<br>
> b/lib/Transforms/Scalar/RegionSimplify.cpp<br>
> new file mode 100644<br>
> index 0000000..c0b2770<br>
> --- /dev/null<br>
> +++ b/lib/Transforms/Scalar/RegionSimplify.cpp<br>
> @@ -0,0 +1,208 @@<br>
> +//===- SeSeRegionInfo.cpp<br>
> -------------------------------------------------===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois<br>
> Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +//<br>
> +// This file converts refined regions detected by the<br>
> RegionInfo analysis<br>
><br>
> Convert refined regions ...<br>
><br>
> +// into simple regions.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +#include "llvm/Instructions.h"<br>
> +#include "llvm/ADT/Statistic.h"<br>
> +#include "llvm/Analysis/Dominators.h"<br>
> +#include "llvm/Analysis/RegionPass.h"<br>
> +#include "llvm/Analysis/RegionInfo.h"<br>
> +#include "llvm/Transforms/Utils/BasicBlockUtils.h"<br>
> +<br>
> +#define DEBUG_TYPE "regionsimplify"<br>
> +<br>
> +using namespace llvm;<br>
> +<br>
> +STATISTIC(NumEntries, "The # of created entry edges");<br>
><br>
> The number of entry edges created<br>
><br>
> +STATISTIC(NumExits, "The # of created exit edges");<br>
><br>
> dito.<br>
><br>
> +<br>
> +namespace {<br>
> +class RegionSimplify: public RegionPass {<br>
> + bool modified;<br>
> + Region *CR;<br>
> + BasicBlock *createSingleEntryEdge(Region *R);<br>
> + BasicBlock *createSingleExitEdge(Region *R);<br>
> +public:<br>
> + static char ID;<br>
> + explicit RegionSimplify() :<br>
> + RegionPass(ID) {<br>
> + initializeRegionSimplifyPass(*PassRegistry::getPassRegistry());<br>
> + }<br>
> +<br>
> + virtual void print(raw_ostream&O, const Module *M) const;<br>
> +<br>
> + virtual bool runOnRegion(Region *R, RGPassManager&RGM);<br>
> + virtual void getAnalysisUsage(AnalysisUsage&AU) const;<br>
> +};<br>
> +}<br>
> +<br>
> +INITIALIZE_PASS(RegionSimplify, "regionsimplify",<br>
> + "Transform refined regions into simple regions", false, false)<br>
> +<br>
> +char RegionSimplify::ID = 0;<br>
> +namespace llvm {<br>
> +Pass *createRegionSimplifyPass() {<br>
> + return new RegionSimplify();<br>
> +}<br>
> +}<br>
> +<br>
> +void RegionSimplify::print(raw_ostream&O, const Module *M) const {<br>
> + BasicBlock *enteringBlock;<br>
> + BasicBlock *exitingBlock;<br>
> +<br>
> + if (modified) {<br>
> + enteringBlock = CR->getEnteringBlock();<br>
> + exitingBlock = CR->getExitingBlock();<br>
> +<br>
> + O<< "\nRegion: ["<< CR->getNameStr()<< "] Edges:\n";<br>
> + if (enteringBlock)<br>
> + O<< " Entry: ]"<< enteringBlock->getNameStr()<< " => "<br>
> +<< enteringBlock->getNameStr()<< "]\n";<br>
> + if (exitingBlock)<br>
> + O<< " Exit: ["<< exitingBlock->getNameStr()<< " => "<br>
> +<< exitingBlock->getNameStr()<< "[\n";<br>
><br>
> Can you use CR->getNameStr() instead of formatting this yourself?<br>
><br>
><br>
> This is is Andreas' code. Do you mean if the region is modified, we just<br>
> print CR->getNameStr()?<br>
<br>
</div></div>I thought it would be nice to have the output of both the entering and<br>
exiting nodes (if changed) and the region namestr (which shows entry &<br>
exit block). That's why I formatted this myself.<br>
<div class="im"><br>
><br>
><br>
><br>
> +<br>
> + O<< "\n";<br>
> + }<br>
> +}<br>
> +<br>
> +void RegionSimplify::getAnalysisUsage(AnalysisUsage&AU) const {<br>
> + AU.addPreserved<DominatorTree> ();<br>
> + AU.addRequired<RegionInfo> ();<br>
><br>
> Does this transformation preserve RegionInfo?<br>
><br>
><br>
> Yes, I think RegionInfo is also preserved.<br>
<br>
</div>There would be no point of putting effort in updating the RegionInfo if<br>
we don't preserve it. If I missed to add it, sorry ;-).<br>
<div><div></div><div class="h5"><br></div></div></blockquote><div>We need to update RegionInfo to reflect the changes.<br>Otherwise when you visit the next region, your pass will break.<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div><div class="h5">
><br>
><br>
><br>
> +}<br>
> +<br>
> +// createSingleEntryEdge - Split the entry BasicBlock of the given<br>
> +// region after the last PHINode to form a single entry edge.<br>
> +// This is similar to CodeExtractor::severSplitPHINodes<br>
> +BasicBlock *RegionSimplify::createSingleEntryEdge(Region *R) {<br>
> + Function *f = R->getEntry()->getParent();<br>
> + if (&f->getEntryBlock() == R->getEntry())<br>
> + return NULL; // Entry node is the function's entry blocks<br>
><br>
> Why do you need a special case for this? AS the entry node as never<br>
> any predecessors, I would imaging there would automatically be no<br>
> splitting.<br>
><br>
><br>
> I thought that we should not split regions whose entry is the function<br>
> entry. But I was wrong. We can split those. But I don't quite understand<br>
> what you mean.<br>
><br>
><br>
> +<br>
> + BasicBlock *oldEntry = R->getEntry();<br>
> + PHINode *PN = dyn_cast<PHINode> (oldEntry->begin());<br>
> + if (!PN)<br>
> + return NULL; // No PHI nodes.<br>
> +<br>
> + BasicBlock::iterator AfterPHIs = oldEntry->getFirstNonPHI();<br>
> + BasicBlock *newEntry = oldEntry->splitBasicBlock(AfterPHIs,<br>
> + oldEntry->getName() + ".simregentry");<br>
><br>
> 'simregionentry' sounds wrong. What about '.single_region_entry',<br>
> 'singleentry', ...?<br>
><br>
> +<br>
> + // Okay, update dominator sets.<br>
><br>
> // Update dominator tree.<br>
><br>
> + if (DominatorTree *DT =<br>
> getAnalysisIfAvailable<DominatorTree>()) {<br>
> + succ_iterator secondSucc = succ_begin(newEntry) + 1;<br>
> + if (secondSucc == succ_end(newEntry)) //newEntry has 1<br>
> successor<br>
> + DT->splitBlock(newEntry);<br>
> + else { // newEntry has more than 1 successor, update DT<br>
> manually<br>
> + // oldEntry dominates newEntry.<br>
> + // newEntry node dominates all other nodes dominated by<br>
> oldEntry.<br>
> + DomTreeNode *OldNode = DT->getNode(oldEntry);<br>
> + if (OldNode) { // don't bother if oldEntry doesn't<br>
> dominates any node<br>
> + std::vector<DomTreeNode *> Children;<br>
> + for (DomTreeNode::iterator I = OldNode->begin(), E =<br>
> OldNode->end(); I<br>
> + != E; ++I)<br>
> + Children.push_back(*I);<br>
> +<br>
> + DomTreeNode *NewNode = DT->addNewBlock(newEntry, oldEntry);<br>
> + for (std::vector<DomTreeNode *>::iterator I =<br>
> Children.begin(), E =<br>
> + Children.end(); I != E; ++I)<br>
> + DT->changeImmediateDominator(*I, NewNode);<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + // Loop over all of the predecessors of the old entry that<br>
> are in the region,<br>
> + // changing them to branch to the new entry instead of the<br>
> old one<br>
> + for (pred_iterator PI = pred_begin(oldEntry), PE =<br>
> pred_end(oldEntry); PI<br>
> + != PE; ++PI) {<br>
> + if (R->contains(*PI)) {<br>
> + TerminatorInst *TI = (*PI)->getTerminator();<br>
> + TI->replaceUsesOfWith(oldEntry, newEntry);<br>
> + }<br>
> + }<br>
> + // just have to update the PHI nodes now, inserting PHI nodes<br>
> into NewBB.<br>
> + for (BasicBlock::iterator PI = oldEntry->begin();<br>
> isa<PHINode> (PI); ++PI) {<br>
> + PHINode *PN = cast<PHINode> (PI);<br>
> + // Create a new PHI node in the new region, which has an<br>
> incoming value<br>
> + // from oldEntry of PN.<br>
> + PHINode *NewPN = PHINode::Create(PN->getType(),<br>
> PN->getName() + ".ph",<br>
> + newEntry->begin());<br>
> +<br>
> + NewPN->addIncoming(PN, oldEntry);<br>
> +<br>
> + // Loop over all of the incoming value in PN, moving them<br>
> to NewPN if they<br>
> + // are from the region.<br>
> + for (unsigned i = 0; i != PN->getNumIncomingValues(); ++i) {<br>
> + BasicBlock *HasValToPN = PN->getIncomingBlock(i);<br>
> + if (R->contains(HasValToPN)) {<br>
> + NewPN->addIncoming(PN->getIncomingValue(i), HasValToPN);<br>
> + PN->removeIncomingValue(i);<br>
> + --i;<br>
> + }<br>
> + }<br>
> + }<br>
><br>
> Do you think we can use splitBlockPredecessors to simplify all this?<br>
><br>
> I guess yes. The different here is if we use splitBlockPredecessors,<br>
> Entry is split into NewNode->Entry. The entry node of R is not still Entry.<br>
><br>
> We only need to update regions whose exit is Entry.<br>
> Their exit must be changed into NewNode.<br>
> I'm not quite sure how to do that.<br></div></div></blockquote><div>Do you have any idea how to do that?<br>Update Entry is simpler since it belongs to the region.<br>Exit node does not.<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div><div class="h5">
><br>
><br>
> +<br>
> + return newEntry;<br>
> +}<br>
> +<br>
> +// createSingleExitEdge - Split the exit BasicBlock of the<br>
> given region<br>
> +// to form a single exit edge.<br>
> +// This does not update RegionInfo analysis.<br>
> +BasicBlock *RegionSimplify::createSingleExitEdge(Region *R) {<br>
> + BasicBlock *BB = R->getExit();<br>
> +<br>
> + SmallVector<BasicBlock*, 4> Preds;<br>
> + for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI<br>
> != PE; ++PI)<br>
> + if (R->contains(*PI))<br>
> + Preds.push_back(*PI);<br>
> +<br>
> + return SplitBlockPredecessors(BB, Preds.data(), Preds.size(),<br>
> ".simregexit",<br>
> + this);<br>
><br>
> I propose to update RegionInfo here. Copying the setRegionFor from<br>
> runOnRegion will not be sufficient. You need to update all regions<br>
> whose entry node was the old exit node of this region.<br>
><br>
><br>
> Why would we do that?<br>
> Suppose another region X has entry oldExit.<br>
> In region R, oldExit is split into (NewExit->oldExit).<br>
> R is now the smallest region containing NewExit.<br>
><br>
> I think it's still OK if the entry of X is oldExit.<br>
><br>
><br>
> +}<br>
> +<br>
> +bool RegionSimplify::runOnRegion(Region *R, RGPassManager&RGM) {<br>
> + RegionInfo *RI =&getAnalysis<RegionInfo> ();<br>
> + modified = false;<br>
> +<br>
> + CR = R;<br>
> + if (!R->isTopLevelRegion()) {<br>
> + BasicBlock *newBB;<br>
> + BasicBlock *oldBB;<br>
> +<br>
> + if (!(R->getEnteringBlock())) {<br>
> + oldBB = R->getEntry();<br>
> +<br>
> + newBB = createSingleEntryEdge(R);<br>
> + if (newBB) { // update RegionInfo only if we split entry<br>
> successfully<br>
> + RI->splitBlock(newBB, oldBB);<br>
><br>
> I would put this into createSingleEntryEdge(), as you also update<br>
> the dominance information there.<br>
><br>
><br>
> OK.<br>
><br>
><br>
><br>
> +<br>
> + modified |= true;<br>
> + ++NumEntries;<br>
> + }<br>
> + }<br>
> +<br>
> + if (!(R->getExitingBlock())) {<br>
> + oldBB = R->getExit();<br>
> + newBB = createSingleExitEdge(R);<br>
> +<br>
> + RI->setRegionFor(newBB, R);<br>
><br>
> I would update the RI in the createSingleExitEdge as you also update<br>
> the regioninto there.<br>
><br>
> OK.<br>
><br>
<br>
</div></div>I thought it would be a lot cleaner if we have the edge creation<br>
separated from the update code.<br>
<div class="im"><br>
><br>
><br>
> +<br>
> + modified |= true;<br>
> + ++NumExits;<br>
> + }<br>
> + }<br>
> +<br>
> + return modified;<br>
> +}<br>
> diff --git a/lib/Transforms/Scalar/Scalar.cpp<br>
> b/lib/Transforms/Scalar/Scalar.cpp<br>
> index bf9ca6d..5d18f22 100644<br>
> --- a/lib/Transforms/Scalar/Scalar.cpp<br>
> +++ b/lib/Transforms/Scalar/Scalar.cpp<br>
> @@ -51,6 +51,7 @@ void<br>
> llvm::initializeScalarOpts(PassRegistry&Registry) {<br>
> initializeMemCpyOptPass(Registry);<br>
> initializeReassociatePass(Registry);<br>
> initializeRegToMemPass(Registry);<br>
> + initializeRegionSimplifyPass(Registry);<br>
> initializeSCCPPass(Registry);<br>
> initializeIPSCCPPass(Registry);<br>
> initializeSROA_DTPass(Registry);<br>
><br>
><br>
> Furthermore, you should add a couple of test cases for the different<br>
> branches.<br>
><br>
> I tested with mysql code. Do you know how to verify that our pass<br>
> preserves the semantics of the program?<br>
><br>
> Tobi<br>
><br>
><br>
<br>
</div>Again I have to thank you a lot for finishing my broken patch :-)<br>
<br>
Cheers,<br>
<font color="#888888">Andreas<br>
<br>
</font></blockquote></div><br>