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

David Blaikie dblaikie at gmail.com
Fri Jun 26 11:16:28 PDT 2015


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

>
> On Jun 26, 2015, at 11:06 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
> Looks generally good
>
> I’m hoping that means LGTM.  Sounds like it :)
>

Yep, sorry for the ambiguity - with the below question resolved however you
see fit, please commit.


>
>
> Is it idiomatic to use SDValues by value,
>
> I wouldn’t say idiomatic.  Inconsistent might be a better word :)
>
> rather than (possibly const) ref?
>
> I’ll change it to const ref here.  It does actually make more sense to use
> const ref where possible.
>

Sounds good - thanks!


>
>
> On Fri, Jun 26, 2015 at 11:00 AM, Pete Cooper <peter_cooper at apple.com>
> wrote:
>
>>
>> On Jun 26, 2015, at 10:33 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> As to the code in the patch itself:
>>
>> Do you need to implement op* and op->, or does the adapter have
>> convevience implementations of one in terms of the other? (be nice if it
>> did, but I could believe/imagine that it might not)
>>
>> You’re right, they do define one.  I needed my own operator* :
>>
>> const SDValue &operator*() const { return I->get(); }
>>
>> but ultimately the iterator_facade_base which we inherit from provides ->
>> in terms of *.
>>
>>
>> Is that naming convention consistent with other names we use when making
>> iterator/range pairs? Plurality in the range accessor, singularity in the
>> begin/end? Should we just avoid providing begin/end and only provide the
>> range accessor (people can always use op_values().begin() if they really
>> need it - I don't mind the syntax being a bit heavier for that case to
>> discourage raw iterator usage a little bit)
>>
>> I just copied the naming from the iterator immediately prior to this one
>> in SelectionDAGNodes.h (so honestly that means I didn’t even take the time
>> to notice about the naming.  Probably should have).  It had op_begin(),
>> op_end() and ops().  I took a look at Value.h for what is one of the most
>> commonly used iterators.  In there we have use_begin(), use_end(), and
>> uses().  So I think my naming is fairly consistent, but I can see your
>> point here.
>>
>> I like the idea of just removing begin()/end().  I can’t think of a
>> reason to iterate over a subset of the operands to a SDNode, so I’ve
>> removed that.
>>
>> What do you think?
>>
>> Cheers,
>> Pete
>>
>>
>>
>>
>> On Fri, Jun 26, 2015 at 10:29 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> 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/dd0bfbe8/attachment.html>


More information about the llvm-commits mailing list