[llvm-commits] [Please review][PATCH] Implement the block_iterator of Region based on df_iterator.

[Tobias Grosser]

On 08/01/2012 11:32 AM, Hongbin Zheng wrote:
> Hi,
>
> A few days ago in the polly-dev list,
> Antoine<trouve.antoine at gmail.com> pointed out that the implementation
> of block_iterator of the (single-entry single-exit) Region class is
> hard to understand,
> so I try to implement the block_iterator of Region based on
> df_iterator, which is simpler.
>
> The general idea is, by iterate the BB in DF order from the entry of
> the region and marking the exit block of the single-entry single-exit
> region as visited when we build the begin iterator, the iteration will
> never visit the exit block and its successors, i.e. the blocks may be
> visited by the DF iterator but outside the region.

Hi ether,

thanks for the patch. Here some comments:

> diff --git a/include/llvm/Analysis/RegionInfo.h
> b/include/llvm/Analysis/RegionInfo.h
> index eae94e7..310dcd6 100644
> --- a/include/llvm/Analysis/RegionInfo.h
> +++ b/include/llvm/Analysis/RegionInfo.h
> @@ -500,50 +500,59 @@ public:
>     /// Region. The iterator also iterates over BasicBlocks that are elements of
>     /// a subregion of this Region. It is therefore called a flat iterator.
>     //@{
> -  template <typename RegionNodeItT>
> +  template <bool IsConst>
>     class block_iterator_wrapper
> -    : public std::iterator<std::forward_iterator_tag, BasicBlock, ptrdiff_t> {
> -    typedef std::iterator<std::forward_iterator_tag, BasicBlock, ptrdiff_t>
> +    : public df_iterator<typename conditional<IsConst,
> +                                              const BasicBlock,
> +                                              BasicBlock>::type*> {
> +    typedef df_iterator<typename conditional<IsConst,
> +                                             const BasicBlock,
> +                                             BasicBlock>::type*>
>         super;
> -
> -    RegionNodeItT Iter;
> -
>     public:
> -    typedef block_iterator_wrapper<RegionNodeItT> Self;
> +    typedef block_iterator_wrapper<IsConst> Self;
>       typedef typename super::pointer pointer;
>
> -    block_iterator_wrapper(RegionNodeItT Iter) : Iter(Iter) {}
> -
> -    bool operator==(const Self &RHS) const { return Iter == RHS.Iter; }
> -    bool operator!=(const Self &RHS) const { return Iter != RHS.Iter; }
> -    pointer operator*() const {
> -      return (*Iter)->template getNodeAs<BasicBlock>();

I think we should define our own operator* as follows.

// FIXME: Even a const_iterator returns a non-const BasicBlock pointer. 
//        This was introduced for backwards compatibility, but should
//        be removed as soon as all users are fixed.
BasicBlock *operator*() const {
       return const_cast<BasicBlock*>(super::operator*());
}

> +    // Construct the end iterator.
> +    block_iterator_wrapper() : super(df_end<pointer>(0)) {}
> +
> +    block_iterator_wrapper(super I) : super(I) {}
> +
>       Self& operator++() {
> -      ++Iter;
> +      super::operator++();
>         return *this;
>       }

Is it necessary to redefine that function?
I believe you can just remove it and calls will automatically be 
forwarded to the definition in df_iterator.

>       Self operator++(int) {
> -      Self tmp = *this;
> -      ++*this;
> -      return tmp;
> -    }
> -
> -    const Self &operator=(const Self &I) {
> -      Iter = I.Iter;
> -      return *this;
> +      return super::operator++(0);
>       }

Is it necessary to redefine that function?
I believe you can just remove it and calls will automatically be 
forwarded to the definition in df_iterator.

(Also, both implementations (the old and the new one) look incorrect. 
The old one was always incrementing by one, the new one is not 
incremented at all. I believe the iterator should be incremented by as 
many steps as defined by the parameter. Removing our implementation, 
should give us the correct implementation from df_iterator)

>     };
> -  typedef block_iterator_wrapper<block_node_iterator> block_iterator;
> -  typedef block_iterator_wrapper<const_block_node_iterator>
> -    const_block_iterator;
>
> -  block_iterator block_begin();
> -  block_iterator block_end();
> +  typedef block_iterator_wrapper<false> block_iterator;
> +  typedef block_iterator_wrapper<true>  const_block_iterator;
> +
> +  block_iterator block_begin() const {
> +   return block_iterator(getEntry(), getExit());
> +  }
>
> -  const_block_iterator block_begin() const;
> -  const_block_iterator block_end() const;
> +  block_iterator block_end() const {
> +   return block_iterator();
> +  }
> +
> +  const_block_iterator const_block_begin() const {
> +    return const_block_iterator(getEntry(), getExit());
> +  }
> +  const_block_iterator const_block_end() const {
> +    return const_block_iterator();
> +  }

This changes the interface of the iterator in a surprising way. Being 
able to get a non-const iterator from a const Region is surprising. I 
think we should keep the signatures of block_begin() and block_end() as 
they are today. Meaning:

block_iterator block_begin();
block_iterator block_end();
const_block_iterator block_begin() const;
const_block_iterator block_end() const;

> --- a/lib/Analysis/RegionPrinter.cpp
> +++ b/lib/Analysis/RegionPrinter.cpp
> @@ -121,7 +121,7 @@ struct DOTGraphTraits<RegionInfo*> : public
> DOTGraphTraits<RegionNode*> {
>
>       RegionInfo *RI = R->getRegionInfo();
>
> -    for (Region::const_block_iterator BI = R->block_begin(),
> +    for (Region::block_iterator BI = R->block_begin(),
>            BE = R->block_end(); BI != BE; ++BI)
>         if (RI->getRegionFor(*BI) == R)
>           O.indent(2 * (depth + 1)) << "Node"

I don't think this change is correct. We should rather add a version of 
RI->getRegionFor() that returns a const BasicBlock in case a const 
Region is passed in.

With the FIXME I proposed above, fixing this (and similar problems in 
Polly) is not immediately required. I propose to first commit the 
df_iterator improvement and then to fix the unrelated const correctness 
issues in subsequent patches.

Cheers
Tobi