<div dir="ltr">If you are willing to commit to making it better, i'm happy to review this version :)</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 1, 2017 at 9:04 AM, Aditya Kumar via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">hiraditya added a subscriber: kuhar.<br>
hiraditya added a comment.<br>
<span class=""><br>
> Such a mapping is by definition n^2 space, and i'm having trouble seeing why it is necessary.<br>
><br>
> Here is what you are doing:<br>
>  For each instruction with the same VN:<br>
><br>
>   find the post-dominance frontier of the block of the instruction<br>
>   Insert a chi there, with certain arguments.<br>
><br>
><br>
> This is unnecessarily wasteful (you may compute the same pdf again and again).<br>
><br>
> Here is what SSUPRE (and you), should do:<br>
><br>
> Collect all the blocks of the instructions with the same VN into defining blocks.<br>
>  Compute the PDF using IDFCalculator.<br>
>  Place empty chis in the PDF.<br>
><br>
> At this point, you have two options:<br>
><br>
>   Walk post-dominator tree top-down and use a stack to store the last value you see.<br>
>   When you hit a chi from a given edge, the value to use as the argument is at the top of the stack.<br>
><br>
><br>
><br>
> This is O(Basic Blocks)<br>
<br>
</span>I tried this based on your suggestions, but post-dominator tree does not work well with infinite loops or CFG with multiple exits.<br>
I can wait on @kuhar 's patch to be merged+stabilize and then I can work on this idea.<br>
<span class=""><br>
> The O(instructions+chis) way to do it is:<br>
><br>
> Make a vector of instructions and chi argument uses.  Each should be given the DFS in/out number  from the post dominator tree node for the basic block they come from, and a local DFS number (IE order in block) in the case of instructions<br>
>  A "chi argument use" is created for each incoming edge to the chi, but is empty/fake.  These should assume the basic block from the other side of the edge (IE not the chi block, but the edge to the chi block).<br>
>  Sort by dfs in/out, then local number<br>
><br>
>   Walk vector with a stack.<br>
>   At each element of vector:<br>
>     while( !top of stack is empty && DFS in/out  of current thing in vector is not inside of DFS number of top of stack)<br>
>      pop stack<br>
>   If element you are staring at is a chi use:<br>
>     if stack is empty, chi has null operand<br>
>     if stack is not, set chi argument for the edge to top of stack<br>
>   else: // must be an instruction<br>
>       if stack is empty, push onto stack<br>
>      If stack is not empty, the thing on the stack post-dominates you and you are redundant :)<br>
><br>
><br>
><br>
><br>
> The forwards version of this algorithm is used by predicateinfo to do SSA renaming.<br>
>  Your algorithm is the same on the reverse graph, except the chi arguments are virtual :)<br>
<br>
</span>Because this patch precomputes ANTIC points based on your suggestions, it is already faster than the previous fix of iterating on dominator tree for each instruction to be hoisted.<br>
Can we merge this patch if you think it is good to go, and then I'll work on this idea once the post-dominator patch by @kuhar is ready.<br>
Thank you for writing the algorithm here, it helped me realize how close the algorithm is to PHI-insertion.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D35918" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D35918</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>