[llvm-commits] Patch for simplifying regions
Vu Le
vmle at ucdavis.edu
Tue Mar 1 12:12:29 PST 2011
Hi Tobias,
Thanks for the feedback.
On Tue, Mar 1, 2011 at 5:49 AM, Tobias Grosser <grosser at fim.uni-passau.de>wrote:
> On 02/28/2011 02:52 PM, Vu Le wrote:
>
>> Hi Tobias,
>>
>> I did make some changes to use splitBlockPredecessors in splitting
>> entry nodes.
>>
>
> Thanks for your fast reaction. The patch looks very nice. Though I have
> some comments that we need to fix before committing it.
>
> Thank you.
>
> I tested it with coreutils and MySQL together with region-extractor pass.
>>
> Nice.
>
>
> If you're still interested in region-extractor pass, I'll make a patch.
>>
> Yes. I am extremely interested.
>
> diff --git a/include/llvm/InitializePasses.h
>> b/include/llvm/InitializePasses.h
>> index 02dbfbd..0591845 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();
>>
> This change does not seem to be needed. Please remove it.
>
> OK
> + (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.
>>
> Are there cases when it is not possible? Would be interesting to mention
> those.
>
> It is when a region has function entry as its entry and a single edge exit.
But then it is a simple region, I suppose.
I'll remove the "if possible" part.
+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..65e4d5c
>> --- /dev/null
>> +++ b/lib/Transforms/Scalar/RegionSimplify.cpp
>> @@ -0,0 +1,185 @@
>> +//===- 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
>> +// 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"
>>
> Can we call the pass "region-simplify"? Like "loop-simplify" (The name was
> recently changed)
>
> Sure
> +using namespace llvm;
>> +
>> +STATISTIC(NumEntries, "The # of created entry edges");
>> +STATISTIC(NumExits, "The # of created exit edges");
>> +
>> +namespace {
>> +class RegionSimplify: public RegionPass {
>> + bool modified;
>> + Region *CR;
>> + void createSingleEntryEdge(Region *R);
>> + void 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",
>>
> "regionsimplify" -> "region-simplify"
>
OK
>
> + "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 {
>>
>> + if (!modified)
>> + return;
>> +
>> + BasicBlock *enteringBlock = CR->getEnteringBlock();
>> + BasicBlock *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";
>> +
>> + O<< "\n";
>> +}
>> +
>> +void RegionSimplify::getAnalysisUsage(AnalysisUsage&AU) const {
>> + AU.addPreserved<DominatorTree> ();
>> + AU.addPreserved<RegionInfo> ();
>> + AU.addRequired<RegionInfo> ();
>> +}
>> +
>> +// createSingleEntryEdge - Split the entry basic block of the given
>>
>> +// region after the last PHINode to form a single entry edge.
>> +void RegionSimplify::createSingleEntryEdge(Region *R) {
>> + BasicBlock *oldEntry = R->getEntry();
>> + SmallVector<BasicBlock*, 4> Preds;
>>
>> + for (pred_iterator PI = pred_begin(oldEntry), PE = pred_end(oldEntry);
>> + PI != PE; ++PI)
>> + if (!R->contains(*PI))
>> + Preds.push_back(*PI);
>> +
>> + if (Preds.size()==0)
>> + return;
>>
> Can we change this to an assert?
>
> assert(Preds.size() && "This region has already a single entry edge");
>
Then we have to make sure that R's entry is not the function entry.
In runOnRegion
if (!(R->getEnteringBlock())
&& (pred_begin(R->getEntry()) != pred_end(R->getEntry()))) {
createSingleEntryEdge(R);
...
> +
>> + BasicBlock *newEntry = SplitBlockPredecessors(oldEntry,
>> + Preds.data(),
>> Preds.size(),
>> + ".single_entry", this);
>> +
>> + RegionInfo *RI =&getAnalysis<RegionInfo> ();
>> + // We do not update entry node for children of this region.
>> + // This make it easier to extract children regions because they do not
>> share
>> + // the entry node with their parents.
>> + // all parent regions whose entry is oldEntry are updated with newEntry
>> + Region *r = R->getParent();
>> + while (r->getEntry() == oldEntry&& !r->isTopLevelRegion()) {
>> + r->replaceEntry(newEntry);
>> + r = r->getParent();
>> + }
>> +
>> + // We do not update exit node for children of this region for the same
>> reason
>> + // of not updating entry node.
>> + // All other regions whose exit is oldEntry are updated with new exit
>> node
>> + r = RI->getTopLevelRegion();
>>
> r = r->getParent() should be enough?
>
I don't think so. We want to find ALL other regions, not just the regions
starting from
parent of R. We must start from the top.
>
> + std::deque<Region *> RQ;
>>
> std::vector is probably simpler
>
> + RQ.push_back(r);
>> + while (!RQ.empty()){
>> + r = RQ.front();
>> + for (Region::const_iterator RI = r->begin(), RE = r->end(); RI!=RE;
>> ++RI)
>> + RQ.push_back(*RI);
>> + if (r->getExit() == oldEntry&& !R->contains(r))
>> + r->replaceExit(newEntry);
>> + RQ.pop_front();
>> + }
>>
> I need to think about this part (Will send feedback later).
>
This is not clean. Here is a cleaner version
r = RI->getTopLevelRegion();
std::vector<Region *> RQ;
RQ.push_back(r);
while (!RQ.empty()){
r = RQ.back();
RQ.pop_back();
if (r->getExit() != oldEntry || R->contains(r))
continue;
r->replaceExit(newEntry);
for (Region::const_iterator RI = r->begin(), RE = r->end(); RI!=RE;
++RI)
RQ.push_back(*RI);
}
We cannot make sure that those regions whose exit is OldEntry belong to any
particular subregions. That's why we have to iterate over all the regions in
the
tree.
>
> +
>> + modified |= true;
>>
> I do not believe this is part of creating the edges. 'modified' is part of
> the status tracking of the pass and should probably be put into
> runOnRegion().
>
> + ++NumEntries;
>>
> Dito. Please put in runOnRegion().
>
Sure.
>
> +}
>> +
>> +// createSingleExitEdge - Split the exit basic of the given region
>>
>> +// to form a single exit edge.
>> +void RegionSimplify::createSingleExitEdge(Region *R) {
>> + BasicBlock *oldExit = R->getExit();
>> +
>> + SmallVector<BasicBlock*, 4> Preds;
>> + for (pred_iterator PI = pred_begin(oldExit), PE = pred_end(oldExit);
>>
>> + PI != PE; ++PI)
>> + if (R->contains(*PI))
>> + Preds.push_back(*PI);
>> +
>> + BasicBlock *newExit = SplitBlockPredecessors(oldExit,
>> + Preds.data(),
>> Preds.size(),
>> + ".single_exit", this);
>> + RegionInfo *RI =&getAnalysis<RegionInfo> ();
>> +
>> + // We do not need to update entry nodes because this split happens
>> inside
>> + // this region and it affects only this region and all of its children.
>> + // The new split node belongs to this region
>>
>
> + RI->setRegionFor(newExit,R);
>> +
>> + // all children of this region whose exit is oldExit is changed to
>> newExit
>> + std::deque<Region *> RQ;
>> + for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE;
>> ++RI)
>> + RQ.push_back(*RI);
>> + while (!RQ.empty()){
>> + R = RQ.front();
>> + for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE;
>> ++RI)
>> + RQ.push_back(*RI);
>> +
>> + if (R->getExit() == oldExit)
>> + R->replaceExit(newExit);
>> + RQ.pop_front();
>> + }
>>
>
> A vector is probably simpler. We also do not need to add child regions for
> regions that do not have an exit matching oldExit. What about this code?
>
I'm not sure about this.
A child might not share the exit with its parent.
I think we still need to check.
>
> std::vector<Region *> RQ;
>
> while (!RQ.empty()){
> R = RQ.back();
> RQ.pop_back();
>
> if (R->getExit() != oldExit)
> continue;
>
> R->replaceExit(newExit);
>
> for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE;
> ++RI)
> RQ.push_back(*RI);
>
> }
>
> std::vector<Region *> RQ;
for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE; ++RI)
RQ.push_back(*RI);
while (!RQ.empty()){
R = RQ.back();
RQ.pop_back();
for (Region::const_iterator RI = R->begin(), RE = R->end(); RI!=RE; ++RI)
RQ.push_back(*RI);
if (R->getExit() == oldExit)
R->replaceExit(newExit);
}
>
> +
>> + modified |= true;
>>
> This function always modifies the CFG. So no need to put additional logic
> here.
>
> + ++NumExits;
>>
> Dito.
>
> +}
>>
>> +
>> +bool RegionSimplify::runOnRegion(Region *R, RGPassManager&RGM) {
>> + modified = false;
>> +
>> + CR = R;
>> + if (!R->isTopLevelRegion()) {
>> +
>> + if (!(R->getEnteringBlock())) {
>> + createSingleEntryEdge(R);
>> + }
>>
>
> if (!(R->getEnteringBlock())) {
> createSingleEntryEdge(R);
> ++NumEntries;
> modified = true.
> }
>
We need to add additional check for the special case where R does not have
any predecessor.
if (!(R->getEnteringBlock())
&& (pred_begin(R->getEntry()) != pred_end(R->getEntry()))) {
createSingleEntryEdge(R);
modified = true;
++NumEntries;
}
>
> +
>> + if (!(R->getExitingBlock())) {
>> + modified |= createSingleExitEdge(R);
>>
>
> if (!(R->getExitingBlock())) {
> createSingleExitEdge(R);
> ++NumExits;
> modified = true;
> }
>
> + }
>> + }
>> +
>> + 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);
>> diff --git a/test/Transforms/RegionSimplify/dg.exp
>> b/test/Transforms/RegionSimplify/dg.exp
>> new file mode 100644
>> index 0000000..f200589
>> --- /dev/null
>> +++ b/test/Transforms/RegionSimplify/dg.exp
>> @@ -0,0 +1,3 @@
>> +load_lib llvm.exp
>> +
>> +RunLLVMTests [lsort [glob -nocomplain $srcdir/$subdir/*.{ll,c,cpp}]]
>> diff --git a/test/Transforms/RegionSimplify/multi_exits.ll
>> b/test/Transforms/RegionSimplify/multi_exits.ll
>> new file mode 100644
>> index 0000000..024971c
>> --- /dev/null
>> +++ b/test/Transforms/RegionSimplify/multi_exits.ll
>> @@ -0,0 +1,15 @@
>> +; RUN: opt -regionsimplify
>>
>
> Did you test this test case. I believe you should use a line like:
>
> ; RUN: opt -regionsimplify %s
>
> Furthermore, we should also check the generated output.
>
> ; RUN: opt -regionsimplify %s | FileCheck %s
>
> +define void @f() nounwind {
>>
> ; CHECK: @f()
> ; CHECK: br label %"1"
> ...
>
> Add after the CHECK lines the output you expect.
>
> The same comments apply to the other test files. I would also be interested
> to have some test files that include PHI-Nodes.
>
> Can you send out an updated patch?
>
> I'll look at FileCheck and create additional tests.
Thank you again for your help ;).
> Thanks a lot
> Tobi
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110301/cac664be/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: regionsimplify2.patch
Type: text/x-patch
Size: 12299 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110301/cac664be/attachment.bin>
More information about the llvm-commits
mailing list