<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 25, 2015 at 2:30 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi David<br>
<br>
Currently the only SDNode iterator over operands does so with SDUse*.  Users frequently then call getNode() on the operand.<br>
<br>
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.<br>
<br>
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.<br></blockquote><div><br>This immediately raises red flags:<br><br><div>  SDNode *operator*() const { return I->getNode(); }</div><div>  SDNode *operator->() const { return operator*(); }<br><br>op* should return a T& and op-> should return T*<br><br>If these SDNode*s can never be null, then perhaps this should be:<br><br>  SDNode &operator*<br>  SDNode *operator-><br><br>? (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.<br></div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
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.,<br>
<br>
for (auto *C : dyn_cast<ConstantSDNode>(N->op_nodes()))<br>
<br>
just a thought, but thats unrelated to this patch for now.<br></blockquote><div><br>Yep, though probably more in the form of a filtered range, I suspect:<br><br>  for (auto &C : filtered_transform(N->op_nodes(), [](SDUse *U) { return U->getNode(); }))<br><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Cheers,<br>
Pete<br>
<br>
</blockquote></div><br></div></div>