[llvm-commits] Patch for simplifying regions
Vu Le
vmle at ucdavis.edu
Tue Mar 1 12:15:09 PST 2011
On Tue, Mar 1, 2011 at 12:12 PM, Vu Le <vmle at ucdavis.edu> wrote:
> 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);
> }
>
Please ignore this code fragment. It should be like this:
r = RI->getTopLevelRegion();
std::vector<Region *> RQ;
RQ.push_back(r);
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() == oldEntry && !R->contains(r))
r->replaceExit(newEntry);
}
> 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/4ade05df/attachment.html>
More information about the llvm-commits
mailing list