[llvm-commits] [PATCH] Add single entry / single exit accessors.

[Andreas Simbuerger]

Am 13.01.2011 16:29, schrieb Tobias Grosser:
> On 01/13/2011 06:27 AM, Andreas Simbuerger wrote:
>> Hi,
>>
>> this time from the correct mail-address;-)
>> This patch adds support for single entry / single exit edge
>> accessors to regions.
> 
> Thanks, Andreas. Just some small comments inline.
> 
>> A (refined) region that has only 2 transition edges (1 entry, 1 exit)
>> to its parent region is defined as a simple region.
>>
>> Both methods added, return null if there is more than one entry/exit
>> edge.
>>
>> Cheers,
>> Andreas
>>
>>
>>
>> 0001-Add-single-entry-single-exit-accessors.patch
>>
>>
>> diff --git include/llvm/Analysis/RegionInfo.h
>> include/llvm/Analysis/RegionInfo.h
>> index 737d46c..4c06ec0 100644
>> --- include/llvm/Analysis/RegionInfo.h
>> +++ include/llvm/Analysis/RegionInfo.h
>> @@ -305,6 +305,20 @@ public:
>>     ///         NULL if such a basic block does not exist.
>>     Region *getExpandedRegion() const;
>>
>> +  /// @brief Return the first block of this region's single entry edge,
>> +  ///        if existing.
>> +  ///
>> +  /// @return The BasicBlock starting this region's single entry edge,
>> +  ///         else NULL.
>> +  BasicBlock *getSingleEntryBlock() const;
> I would call this getEnteringBlock() following LoopInfo in this case.

Ok :-)
> 
> 
>> +
>> +  /// @brief Return the first block of this region's single exit edge,
>> +  ///        if existing.
>> +  ///
>> +  /// @return The BasicBlock starting this region's single exit edge,
>> +  ///         else NULL.
>> +  BasicBlock *getSingleExitBlock() const;
> getExitingBlock() would follow LoopInfo probably.

Done.
> 
> 
>>     /// @brief Is this a simple region?
>>     ///
>>     /// A region is simple if it has exactly one exit and one entry edge.
>> diff --git lib/Analysis/RegionInfo.cpp lib/Analysis/RegionInfo.cpp
>> index cf48a71..22e822b 100644
>> --- lib/Analysis/RegionInfo.cpp
>> +++ lib/Analysis/RegionInfo.cpp
>> @@ -131,43 +131,55 @@ Loop *Region::outermostLoopInRegion(Loop *L)
>> const {
>>   Loop*Region::outermostLoopInRegion(LoopInfo *LI, BasicBlock*  BB)
>> const {
>>     assert(LI&&  BB&&  "LI and BB cannot be null!");
>>     Loop *L = LI->getLoopFor(BB);
>> +

I just noticed that this does not belong to the patch, removed.

>>     return outermostLoopInRegion(L);
>>   }
>>
>> -bool Region::isSimple() const {
>> -  bool isSimple = true;
>> -  bool found = false;
>> -
>> -  BasicBlock *entry = getEntry(), *exit = getExit();
>> -
>> -  if (isTopLevelRegion())
>> -    return false;
>> +BasicBlock *Region::getSingleEntryBlock() const {
>> +  BasicBlock *entry = getEntry();
>> +  BasicBlock *Pred;
>> +  BasicBlock *ee = 0;
>>
>>     for (pred_iterator PI = pred_begin(entry), PE = pred_end(entry);
>> PI != PE;
>>          ++PI) {
>> -    BasicBlock *Pred = *PI;
>> +    Pred = *PI;
>>       if (DT->getNode(Pred)&&  !contains(Pred)) {
>> -      if (found) {
>> -        isSimple = false;
>> -        break;
>> -      }
>> -      found = true;
>> +      if (ee)
>> +        return 0;
>> +
>> +      ee = Pred;
> Can you combine the two if conditions?

I don't understand, as the predicate
(DT->getNode(Pred) && !contains(Pred)) has to be valid for
both statements inside the block, so I would have to recheck that in
an else branch after merging both if's? Perhaps I'm just not thinking
clearly, it's a bit late ;-)

> 
>>       }
>>     }
>>
>> -  found = false;
>> +  return ee;
>> +}
> Maybe use a more expressive variable name instead of 'ee'. Maybe
> something like 'enteringBlock'?

Done.
> 
>> +BasicBlock *Region::getSingleExitBlock() const {
>> +  BasicBlock *exit = getExit();
>> +  BasicBlock *Pred;
>> +  BasicBlock *ee = 0;
>> +
>> +  if (!exit)
>> +    return 0;
>>
>>     for (pred_iterator PI = pred_begin(exit), PE = pred_end(exit); PI
>> != PE;
>> -       ++PI)
>> -    if (contains(*PI)) {
>> -      if (found) {
>> -        isSimple = false;
>> -        break;
>> -      }
>> -      found = true;
>> +       ++PI) {
>> +    Pred = *PI;
>> +    if (contains(Pred)) {
>> +      if (ee)
>> +        return 0;
>> +
>> +      ee = Pred;
>>       }
>> +  }
>>
>> -  return isSimple;
>> +  return ee;
>> +}
> Same comments apply here.

Done.

> 
> 
>> +bool Region::isSimple() const {
>> +  return !(isTopLevelRegion())
>> +&&  (getSingleEntryBlock() != 0)
>> +&&  (getSingleExitBlock() != 0);
> I am not sure if my email has removed some spaces/tabs. But please
> recheck indentation.

I think so, the indentation is correct within my mail client. I attached
two versions of the patch

without *.svn.patch was generated using git diff --no-prefix ... and
the other one was generated using git format-patch.

> 
> Thanks
> 
> Tobi
> 

Cheers,
Andreas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-single-entry-single-exit-accessors.patch
Type: text/x-patch
Size: 3432 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110113/af962547/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-single-entry-single-exit-accessors.svn.patch
Type: text/x-patch
Size: 2730 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110113/af962547/attachment-0001.bin>