[llvm-commits] [Please review][PATCH] Implement the block_iterator of Region based on df_iterator.
Hongbin Zheng
etherzhhb at gmail.com
Thu Aug 2 06:55:29 PDT 2012
Hi tobi,
On Thu, Aug 2, 2012 at 8:08 PM, Tobias Grosser <tobias at grosser.es> wrote:
>
> 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*());
>
> }
Sounds reasonable.
>
>> + // 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.
Originally, I add these redefined functions because the resultant type
of operator++() and operator++(int) is df_iterator instead of
block_iterator_wrapper, but these problem is fixed by the third
constructor of block_iterator which convert the df_iterator to
block_iterator implicitly.
>
>
>> 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)
You means I should not pass 0 to the post-increment operator?
Anyway, I will fix this by removing the unnecessary functions.
>
>
>> };
>> - 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.
Yes
>
> Cheers
> Tobi
The up to date patch is attached.
best regards
ether
---
include/llvm/Analysis/RegionInfo.h | 72 ++++++++++++++++++++----------------
lib/Analysis/RegionInfo.cpp | 16 --------
2 files changed, 40 insertions(+), 48 deletions(-)
diff --git a/include/llvm/Analysis/RegionInfo.h
b/include/llvm/Analysis/RegionInfo.h
index eae94e7..df30ab7 100644
--- a/include/llvm/Analysis/RegionInfo.h
+++ b/include/llvm/Analysis/RegionInfo.h
@@ -500,50 +500,58 @@ 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>();
+ // Construct the begin iterator.
+ block_iterator_wrapper(pointer Entry, pointer Exit) :
super(df_begin(Entry))
+ {
+ // Mark the exit of the region as visited, so that the children of the
+ // exit and the exit itself, i.e. the block outside the region will never
+ // be visited.
+ super::Visited.insert(Exit);
}
- Self& operator++() {
- ++Iter;
- return *this;
- }
- Self operator++(int) {
- Self tmp = *this;
- ++*this;
- return tmp;
- }
+ // Construct the end iterator.
+ block_iterator_wrapper() : super(df_end<pointer>(0)) {}
+
+ block_iterator_wrapper(super I) : super(I) {}
- const Self &operator=(const Self &I) {
- Iter = I.Iter;
- return *this;
+ // 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*());
}
};
- 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;
- const_block_iterator block_begin() const;
- const_block_iterator block_end() const;
+ block_iterator block_begin() {
+ return block_iterator(getEntry(), getExit());
+ }
+
+ block_iterator block_end() {
+ return block_iterator();
+ }
+
+ const_block_iterator block_begin() const {
+ return const_block_iterator(getEntry(), getExit());
+ }
+ const_block_iterator block_end() const {
+ return const_block_iterator();
+ }
//@}
/// @name Element Iterators
diff --git a/lib/Analysis/RegionInfo.cpp b/lib/Analysis/RegionInfo.cpp
index 5f4458b..868f483 100644
--- a/lib/Analysis/RegionInfo.cpp
+++ b/lib/Analysis/RegionInfo.cpp
@@ -262,22 +262,6 @@ Region::const_block_node_iterator
Region::block_node_end() const {
return GraphTraits<FlatIt<const Region*> >::nodes_end(this);
}
-Region::block_iterator Region::block_begin() {
- return block_node_begin();
-}
-
-Region::block_iterator Region::block_end() {
- return block_node_end();
-}
-
-Region::const_block_iterator Region::block_begin() const {
- return block_node_begin();
-}
-
-Region::const_block_iterator Region::block_end() const {
- return block_node_end();
-}
-
Region::element_iterator Region::element_begin() {
return GraphTraits<Region*>::nodes_begin(this);
}
--
1.7.5.4
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Implement-the-block_iterator-of-Region-based-on-df_i.patch
Type: text/x-diff
Size: 4636 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120802/9075a502/attachment.patch>
More information about the llvm-commits
mailing list