[PATCH] Add iterator_adaptor to iterate over SDNode operand SDNode's

[David Blaikie]

On Thu, Jun 25, 2015 at 2:30 PM, Pete Cooper <peter_cooper at apple.com> wrote:

> Hi David
>
> Currently the only SDNode iterator over operands does so with SDUse*.
> Users frequently then call getNode() on the operand.
>
> This patch adds an iterator to SDNode which returns the SDNode of the
> operand.  This allows more patterns to be converted to foreach.  It is
> based on value_op_iterator which I found in User.h.
>
> For now i’ve only used it in a single place, but I found a bunch more in
> DAGCombiner for example which should be applicable.  I would convert those
> in a later commit assuming you are ok with this solution.
>

This immediately raises red flags:

  SDNode *operator*() const { return I->getNode(); }
  SDNode *operator->() const { return operator*(); }

op* should return a T& and op-> should return T*

If these SDNode*s can never be null, then perhaps this should be:

  SDNode &operator*
  SDNode *operator->

? (because I assume you don't have an SDNode* lvalue to return a reference
to) I assume the adapter helper can implement one of those in terms of the
other so you only have to implement one of them? I forget how the adapter
utility works.


>
> BTW, been trying to work out if there would ever be a good solution for an
> iterator combined with isa<> or dyn_cast<>.  If you look at the code this
> patch touches in AArch64ISelLowering, it is immediately followed by a
> dyn_cast.  I’d really like to find a clean way to fold that it to the
> foreach loop, i.e.,
>
> for (auto *C : dyn_cast<ConstantSDNode>(N->op_nodes()))
>
> just a thought, but thats unrelated to this patch for now.
>

Yep, though probably more in the form of a filtered range, I suspect:

  for (auto &C : filtered_transform(N->op_nodes(), [](SDUse *U) { return
U->getNode(); }))

It'd be a bit tricky to deal with the value type of this range's iterators
- chances are the predicate should return an Optional<T&> (Hmm, don't think
our Optional template supports ref parameters yet anyway) or T* (not sure
if we could generalize it so it could cope with Optional<T>, maybe - so we
could support generators where the values are not already/permanently
in-memory) and then the value_type is T.


>
> Cheers,
> Pete
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150625/7acdd1e5/attachment.html>