<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 26, 2015 at 11:08 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Jun 26, 2015, at 11:06 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr">Looks generally good<br></div></div></blockquote>I’m hoping that means LGTM. Sounds like it :)</div></div></blockquote><div><br>Yep, sorry for the ambiguity - with the below question resolved however you see fit, please commit.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class=""><br><blockquote type="cite"><div><div dir="ltr"><br>Is it idiomatic to use SDValues by value, </div></div></blockquote></span>I wouldn’t say idiomatic. Inconsistent might be a better word :)<span class=""><br><blockquote type="cite"><div><div dir="ltr">rather than (possibly const) ref?</div></div></blockquote></span>I’ll change it to const ref here. It does actually make more sense to use const ref where possible.</div></div></blockquote><div><br></div><div>Sounds good - thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div class="h5"><br><blockquote type="cite"><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 26, 2015 at 11:00 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span><blockquote type="cite"><div>On Jun 26, 2015, at 10:33 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr">As to the code in the patch itself:<br><br>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)<br></div></div></blockquote></span>You’re right, they do define one. I needed my own operator* :</div><div><br></div><div><div style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(187,44,162)">const<span> </span><span style="color:#4f8187">SDValue</span><span> &</span>operator<span>*() </span>const<span> { </span>return<span> </span><span style="color:#703daa">I</span><span>-></span><span style="color:#31595d">get</span><span>(); }</span></div></div><div><br></div><div>but ultimately the iterator_facade_base which we inherit from provides -> in terms of *.</div><div><span><blockquote type="cite"><div><div dir="ltr"><br>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)</div></div></blockquote></span>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.</div><div><br></div><div>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.</div><span><div><br></div><div>What do you think?</div><div><br></div><div>Cheers,</div><div>Pete</div><div><br></div><div></div></span></div><br><div style="word-wrap:break-word"><div><br><blockquote type="cite"><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 26, 2015 at 10:29 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On Fri, Jun 26, 2015 at 10:18 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div><blockquote type="cite"><div>On Jun 25, 2015, at 5:49 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><br><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Thu, Jun 25, 2015 at 5:36 PM, Pete Cooper<span> </span><span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span><span> </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"><div style="word-wrap:break-word"><br><div><span><div>On Jun 25, 2015, at 5:21 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><br><br>On Thu, Jun 25, 2015 at 5:11 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><br><div>On Jun 25, 2015, at 2:40 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><br><br>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><br>This immediately raises red flags:<br><br> SDNode *operator*() const { return I->getNode(); } SDNode *operator->() const { return operator*(); }<br><br>op* should return a T& and op-> should return T*<br>I’d forgotten about that.<br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><br>If these SDNode*s can never be null, then perhaps this should be:<br></div></div></div></div></div></div></blockquote>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.<div><br>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.<br></div></span>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.<span><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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"><div style="word-wrap:break-word"><div><br></div><div></div></div><br><div style="word-wrap:break-word"><div></div><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><br> <span> </span>SDNode &operator*<br> <span> </span>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></div></div></div></div></blockquote>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.<br></div></div></blockquote><div><br>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.<br><br>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->.</div></div></div></div></div></blockquote></span>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.</div></div></blockquote><div><br>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.<br></div></div></div></blockquote></div></div>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.</div><div><br></div><div>I could have done this</div><div><br></div><div>for (const SDNode &Elt : N->op_values()) {<br> if (const ConstantSDNode *C = dyn_cast<ConstantSDNode>(&Elt)) {</div><div><br></div><div>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.</div></div></blockquote></div></div><div><br>What went wrong there?<br><br>(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*)<br> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>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.<br></div></div></blockquote><div><br></div></span><div>Ah, it's already implemented? Good-o then.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div></div><div>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.</div></div></blockquote></span><div><br>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*<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><div style="word-wrap:break-word"><div> Then the dyn_cast can be left unchanged.<br></div><div><br></div><div>What do you think?</div><div><br></div><div>Cheers,</div><div>Pete</div><div><br></div><div></div></div><br></span><div style="word-wrap:break-word"><div><br><blockquote type="cite"><div><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div><br><span>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.</span></div><span><div><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"><div style="word-wrap:break-word"><div><br></div><div>Thanks,</div><div>Pete<span><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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"><div style="word-wrap:break-word"><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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> <span> </span>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></div></div></div></div></blockquote>Interesting. I hadn’t though to use Optional. I might try to implement something like this if i get time.</div><div><br></div><div>Cheers,</div><div>Pete<br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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>Cheers,<br>Pete</blockquote></div></div></div></div></blockquote></div></div></blockquote></div></div></div></div></blockquote></span></div></div></blockquote></span></div></div></blockquote></div><br></div><br></blockquote></div><br></div></div>
</blockquote></div><br></div>
</div></blockquote></div><br></div><br></blockquote></div><br></div>
</div></blockquote></div></div></div><br></div></blockquote></div><br></div></div>