[PATCH] Add iterator for PHINode value/BB pair

[Duncan P. N. Exon Smith]

> On 2015-Jul-21, at 18:55, Pete Cooper <peter_cooper at apple.com> wrote:
> 
> Hi David, Duncan
> 
> This is another iterator which i’d like to use to enable more foreach loops in LLVM.
> 
> This time its for returning both the value and basic block from a phi node.  Its very common to need both of these together, but currently involves a manual loop to do so.
> 
> The code here is similar to generic_gep_type_iterator where i’m wrapping an iterator (in this case 2) and on updates to the iterators I update the ‘Pair’ field.  operator* then returns the Pair when requested.
> 
> An alternative would be to only update Pair in operator*, but that would require making the field mutable.
> 
> I’ve added a single use of this in BasicAliasAnalysis.  A grep of the code finds 47 total loops over PN[.,->]getNumIncomingValues(), of which around 40 look eligible to use this.
> 
> Comments welcome.  
> 
> Cheers,
> Pete
> 
> <phinode.diff>

> diff --git a/include/llvm/IR/Instructions.h b/include/llvm/IR/Instructions.h
> index 07d5f11..d6b5b80 100644
> --- a/include/llvm/IR/Instructions.h
> +++ b/include/llvm/IR/Instructions.h
> @@ -2377,6 +2377,90 @@ public:
>  
>    const_op_range incoming_values() const { return operands(); }
>  
> +  /// This contains a value and basic block corresponding to a single incoming
> +  /// value/block pair.
> +  class PHINodeEdge {
> +  private:
> +    Value *Val;
> +    BasicBlock *BB;
> +
> +  public:
> +    PHINodeEdge(Value *Val,
> +                BasicBlock *BB) : Val(Val), BB(BB) { }
> +
> +    Value *getValue() const { return Val; }
> +    BasicBlock *getBlock() const { return BB; }
> +  };

Any reason this isn't just POD?

    struct PHINodeEdge {
      Value *Val;
      BasicBlock *BB;
    };

> +
> +  /// This class provides iterator support for edges which
> +  /// provide access to both the value and block on an incoming edge without
> +  /// needing to manage indices.
> +  template<typename OpIteratorTy, typename BlockIteratorTy>
> +  class edge_iterator : public std::iterator<std::forward_iterator_tag,
> +                                             PHINodeEdge, ptrdiff_t> {
> +
> +    OpIteratorTy Values;
> +    BlockIteratorTy Blocks;
> +    PHINodeEdge Pair;

s/Pair/Edge/ ?

> +    explicit edge_iterator(OpIteratorTy Values, BlockIteratorTy Blocks)
> +      : Values(Values), Blocks(Blocks), Pair(*Values, *Blocks) {

If `Values` and `Blocks` are end iterators, this could be an invalid
dereference.

> +    }
> +    friend class PHINode;
> +  public:
> +    typedef std::iterator<std::forward_iterator_tag,
> +                          PHINodeEdge, ptrdiff_t>::reference reference;
> +    typedef std::iterator<std::forward_iterator_tag,
> +                          PHINodeEdge, ptrdiff_t>::pointer pointer;
> +
> +    edge_iterator(const edge_iterator &I)
> +      : Values(I.Values), Blocks(I.Blocks), Pair(*Values, *Blocks) {}

Why not `= default`?

> +    edge_iterator()
> +      : Values(OpIteratorTy()), Blocks(BlockIteratorTy()),
> +        Pair(*Values, *Blocks) {}

This looks like a null dereference.

> +
> +    bool operator==(const edge_iterator &x) const {
> +      return Values == x.Values && Blocks == x.Blocks;
> +    }
> +    bool operator!=(const edge_iterator &x) const {
> +      return !operator==(x);
> +    }
> +
> +    // Iterator traversal: forward iteration only.
> +    // Preincrement
> +    edge_iterator &operator++() {
> +      ++Values;
> +      ++Blocks;
> +      Pair = PHINodeEdge(*Values, *Blocks);

Could be a dereference of invalid.

> +      return *this;
> +    }
> +
> +    // Postincrement
> +    edge_iterator operator++(int) {
> +      edge_iterator tmp = *this; ++*this; return tmp;
> +    }
> +
> +    /// Retrieve a pointer to the current user node.
> +    const PHINodeEdge &operator*() const {

If you drop the requirements from `forward_iterator` to
`input_iterator`, then you're allowed to return a `PHINodeEdge` by-value
here instead of by-reference (unfortunately this makes it illegal to use
a bunch of STL algorithms; STL iterator traits are completely broken
IMO).

> +      return Pair;
> +    }
> +  };
> +
> +  typedef edge_iterator<op_iterator, block_iterator> MutableEdgeType;
> +  iterator_range<MutableEdgeType> incoming_edges() {
> +    return iterator_range<MutableEdgeType>(MutableEdgeType(op_begin(),
> +                                                           block_begin()),
> +                                           MutableEdgeType(op_end(),
> +                                                           block_end()));
> +  }
> +
> +  typedef edge_iterator<const_op_iterator, const_block_iterator> ConstEdgeType;
> +  iterator_range<ConstEdgeType> incoming_edges() const {
> +    return iterator_range<ConstEdgeType>(ConstEdgeType(op_begin(),
> +                                                       block_begin()),
> +                                         ConstEdgeType(op_end(),
> +                                                       block_end()));
> +  }
> +
>    /// getNumIncomingValues - Return the number of incoming edges
>    ///
>    unsigned getNumIncomingValues() const { return getNumOperands(); }