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

[David Blaikie]

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

>
> On Jun 25, 2015, at 2:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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*
>
> I’d forgotten about that.
>
>
> If these SDNode*s can never be null, then perhaps this should be:
>
> I wasn’t actually sure if they could be.  My initial reaction was that
> null operands wouldn’t make sense, but it turns out we never checked.  So
> here’s a patch which does actually ensure that the SDNode's referenced as
> operands are never null.  It passes make check.  I can put it on another
> email for review if you prefer I don’t add it here.
>

It might be worth a separate thread, or at least a drive-by by someone who
deals with this part of the code. I don't really understand the
necessity/merits/drawbacks of the 'SDUse::reset()" member function you've
introduced.


>
>
>
>   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.
>
> I think it makes sense to do this.  This will unfortunately be one of the
> few SDNode & in the entire codebase though, which makes it stand out.
> SDNode really does seem to always be a pointer.  I’ll fix up the patch to
> do this soon.
>

Yeah, there are a few types (read: Lots) like this in LLVM. I personally
don't mind being more referential in spite of that, but I can understand
others might feel less comfortable with that.

If you wanted to preserve the pointer-ness, you'd have op* return SDNode *
(this would be a bit incorrect, it should really be SDNode * const &, and
you can do that by having an SDNode* member in the iterator that you init
and return a ref to... technically that's the more correct option - I'm not
entirely sure where that matters) and no op->.

>
>
>>
>> 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.
>
> Interesting.  I hadn’t though to use Optional.  I might try to implement
> something like this if i get time.
>
> Cheers,
> Pete
>
>
>
>>
>> Cheers,
>> Pete
>>
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150625/d6ce6468/attachment.html>