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

[David Blaikie]

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

>
> On Jun 25, 2015, at 5:21 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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.
>
> No problem.  Thanks for taking a look.  I’ve just sent out an email to
> llvm-commits and asked Hal for review as I know he’s done lots of SD work.
>
>
>
>>
>>
>>
>>   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->.
>
> I like this solution.  I tried SDNode * const & earlier but of course
> getNode() is a temporary so this doesn’t work.  I’ll get a patch together
> which makes this change.  Technically I guess that means we don’t need the
> nonnull SDNode patch, but I don’t see any harm in it anyway.
>

Personally I'd go the reference route - the more references the clearer the
documentation of nullness contracts (& tools like ubsan null reference
checking can catch bugs sooner/closer to the point of the problem rather
than later on). But I realize that's a bit jarring, stylistically, so I
certainly wouldn't insist on it.

Mightn't hurt to think a bit about the tradeoff of using temporary storage
or just cheating and returning a pointer by value from op*... I mean it'll
probably work for the range-for. I'm not sure where it'd really break - if
the value_type of the iterator is "const SDNode" then it might be
hard-to-impossible to for any algorithm to really have a problem with this.



>
> Thanks,
> Pete
>
>
>>
>>>
>>> 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/fedb0844/attachment.html>