<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 14, 2019 at 1:15 PM Eli Friedman via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.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">efriedma added a comment.<br>
<br>
> Handling what Wei's case will be a nice thing to have, but it may require more significant change in JT.<br>
<br>
I think we need to have a plan for what this is going to look like, so the new code here doesn't immediately become obsolete.<br>
<br></blockquote><div><br></div><div>Longer term, I think we should have a different IR optimization pass to do simplification (non controlflow) based tailDuplication pass. Most of the code in JT pass can be reused, so a lot of refactorization is needed to make majority of the code into a utility.</div><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">
> Another thing to consider is the cost model difference.<br>
<br>
I think the necessary cost model is effectively the same for ret vs. other instructions.  In particular, the "ret" might go away after after inlining.<br>
<br></blockquote><div><br></div><div>Right. This is actually another reason handling 'ret' belongs to JT because after inlining it will most likely become a normal jump-threading candidate. However we should not fully rely on inlining transformation to enable this optimization.</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">
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2005<br>
     LVI->enableDT();<br>
   LVI->threadEdge(PredBB, BB, SuccBB);<br>
<br>
----------------<br>
I'm surprised threadEdge still works.  Probably want to update the documentation for that API if we really want to allow SuccBB to be null.<br>
<br></blockquote><div><br></div><div>I looked at the code  -- it seems to handle this by design. It is a worklist based method that drops cached value entries in old SuccBB and all its descendents (except from NewSuccBB).  If the oldSuccBB does not have any successors, it will just drop values in it and terminates.</div><div><br></div><div><br></div><div><br></div><div><br></div><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">
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D68898/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D68898/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D68898" rel="noreferrer" target="_blank">https://reviews.llvm.org/D68898</a><br>
<br>
<br>
<br>
</blockquote></div></div>