<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="font-size:12.8px">That may actually be fast enough already if you do that, which i'm hoping (but you know better than I) is not a lot more work than you have here.</span></blockquote><div>There are a couple of complications with this method as I think about it. The first is the existence of DT used outside of the Jumpthreading pass: Local.cpp and other helper functions like the ones in BasicBlockUtils.cpp. I'll have to flush before calling these sites, or rework the interfaces to them to have an optional vector passed around. The hand-off between these separate functions will require me figuring out the most efficient way of updating the DT.</div><div><br></div><div>The second is I haven't done the analysis of exactly where I need the DT analysis preserved within the JT pass. Part of the reason we did the work this way was so we didn't have to worry about the state of DT, we could just use it at any point. This proposal changes it to be a much more explicit sync-then-analysis event.</div><div><br></div><div>I suspect both of these will cause more frequent than "block is deleted so flush DT" style code. It might be more efficient to batch the updates, I'm not sure.</div><div><br></div><div>Finally, I'd need a way to detect if the root block is to be removed from the function: in that case I have to recalculate the DT.</div><div><br></div><div>If this is the recommendation going forward I can start on this work.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 13, 2017 at 12:44 PM, 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">and just because i don't think i said it:<div>I know it is a large amount of work to get it to this point at all, and I am *very* glad to see this happening.</div><div>I think to get it over the finish line, unfortunately, we will have to choose "at what point do we actually need DT up to date" based on cost tradeoffs, , and make batching work for doing that.</div><div><br></div><div>The good news is "it should already work except where blocks are completely removed"</div><div><br></div><div>IE you should already be able to defer *all* updates to the point a block is removed, and only call applyUpdates when a block is erased.</div><div><br></div><div>That may actually be fast enough already if you do that, which i'm hoping (but you know better than I) is not a lot more work than you have here.</div><div><br></div><div>If you want to cut down on plumbing, i know all these things have DT anyway.</div><div>We could have DT be able to hand out the updater itself as a singleton.</div><div><br></div><div>(Otherwise, we should encapsulate the update vector instead of passing it directly. It probably only needs to take a DT and have functions called addEdgeUpdate and applyUpdates)</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 13, 2017 at 10:31 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>On Fri, Oct 13, 2017 at 9:41 AM, Brian M. Rzycki <span dir="ltr"><<a href="mailto:brzycki@gmail.com" target="_blank">brzycki@gmail.com</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">Hi Daniel, thanks for coming back to the conversation.<span><div><br></div><div><br></div><div><span style="font-size:12.8px">> </span><span style="font-size:12.8px">In that sense, 4% is really really good.  You are saving probably</span></div><div><span style="font-size:12.8px">> 100's or 1000's of dominator tree rebuilds to get the equivalent preservation.</span></div></span><div><span style="font-size:12.8px">One of my first attempts at preserving DT across JT was to simply use recalculate(). It was terribly slow but helped me to identify the areas where DT needed to be properly preserved with the new API.</span></div><span><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">> This is being done presumably to enable some better optimization.</span><span style="font-size:12.8px"><br></span></div></span><div><span style="font-size:12.8px">Correct. The JT pass in LLVM misses opportunities GCC catches. Two of the areas Sebastian and I would like to explore are threaded jumps across loop boundaries and 2 (or more) blocks threaded in one pass. DT is essential to adding LI analysis preservation to detect loops for jumping.</span></div><span><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">> </span><span style="font-size:12.8px">The only other option is to batch more (which i believe should just work.</span></div><div><span style="font-size:12.8px">> You should be able to keep a more global update vector, and apply less,</span></div><div><span style="font-size:12.8px">> and it should just work)</span></div></span><div><span style="font-size:12.8px">The jumpthreading pass makes heavy use of functions in Local.cpp and a few other calls that update DT like the ones in BasicBlockUtils. It would take coordination between these different call sites to use the global data structure. </span></div></div></blockquote><div><br></div></span><div>Or an updater object :)</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><span style="font-size:12.8px">If there was some way to "defer" updates in the DT interface that may help mitigate some of this overhead. Something like:</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">DT->deleteEdgeDeferred(BB, BBFoo);</span></div><div><span style="font-size:12.8px">...</span></div><div><span style="font-size:12.8px">DT->deferredUpdates(UpdateVect<wbr>or);</span></div><div><span style="font-size:12.8px">...</span></div><div><span style="font-size:12.8px">DT->applyDeferredUpdates();</span></div></div></blockquote><div><br></div></span><div>This should already work.</div><div>The batch updater should be able to apply the updates whenever you want.  It should not require they be applied in lockstep with the CFG.</div><div><br></div><div>The only requirement, i believe, is that the CFG nodes themselves (not the edges) don't go completely away because it literally needs to be able to query them without use after free (so it can, for example, get the DT node for a CFG node).</div><div>This seems fixable to me.</div><div>If you have an updater object, you use WeakVH for the CFG nodes so they go away without use after free, and Updater.add stores a pointer to the DT node so we can call deleteunreachable on it (by definition, any completely erased cfg node must be unreachable, or else you are going to have a bad time anyway).</div><span><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">It would also have the benefit if people use the older API then DT will know to implicitly applyDeferredUpdates before executing the older functions.</span></div><div><span style="font-size:12.8px"><br></span></div></div></blockquote><div><br></div></span><div>I suggested this interface at one point to Kuba, but it turns out there are things that directly access the bits of DT that make this likely to fail as an interface, unfortunately.</div><div><br></div></div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>