<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
</head>
<body dir="ltr">
<div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<p>> If you are willing to commit to making it better, i'm happy to review this version :)<br>
Yes, I'll update gvn-hoist once post-dominator patch is merged.</p>
<p><br>
</p>
<p>Thanks,</p>
<p>-Aditya<br>
</p>
<br>
<br>
<div style="color: rgb(0, 0, 0);">
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="divRplyFwdMsg" dir="ltr"><font style="font-size:11pt" color="#000000" face="Calibri, sans-serif"><b>From:</b> Daniel Berlin <dberlin@dberlin.org><br>
<b>Sent:</b> Tuesday, August 1, 2017 11:11 AM<br>
<b>To:</b> reviews+D35918+public+40ddab4ebe264bbb@reviews.llvm.org<br>
<b>Cc:</b> Aditya Kumar; Sebastian Pop; Geoff Berry; Davide Italiano; Kuba Kuderski; Piotr Padlewski; llvm-commits<br>
<b>Subject:</b> Re: [PATCH] D35918: [GVNHoist] Factor out reachability to search for anticipable instructions quickly</font>
<div> </div>
</div>
<div>
<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>
</div>
</div>
</div>
</body>
</html>