<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jun 25, 2015, at 5:49 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><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; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Thu, Jun 25, 2015 at 5:36 PM, Pete Cooper<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><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;" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Jun 25, 2015, at 5:21 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Jun 25, 2015 at 5:11 PM, Pete Cooper<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><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;" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Jun 25, 2015, at 2:40 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Thu, Jun 25, 2015 at 2:30 PM, Pete Cooper<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><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 class=""><br class="">Currently the only SDNode iterator over operands does so with SDUse*. Users frequently then call getNode() on the operand.<br class=""><br class="">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 class=""><br class="">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 class=""></blockquote><div class=""><br class="">This immediately raises red flags:<br class=""><br class=""><div class=""> <span class="Apple-converted-space"> </span>SDNode *operator*() const { return I->getNode(); }</div><div class=""> <span class="Apple-converted-space"> </span>SDNode *operator->() const { return operator*(); }<br class=""><br class="">op* should return a T& and op-> should return T*<br class=""></div></div></div></div></div></div></blockquote></span>I’d forgotten about that.<span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><div class=""><br class="">If these SDNode*s can never be null, then perhaps this should be:<br class=""></div></div></div></div></div></div></blockquote></span>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></div></blockquote><div class=""><br class="">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 class=""></div></div></div></div></div></blockquote></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 class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </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;" class=""><div class=""><br class=""></div><div class=""></div></div><br class=""><div style="word-wrap: break-word;" class=""><div class=""></div><div class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><div class=""><br class=""> <span class="Apple-converted-space"> </span>SDNode &operator*<br class=""> <span class="Apple-converted-space"> </span>SDNode *operator-><br class=""><br class="">? (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 class=""></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 class=""></div></div></blockquote><div class=""><br class="">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 class=""><br class="">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 class=""><br class="">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 class=""></div></div></div></blockquote>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 class=""></div><div>I could have done this</div><div><br class=""></div><div>for (const SDNode &Elt : N->op_values()) {<br class=""> if (const ConstantSDNode *C = dyn_cast<ConstantSDNode>(&Elt)) {</div><div><br class=""></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><br class=""></div><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.</div><div><br class=""></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. Then the dyn_cast can be left unchanged.</div><div><br class=""></div><div>What do you think?</div><div><br class=""></div><div>Cheers,</div><div>Pete</div><div><br class=""></div><div></div></body></html>