[llvm-commits] Patch for simplifying regions

[Vu Le]

Add more test cases.
Vu

On Tue, Mar 1, 2011 at 12:15 PM, Vu Le <vmle at ucdavis.edu> wrote:

>
>
> 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/20110302/b2568b44/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: regionsimplify3.patch
Type: text/x-patch
Size: 14879 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110302/b2568b44/attachment.bin>