<div dir="ltr"><div dir="ltr">On Tue, Mar 10, 2020 at 9:46 AM Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com">nhaehnle@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">Hi Alina,<br>
<br>
On Tue, Mar 10, 2020 at 4:31 PM Alina Sbirlea via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
> 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.<br>
> 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.<br>
>  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.<br>
<br>
So the specific reason I was asking is that for GPU applications, we<br>
are interested in potentially having two CFGs overlaying the same set<br>
of basic blocks. Due to the SIMT nature of execution there are two<br>
types of control flow.<br>
<br>
On second thought, however, it probably doesn't make much of a<br>
difference for that particular problem whether succ_begin/end are<br>
global or member functions.<br>
<br>
And from other perspectives, having them as member functions seems<br>
slightly better because it makes it easier to find them. So in<br>
summary, I suppose either way is fine, as long as we're making things<br>
consistent :)<br></blockquote><div><br></div><div>Sounds good :). Thank you for your input!</div><div><br></div><div>Best,</div><div>Alina</div><div><br></div><div>Alina</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Cheers,<br>
Nicolai<br>
<br>
><br>
><br>
> Thanks,<br>
> Alina<br>
><br>
> On Mon, Mar 9, 2020, 4:16 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>> On Mon, Mar 9, 2020 at 3:57 PM Alina Sbirlea via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>>><br>
>>> Hi,<br>
>>><br>
>>> 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.<br>
>><br>
>><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>
>><br>
>>><br>
>>> In particular, the need arose for BasicBlocks, MachineBasicBlocks, VPBlockBase and clang::CFGBlock.<br>
>>><br>
>>> The least invasive change seemed to be to use the interface already being used in MachineBasicBlock and clang::CFGBlock, and:<br>
>>> (1) update BasicBlock to use this instead of the "global" `succ_iterator` in IR/CFG.h<br>
>>> (2) add the same interfaces in VPBlockBase as simple wrappers over existing Successors/Predecessors vectors.<br>
>>><br>
>>> 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.<br>
>>><br>
>>> For some concrete view of what the changes look like, I uploaded two preliminary patches:<br>
>>> (1) part 1: D75881: Introducing class specific iterators<br>
>>> (2) D75882<br>
>>> (1) part 2: pending: Cleaning up existing usages; example replacement: `succ_begin(BB)` with `BB->succ_begin()`.<br>
>>> (1) part3/4: pending: Add class specific iterators to `Instruction` and clean up existing usages just as for `BasicBlock`.<br>
>>><br>
>>> 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.<br>
>>><br>
>>> I welcome comments on this, and if there's something I missed explaining please let me know.<br>
>>><br>
>>> Thank you,<br>
>>> Alina<br>
>>><br>
>>><br>
>>><br>
>>><br>
>>> _______________________________________________<br>
>>> LLVM Developers mailing list<br>
>>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
>>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
<br>
<br>
<br>
-- <br>
Lerne, wie die Welt wirklich ist,<br>
aber vergiss niemals, wie sie sein sollte.<br>
</blockquote></div></div>