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

[David Blaikie]

On Fri, Jun 26, 2015 at 10:18 AM, Pete Cooper <peter_cooper at apple.com>
wrote:

>
> On Jun 25, 2015, at 5:49 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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.
>
> I tried making the op* return an SDNode& then propagated that through to
> the dyn_cast in AArch64ISelLowering.cpp.  Unfortunately after that things
> got messy.
>
> I could have done this
>
> for (const SDNode &Elt : N->op_values()) {
>     if (const ConstantSDNode *C = dyn_cast<ConstantSDNode>(&Elt)) {
>
> but it seemed a shame to have to add the & in the dyn_cast.  So I tried to
> actually make a version of dyn_cast on references to non-pointers, and
> return an optional if the cast fails.  Unfortunately that wasn’t much
> better.
>

What went wrong there?

(actually a while back I proposed having dyn_cast be able to accept
references and return pointers (Optional<T> and T* are basically the same -
though Optional<T> is nice documentation for "this really can be null"
rather than pointers where that's a bit more vague) - Chandler didn't
particularly like the idea so I have up... *shrug*)


> But it did lead me to an interesting piece of code in Casting.h.  We have
> a struct called simplify_type which can be used to automatically do things
> like convert to other classes.  This is implemented to go from SDValue to
> SDNode* which is really what I want.
>

Ah, it's already implemented? Good-o then.


> So, i’ve totally changed the iterator now to return SDValue& and
> SDValue*.  Thats really what the SDUse’s used as operands were storing
> anyway.
>

Sounds plausible - and I'll neglect to pass judgment on the merits or
issues of the existing simplify_type from SDValue -> SDNode*. Again, don't
know enough of the context there to even have a clue if that's a nice thing
to do or not. I do think, generally, that simplify_type gets a bit
overused/abused. But if it's already doing this one.. *shrug*


>  Then the dyn_cast can be left unchanged.
>
> What do you think?
>
> Cheers,
> Pete
>
>
>
>
> 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/20150626/c05c1b84/attachment.html>