Hi Tobias,<br><br><div class="gmail_quote">On Tue, Feb 1, 2011 at 2:02 PM, Tobias Grosser <span dir="ltr"><<a href="mailto:grosser@fim.uni-passau.de">grosser@fim.uni-passau.de</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;">
<div><div></div><div class="h5">On 02/01/2011 02:03 PM, Vu Le 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 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>
</blockquote>
<br></div></div>
Hi Vu,<br>
<br>
thanks for submitting this patch. I added my comments inline.<br>
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
region-simplify.patch<br>
<br>
<br>
diff --git a/include/llvm/InitializePasses.h 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 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>
</blockquote>
Any need to change this line?<br>
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+ (void) llvm::createRegionSimplifyPass();<br>
(void) llvm::createRegionViewerPass();<br>
(void) llvm::createSCCPPass();<br>
(void) llvm::createScalarReplAggregatesPass();<br>
diff --git a/include/llvm/Transforms/Scalar.h 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 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 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>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+//<br>
+// This file converts refined regions detected by the RegionInfo analysis<br>
</blockquote>
Convert refined regions ...<br>
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+// 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>
</blockquote>
The number of entry edges created<br>
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+STATISTIC(NumExits, "The # of created exit edges");<br>
</blockquote>
dito.<br>
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+<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>
</blockquote>
Can you use CR->getNameStr() instead of formatting this yourself?<br></blockquote><div><br>This is is Andreas' code. Do you mean if the region is modified, we just print CR->getNameStr()?<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;">
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+<br>
+ O<< "\n";<br>
+ }<br>
+}<br>
+<br>
+void RegionSimplify::getAnalysisUsage(AnalysisUsage&AU) const {<br>
+ AU.addPreserved<DominatorTree> ();<br>
+ AU.addRequired<RegionInfo> ();<br>
</blockquote>
Does this transformation preserve RegionInfo?<br></blockquote><div><br>Yes, I think RegionInfo is also preserved.<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;">
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+}<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>
</blockquote>
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.<br></blockquote><div><br>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.<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;">
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+<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>
</blockquote>
'simregionentry' sounds wrong. What about '.single_region_entry', 'singleentry', ...?<br>
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+<br>
+ // Okay, update dominator sets.<br>
</blockquote>
// Update dominator tree.<br>
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+ if (DominatorTree *DT = getAnalysisIfAvailable<DominatorTree>()) {<br>
+ succ_iterator secondSucc = succ_begin(newEntry) + 1;<br>
+ if (secondSucc == succ_end(newEntry)) //newEntry has 1 successor<br>
+ DT->splitBlock(newEntry);<br>
+ else { // newEntry has more than 1 successor, update DT manually<br>
+ // oldEntry dominates newEntry.<br>
+ // newEntry node dominates all other nodes dominated by oldEntry.<br>
+ DomTreeNode *OldNode = DT->getNode(oldEntry);<br>
+ if (OldNode) { // don't bother if oldEntry doesn't dominates any node<br>
+ std::vector<DomTreeNode *> Children;<br>
+ for (DomTreeNode::iterator I = OldNode->begin(), E = 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 = 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 are in the region,<br>
+ // changing them to branch to the new entry instead of the old one<br>
+ for (pred_iterator PI = pred_begin(oldEntry), PE = 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 into NewBB.<br>
+ for (BasicBlock::iterator PI = oldEntry->begin(); isa<PHINode> (PI); ++PI) {<br>
+ PHINode *PN = cast<PHINode> (PI);<br>
+ // Create a new PHI node in the new region, which has an incoming value<br>
+ // from oldEntry of PN.<br>
+ PHINode *NewPN = PHINode::Create(PN->getType(), 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 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>
</blockquote>
Do you think we can use splitBlockPredecessors to simplify all this?<br></blockquote><div>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.<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><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+<br>
+ return newEntry;<br>
+}<br>
+<br>
+// createSingleExitEdge - Split the exit BasicBlock of the 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 != PE; ++PI)<br>
+ if (R->contains(*PI))<br>
+ Preds.push_back(*PI);<br>
+<br>
+ return SplitBlockPredecessors(BB, Preds.data(), Preds.size(), ".simregexit",<br>
+ this);<br>
</blockquote>
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.<br></blockquote><div><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></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+}<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 successfully<br>
+ RI->splitBlock(newBB, oldBB);<br>
</blockquote>
I would put this into createSingleEntryEdge(), as you also update the dominance information there.<br></blockquote><div><br>OK.<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;">
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+<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>
</blockquote>
I would update the RI in the createSingleExitEdge as you also update the regioninto there.<br>
<br></blockquote><div>OK.<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;">
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+<br>
+ modified |= true;<br>
+ ++NumExits;<br>
+ }<br>
+ }<br>
+<br>
+ return modified;<br>
+}<br>
diff --git a/lib/Transforms/Scalar/Scalar.cpp 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 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>
</blockquote>
<br>
Furthermore, you should add a couple of test cases for the different branches.<br>
<br></blockquote><div>I tested with mysql code. Do you know how to verify that our pass preserves the semantics of the program? <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Tobi<br>
</blockquote></div><br>