[PATCH] Add iterator_adaptor to iterate over SDNode operand SDNode's
Pete Cooper
peter_cooper at apple.com
Thu Jun 25 17:11:30 PDT 2015
> 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 <mailto: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.
>
> 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.
>
>
> 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/8fbbb763/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: null-sdnodes.patch
Type: application/octet-stream
Size: 5157 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150625/8fbbb763/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150625/8fbbb763/attachment-0001.html>
More information about the llvm-commits
mailing list