<div dir="ltr">Yup, that solves my problem :-). Thanks a lot for the taking the time to understand and address the underlying issue!<div><br></div><div>I have a couple of NFC patches sent out that I think are still worth checking-in (<a href="https://reviews.llvm.org/D75952">D75952</a>, <a href="https://reviews.llvm.org/D75962">D75962</a>), but the others I'll abandon.</div><div><br></div><div>Best,</div><div>Alina</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 12, 2020 at 10:52 AM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Think we got this all figured out with: <a href="https://reviews.llvm.org/D76034" target="_blank">https://reviews.llvm.org/D76034</a> - but if there's more issues here (or just general C++-y design questions, etc) more than happy to discuss them here/other threads/whenever :)</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Mar 10, 2020 at 4:42 PM Alina Sbirlea <<a href="mailto:alina.sbirlea@gmail.com" target="_blank">alina.sbirlea@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Tue, Mar 10, 2020 at 2:30 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Mar 10, 2020 at 8:31 AM Alina Sbirlea <<a href="mailto:alina.sbirlea@gmail.com" target="_blank">alina.sbirlea@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div>Hi Dave,</div><div dir="auto"><br></div><div dir="auto">It may be possible to do this with the current API, but what I was looking for is a common API for existing block types. For example there is no succ_begin for Machine BasicBlock.  </div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"><br></div><div dir="auto">I'm looking to make the CFGSuccessors and CFGPredecessors classes in CFGDiff.h templated, and this needs a common API for all types instantiations. </div><div dir="auto"><br></div><div dir="auto">Does this clarify your question or did I misunderstand your suggestion?</div></div></blockquote><div><br></div><div>Possibly some misunderstanding - sounds like Nicolai had the same suggestion phrased more clearly.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto">Nicolai,<br></div><div dir="auto"><br></div><div dir="auto">Yes, I considered declaring the "global" succ_begin for the other block types, but it seems like a more complex change (probably to declare the type as Dave described, and these need adding for 3 more types vs 2 now) and these wouldn't be used anywhere else.</div></div></blockquote><div><br>What do you mean by "adding 3 more types vs 2 now" - where the iterator types are written is pretty separate from this API change (even if the iterator types remain non-members - the succ_begin non-member -> member functions with a member type could be achieved by using a member typedef rather than a member /type/ )<br></div></div></div></blockquote><div><br></div><div>I meant adding the global APIs for MachineBasicBlock, VPBlockBase, clang::CFGBlock (3), vs adding member APIs in BasicBlock and VPBlockBase (2).</div><div>This is only because the use case I am looking at involves the DominatorTree and there are no instantiations around other types (like you mentioned: Interval). </div><div>Looking closer at the comments for Interval, I see that the global APIs were added to mirror the BasicBlock ones, and they're essentially wrappers over iterators expressed as class members.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br>So currently we have:<br><br>class Instruction;<br>class BasicBlock;<br>class Interval;<br><br>succ_begin(Instruction*)<br>succ_begin(BasicBlock*)<br>succ_begin(Interval*)<br>(& some kind of Region thing in RegionIterator.h)<br><br>and you'd like to generalize this over more types, MachineBasicBlocks, VPBlockBase and clang::CFGBlock?<br><br>So the suggestion would be to add:<br><br>succ_begin(MachineBasicBlock*)<br>succ_begin(VPBlockBase*)<br>succ_begin(CFGBlock*)<br><br>what would be the negative side of adding that, rather than porting the extra 3 to member functions?<br></div></div></div></blockquote><div><br></div><div>I don't think there is any negative, tbh. I simply found it clearer to reason about these as class member methods, and, looking at the existing cases, the "global" API approach looked to me to be the outlier.</div><div>I also found somewhat confusing that, for Instructions, there are defined successor iterators via the global APIs but not predecessors, and these are in a separate file (CFG.h) so it's not as obvious this is the case. If these were class iterators, this would stand out.<br></div><div><br></div><div>Thinking more, I think your suggestion to add the global ones is easier to implement as as quick solution:</div><div>succ_begin(MachineBasicBlock*)<br>succ_begin(VPBlockBase*)<br>succ_begin(CFGBlock*)<br></div><div>just like you suggested, as these are just wrappers over the class member methods (like those defined for Interval).</div><div>If folks feel this is a cleaner/better approach, I'm happy to work towards that.</div><div><br></div><div>The member methods still seem cleaner to me, but I realize I'll have to sign up for changing the whole code-base to use that if going that route :-).</div><div>Again, as long as the end result is consistent, I'm flexible to go either way.</div><div><br></div><div>Best,</div><div><div></div></div><div>Alina</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto">AFAICT, there is no issue with replacing the current "global" iterators with class specific ones, they are already used as such. But perhaps I don't have the full picture.</div><div dir="auto"> In the first patch I put up, the iterators added inside BasicBlock can co-exist with the global ones, so the switch can be done incrementally.</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto">Thanks,</div><div dir="auto">Alina</div><div dir="auto"><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr">On Mon, Mar 9, 2020, 4:16 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 9, 2020 at 3:57 PM Alina Sbirlea via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi,<div><br></div><div>As part of an ongoing work to extend the GraphDiff (this models a CFG view), I came across the need to have a common interface for accessing successors/predecessors in various IR units, such that a type such as `typename NodeT::succ_iterator` could be used in templated code.</div></div></blockquote><div><br>I /think/ this can be achieved with the existing API by using "decltype(succ_begin(std::declval<NodeT>()))" instead of the typename you've got as an example (it looks like succ_begin is the extension point - but the problem you're having is naming its return type? decltype would be one option) - you could make a trait wrapper around that or the like if you need this type name in a bunch of disparate places (where they can't share a typedef).<br><br>Would that suffice? are there other aspects of your use case that don't line up well with the existing non-member/overload API?<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>In particular, the need arose for BasicBlocks, MachineBasicBlocks, VPBlockBase and clang::CFGBlock.</div><div><br></div><div>The least invasive change seemed to be to use the interface already being used in MachineBasicBlock and clang::CFGBlock, and: </div><div>(1) update BasicBlock to use this instead of the "global" `succ_iterator` in IR/CFG.h</div>(2) add the same interfaces in VPBlockBase as simple wrappers over existing Successors/Predecessors vectors.<div><br></div><div>I've been working on a few patches to make this happen, but I'd like the community's thoughts on this before deep-diving into code reviews.</div><div><br></div><div>For some concrete view of what the changes look like, I uploaded two preliminary patches:</div><div>(1) part 1: <a href="https://reviews.llvm.org/D75881" style="text-decoration-line:none" rel="noreferrer" target="_blank"><span style="font-size:11pt;font-family:Arial;background-color:transparent;font-variant-numeric:normal;font-variant-east-asian:normal;text-decoration-line:underline;vertical-align:baseline;white-space:pre-wrap">D75881</span></a>: Introducing class specific iterators</div><div>(2) <a href="https://reviews.llvm.org/D75882" style="text-decoration-line:none" rel="noreferrer" target="_blank"><span style="font-size:11pt;font-family:Arial;background-color:transparent;font-variant-numeric:normal;font-variant-east-asian:normal;text-decoration-line:underline;vertical-align:baseline;white-space:pre-wrap">D75882</span></a></div><div>(1) part 2: pending: Cleaning up existing usages; example replacement: `succ_begin(BB)` with `BB->succ_begin()`.<br></div><div>(1) part3/4: pending: Add class specific iterators to `Instruction` and clean up existing usages just as for `BasicBlock`.</div><div><br></div><div>I split the above (1) just to clarify what interfaces are added versus the NFC cleanups that follow. But it could be done just as well in a single patch.</div><div><br></div><div>I welcome comments on this, and if there's something I missed explaining please let me know.</div><div><br></div><div>Thank you,</div><div>Alina</div><div><br></div><div><br></div><div><br></div><div><br></div></div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>
</blockquote></div></div></div>
</blockquote></div></div>
</blockquote></div></div>
</blockquote></div>
</blockquote></div>