Hi Tobias and Andreas,<br>I'm looking at Andreas' SeseRegionInfo.cpp<br><br>I think after we split the entry and exit node, we have to update<br>the DominatorTree since Region is based on it.<br><br>I applied the patch and came up with the problem described below.<br>
Suppose I have a region like this. BB1 is the Entry and BB3 is the exit<br> \ /<br> BB1<br> / \<br> | BB2<br> \ /<br> BB3<br>We have to split the entry BB1, since BB1 does not have a single entry edge.<br>
The result looks like this.<br> \ /<br>
BB1<br> |<br> BB1.new<br>
/ \<br>
| BB2<br>
\ /<br>
BB3<br>
The new entry node is BB1.new.<br>BB1 is split into BB1->BB1.new.<br>We then use DT->splitBlock(BB1.new) to update the DominatorTree.<br>(This is similar to CodeExtractor::severSplitPHINode)<br><br>But that is where we have problems.<br>
DominatorTree::splitBlock(NewBB) make an assumption that<br>NewBB has only one successor.<br><br>But in our case, BB1.new has 2 successors, BB2 and BB3.<br>I do think that is not a valid assumption.<br>Tell me what you guys think, and I'll fix this.<br>
Thanks a lot.<br>Vu<br> <br><blockquote style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;" class="gmail_quote">
<pre>On 01/14/2011 07:06 AM, Andreas Simbuerger wrote:<br>> Hi,<br>><br>> this patch adds a small transform pass that converts a refined region<br>> into a simple region by adding the required edges at the region entry /<br>
> exit.<br><br>Thanks Andreas,<br><br>> Basically it should update the RegionInfo analysis correctly, but<br>> sometimes it fails to do so.<br>> This requires some work on the RegionInfo::splitBlock methods.<br>
> I'll try to figure it out and send a patch for that at a later time.<br> ><br>> Test cases will follow in a separate patch.<br><br>Here my review. Let's commit it as soon as splitBlock() is enhanced and<br>
we have got some test cases.<br><br>> 0001-Add-SeSeRegionInfo-transform-pass.patch<br>><br>><br>> From cf305190c65a72505408d663b0e4bf01bb7afa74 Mon Sep 17 00:00:00 2001<br>> From: Andreas Simbuerger<<a href="http://gmane.org/get-address.php?address=simbuerg%2djTxsZArgI4PmFSIgyKwhw7NAH6kLmebB%40public.gmane.org" rel="nofollow" target="_blank">simbuerg@...</a>><br>
> Date: Fri, 14 Jan 2011 10:01:19 +0100<br>> Subject: [PATCH] Add SeSeRegionInfo transform pass.<br><br>As this is a transformation pass, I believe calling it "Info" is <br>misleading. What about "SimplifyRegions" or "SimpleRegions"?<br>
<br>> This adds a single entry / single exit edge transformation pass.<br>> The RegionInfo analysis constructs a region tree that contains<br>> of 2 types of regions:<br>> * Refined regions<br>> * Simple regions<br>
><br>> While simple regions only have two transitions inside<br>> (1 entry, 1 exit), refined regions don't have that property.<br>> However, refined regions can be transformed into simple regions<br>> by merging multiple edges into a new split BasicBlock and forming<br>
> a new edge.<br>> ---<br>> include/llvm/InitializePasses.h | 1 +<br>> include/llvm/LinkAllPasses.h | 1 +<br>> include/llvm/Transforms/Scalar.h | 6 +<br>> lib/Transforms/Scalar/CMakeLists.txt | 1 +<br>
> lib/Transforms/Scalar/Scalar.cpp | 1 +<br>> lib/Transforms/Scalar/SeSeRegionInfo.cpp | 167 ++++++++++++++++++++++++++++++<br>> 6 files changed, 177 insertions(+), 0 deletions(-)<br>> create mode 100644 lib/Transforms/Scalar/SeSeRegionInfo.cpp<br>
><br>> diff --git a/include/llvm/InitializePasses.h b/include/llvm/InitializePasses.h<br>> index d86b199..a50db8d 100644<br>> --- a/include/llvm/InitializePasses.h<br>> +++ b/include/llvm/InitializePasses.h<br>
> @@ -197,6 +197,7 @@ void initializeSROA_DFPass(PassRegistry&);<br>> void initializeSROA_SSAUpPass(PassRegistry&);<br>> void initializeScalarEvolutionAliasAnalysisPass(PassRegistry&);<br>> void initializeScalarEvolutionPass(PassRegistry&);<br>
> +void initializeSeSeRegionInfoPass(PassRegistry&);<br>> void initializeSimpleInlinerPass(PassRegistry&);<br>> void initializeSimpleRegisterCoalescingPass(PassRegistry&);<br>> void initializeSimplifyHalfPowrLibCallsPass(PassRegistry&);<br>
> diff --git a/include/llvm/LinkAllPasses.h b/include/llvm/LinkAllPasses.h<br>> index 7dd8ebd..bb7a68a 100644<br>> --- a/include/llvm/LinkAllPasses.h<br>> +++ b/include/llvm/LinkAllPasses.h<br>> @@ -115,6 +115,7 @@ namespace {<br>
> (void) llvm::createRegionViewerPass();<br>> (void) llvm::createSCCPPass();<br>> (void) llvm::createScalarReplAggregatesPass();<br>> + (void) llvm::createSeSeRegionInfoPass();<br>
> (void) llvm::createSimplifyLibCallsPass();<br>> (void) llvm::createSimplifyHalfPowrLibCallsPass();<br>> (void) llvm::createSingleLoopExtractorPass();<br>> diff --git a/include/llvm/Transforms/Scalar.h b/include/llvm/Transforms/Scalar.h<br>
> index 9a87eab..6c63eb8 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>> +// SeSeRegionInfo - Convert refined regions to simple regions.<br>
> +//<br>> +Pass *createSeSeRegionInfoPass();<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..836c22c 100644<br>> --- a/lib/Transforms/Scalar/CMakeLists.txt<br>> +++ b/lib/Transforms/Scalar/CMakeLists.txt<br>> @@ -26,6 +26,7 @@ add_llvm_library(LLVMScalarOpts<br>> SCCP.cpp<br>> Scalar.cpp<br>
> ScalarReplAggregates.cpp<br>> + SeSeRegionInfo.cpp<br>> SimplifyCFGPass.cpp<br>> SimplifyHalfPowrLibCalls.cpp<br>> SimplifyLibCalls.cpp<br>> diff --git a/lib/Transforms/Scalar/Scalar.cpp b/lib/Transforms/Scalar/Scalar.cpp<br>
> index 1d0ca7d..7ab83b9 100644<br>> --- a/lib/Transforms/Scalar/Scalar.cpp<br>> +++ b/lib/Transforms/Scalar/Scalar.cpp<br>> @@ -53,6 +53,7 @@ void llvm::initializeScalarOpts(PassRegistry&Registry) {<br>> initializeRegToMemPass(Registry);<br>
> initializeSCCPPass(Registry);<br>> initializeIPSCCPPass(Registry);<br>> + initializeSeSeRegionInfoPass(Registry);<br>> initializeSROA_DFPass(Registry);<br>> initializeSROA_SSAUpPass(Registry);<br>
> initializeCFGSimplifyPassPass(Registry);<br>> diff --git a/lib/Transforms/Scalar/SeSeRegionInfo.cpp b/lib/Transforms/Scalar/SeSeRegionInfo.cpp<br>> new file mode 100644<br>> index 0000000..6005e64<br>> --- /dev/null<br>
> +++ b/lib/Transforms/Scalar/SeSeRegionInfo.cpp<br>> @@ -0,0 +1,167 @@<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>> +// into simple regions.<br>> +//<br>> +//===----------------------------------------------------------------------===//<br>
> +<br>> +#include "llvm/Instructions.h"<br>> +#include "llvm/ADT/Statistic.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 "sese-regions"<br>> +<br>> +using namespace llvm;<br>> +<br>> +STATISTIC(NumEntries, "The # of created entry edges");<br>
> +STATISTIC(NumExits, "The # of created exit edges");<br>> +<br>> +namespace {<br>> + class SeSeRegionInfo : 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 SeSeRegionInfo() : RegionPass(ID) {<br>> + initializeSeSeRegionInfoPass(*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(SeSeRegionInfo, "sese-regions",<br>I believe sese is difficult to understand. Can we use simple-regions for <br>this?<br><br>> + "Transform refined regions into simple regions", false, false)<br>
> +<br>> +char SeSeRegionInfo::ID = 0;<br>> +namespace llvm {<br>> + Pass *createSeSeRegionInfoPass() {<br>> + return new SeSeRegionInfo();<br>> + }<br>> +}<br>> +<br>> +void SeSeRegionInfo::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: ]"<br>> +<< enteringBlock->getNameStr()<br>
> +<< " => "<br>> +<< enteringBlock->getNameStr()<< "]\n";<br>> + if (exitingBlock)<br>> + O<< " Exit: ["<br>> +<< exitingBlock->getNameStr()<br>
> +<< " => "<br>> +<< exitingBlock->getNameStr()<< "[\n";<br>> +<br>> + O<< "\n";<br>> + }<br>> +}<br>> +<br>> +void SeSeRegionInfo::getAnalysisUsage(AnalysisUsage&AU) const {<br>
> + AU.setPreservesAll();<br>Is this correct?<br></pre></blockquote><div></div><blockquote style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;" class="gmail_quote"><pre><br>
> + AU.addRequired<RegionInfo>();<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 does not update RegionInfo analysis.<br>
> +BasicBlock *SeSeRegionInfo::createSingleEntryEdge(Region *R) {<br>> + BasicBlock *BB = R->getEntry();<br>> + BasicBlock::iterator SplitIt = BB->begin();<br>> +<br>> + while (isa<PHINode>(SplitIt))<br>
> + ++SplitIt;<br>> +<br>> + BasicBlock *newBB = SplitBlock(BB, SplitIt, this);<br></pre></blockquote><blockquote style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;" class="gmail_quote">
<pre>> +<br>> + for (BasicBlock::iterator PI = BB->begin(); isa<PHINode>(PI); ++PI) {<br>> + PHINode *PN = cast<PHINode>(PI);<br>> + PHINode *NPN =<br>> + PHINode::Create(PN->getType(), PN->getName()+".ph", newBB->begin());<br>
> +<br>> + for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE; ++PI) {<br>> + if (R->contains(*PI)) {<br>> + Value *V = PN->removeIncomingValue(*PI, false);<br>> + NPN->addIncoming(V, *PI);<br>
> + }<br>> + }<br>You can get rid of a set of parenthesis.<br><br>> + PN->replaceAllUsesWith(NPN);<br>> + NPN->addIncoming(PN,BB);<br>> + }<br>> +<br>> + for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE; ++PI)<br>
> + if (R->contains(*PI))<br>> + (*PI)->getTerminator()->replaceUsesOfWith(BB, newBB);<br>> +<br>> + return newBB;<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 *SeSeRegionInfo::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(), ".region", this);<br>
> +}<br>> +<br>> +bool SeSeRegionInfo::runOnRegion(Region *R, RGPassManager&RGM) {<br>> + RegionInfo *RI =&getAnalysis<RegionInfo>();<br>> + modified = false;<br>> +<br>> + CR = R;<br>
> + if (!R->isTopLevelRegion()) {<br>> + BasicBlock *nBB;<br>> + BasicBlock *oBB;<br>Can you use more descriptive variable names?<br><br>> +<br>> + BasicBlock *enteringBlock = R->getEnteringBlock();<br>
> + if (!enteringBlock) {<br>You can probably move the R->getEnteringBlock() into the if condition. <br>enteringBlock is not used anywhere else.<br><br>> + oBB = R->getEntry();<br>> + nBB = createSingleEntryEdge(R);<br>
> +<br>> + RI->splitBlock(nBB, oBB);<br>> +<br>> + modified |= true;<br>> + ++NumEntries;<br>> + }<br>> +<br>> + BasicBlock *exitingBlock = R->getExitingBlock();<br>> + if (!exitingBlock) {<br>
Dito.<br><br>> + oBB = R->getExit();<br>> + nBB = createSingleExitEdge(R);<br>> +<br>> + RI->splitBlock(nBB, oBB);<br>> +<br>> + modified |= true;<br>> + ++NumExits;<br>
> + }<br>> + }<br>> +<br>> + return modified;<br>> +}<br>> -- 1.7.1<br></pre></blockquote><br>