<div dir="ltr">(all that said, if you want to approve it as-is, i'm not going to object)</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 17, 2016 at 7:46 AM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Nov 16, 2016 at 9:57 PM, Mehdi AMINI <span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">mehdi_amini added a comment.<br>
<br>
Sorry for the delay, I keep getting interrupted in the middle of getting this code into my head, and paging out has a real cost when getting back to it. Especially as I don't really know the algorithm, I have to infer what you're doing from the implementation (which isn't a bad way to judge of the clarity of the code).<br>
<br>
Anyway, a few inline comments.<br>
<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/ADCE.cpp<wbr>:610<br>
<span class="m_-3050166353640861779gmail-">+  }<br>
+<br>
+  // Update each live phi based on the new incoming edges.<br>
----------------<br>
</span>david2050 wrote:<br>
<span class="m_-3050166353640861779gmail-">> dberlin wrote:<br>
> > Is the above really necessary?<br>
> ><br>
> > The standard way to do this, AFAIK, is to mark the useful block set while doing marking, and then just walking up the PDT to find the nearest block marked useful for each dead branch.<br>
> > Replace all uses of the bb with that block using RAUW.<br>
> > This should update phi nodes, since I believe the blocks in phi nodes are still considered uses.<br>
> ><br>
> I don't understand this approach. Given a live branch  (x,y) where y is dead and  z is the live post-dominator of y, we can't just replace uses of 'y' because the 'y' may not be directly referenced in phi nodes in 'z' which only will old references to its predecessors. Also, 'y' could have multiple branches into it which are dead.<br>
</span>I'm not sure I understood what @dberlin referred to, </blockquote><div><br></div></span><div>I am referring to pretty much what every compiler optimization class and textbook list as the way to do this :)</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">but as I read it it should be possible to just process DeadBlocks without computing the dead region, and only manipulate Phi in the live PDOM if the live PDOM is a direct successor of the current block. I think this should get the same result in term of Phi, but does not seems less costly than computing the region.<br></blockquote><div><br></div></span><div>In most compilers, it's like5-10 lines of code, so while maybe not less expensive, it's less complex.</div><span class=""><div><br></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">
Also I'm not sure if you can rewrite the branches from the live predecessors into the dead region to jump directly to the live PDOM without computing the region (but with the invariant you are already relying on to get a dead region with a unique live PDOM it may be possible).<br></blockquote><div><br></div></span><div>I linked both the papers and the implementation, but, like i said, this is the standard method.</div><div>Keith Cooper's comp 512 slides also go over it<br></div><div><a href="https://www.clear.rice.edu/comp512/Lectures/10Dead-Clean-SCCP.pdf" target="_blank">https://www.clear.rice.edu/<wbr>comp512/Lectures/10Dead-Clean-<wbr>SCCP.pdf</a><br></div><div><br></div><div>I believe it is also gone over in "<i style="font-family:times;font-size:medium"><a href="http://store.elsevier.com/product.jsp?isbn=9780120884780" style="font-family:times;font-size:medium" target="_blank">Engineering a Compiler, 2nd Edition</a>"<a href="http://store.elsevier.com/product.jsp?isbn=9780120884780" target="_blank"></a></i></div><div><br></div><div><br></div></div></div></div>
</blockquote></div><br></div>