<div dir="ltr">Daniel, thank you for the explanation. I completely agree if we can batch everything then it's a net win. Where I have concerns is in the gritty details of the function calls and how they interface with each other. While waiting on your response I started to look at just one place where I would need to do batch updates.<div><div><br></div><div>JumpThreadingPass::runImpl()</div><div> llvm::removeUnreachableBlocks()</div><div> static markAliveBlocks()</div><div> llvm::changeToUnreachable()</div><div><br></div><div>In this example I start batching DT requests for removeUnreachableBlocks. Because this function lives in Local.cpp, is a public interface, and removes blocks I decided this routine would isolate its own batch updates. This works well until I get to the call to changeToUnreachable(). CTU alters the state of the CFG and I need to know the changes for DT updates. I now have the following options:</div><div><br></div><div>1. change the public api and all its call-sites</div><div>2. attempt to use optional parameters and conditionally batch DT updates</div><div>3. view this function call as a synchronization event and force applyUpdates before calling.</div><div><br></div></div><div>For (1) this is error-prone and opens up lots of fun debug issues. I'd prefer to not go down this path.</div><div>For (3) we lose the whole point of doing batch updates in the first place.</div><div>For (2) I have to add three new optional variables: DT, a Vector reference, and a flag indicating batch mode. This will work but it's clunky.</div><div><br></div><div>Note that this isn't the only place I'll have to make this compromise. Here's another that makes me a lot more nervous:</div><div>JumpThreadingPass::SPlitBlockPredecessors()</div><div> SplitBlockPredecessors()</div><div><br></div><div>In this case SplitBlockPredecessors() does have DT support using the older API. It's defined in BasicBlockUtils.cpp. Again, I'd like to avoid option (1) and doing option (2) means I have to update the DT inside the routine to the new API as well as pass 2 optional parameters. In this case (3) makes the most sense, but I have no idea at what performance cost.</div><div><br></div><div>I am willing to do the work but it's going to take time (and a bit of guidance from others on how they want this interface to involve).</div><div><br></div><div>There's another issue: every edge that is updated via batch has to be checked to make sure it's not already in the list of updates. This means as I insert into Updates I have to do the find() != idiom every edge. This is O(n). As the size of the batch list gets larger that time becomes less trivial.</div><div><br></div><div>Reasons like these are why I was suggesting an alternative: have the pending queue of requests live inside the DominatorTree class and use methods on DT to enqueue or flush. It also has the benefit of being useful to others outside of JumpThreading and Local that will eventually want to use the new DT interface. Any call to methods other than the following automatically cause a flush of the pending queue:</div><div>deleteEdgePend()</div><div>insertEdgePend()</div><div>applyUpdatesPend()</div><div><br></div><div>It would also be nice if the pending inserts didn't care about duplicate edge update requests. That would make the call site much simpler and move the work to a single scrub of the list just before processing it.</div><div><br></div><div>Guidance on how to proceed is greatly appreciated. Frankly I've been fighting dominators inb JT now for about 4 months and I really just want to move onto the more productive work of improving the code generation in JumpThreading. :)</div><div><br></div><div>Thanks,</div><div>-Brian</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 13, 2017 at 2:38 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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Fri, Oct 13, 2017 at 10:48 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><span class="m_621160865706844078gmail-"><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">I *believe* the motivation is that they want DT up to date *in-pass* so they can use it do to better jump threading over loops.</span></blockquote></span><div><span style="font-size:12.8px">Daniel, you are correct in this assumption. Dominance is the first step in enhancing the kinds of analysis and extend the number of Jumpthreading opportunities. Analysis of loops is part of this work.</span></div><span class="m_621160865706844078gmail-"><div><span style="font-size:12.8px"><br></span></div><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* kind of change, IMHO, should be subject to our usual "how much faster code does it make vs how much does it cost" tradeoff.</span></blockquote></span><div>I have not started the work on actual code optimizations simply because I could not intelligently make the analysis decisions needed. Rather than create a huge change to JT (or another pass) Sebastian and I decided to incrementally update JT with the features we needed to get us to the end goal.</div><span class="m_621160865706844078gmail-"><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"><span style="font-size:12.8px">This was *our* plan to remove the recalculates, but it does not appear to have be what Sebastian/etc want for JT.</span></blockquote></span><div>I'm a bit confused by this statement: that's what the patch is doing. </div></div></blockquote><div><br></div></span><div>You are confused because i was talking about the recalculations that occur elsewhere.</div><div><br>But let me try to diagram it for you.</div><div><br></div><div>Here is the equivalent of what JT did before your patch<br><br></div><div><br></div><div>iterate</div><div> for each bb</div><div> maybe change an edge and some code</div><div> call some utilities<br></div><div> for each dead block</div><div> erase it</div><div>DT->recalculate<br></div><div><br></div><div>Here is what your patch would look like without the incremental API existing:<br>iterate</div><div> for each bb</div><div> maybe change an edge and some code</div><div> for each edge we changed:</div><div> DT->recalculate</div><div> call some utilities, calling DT->recalculate in each one</div><div> for each dead block</div><div> erase it</div><div> DT->recalculate</div><div><br></div><div>You can see that this has added a O(number of changes ) DT->recalculate calls compared to what happened before.</div><div><br></div><div>That would be ridiculously slow. Probably 100x-1000x as slow as the first case.</div><div><br></div><div>You then have changed that O(number of changes) set of calls to use the incremental API, so that it looks like this:<br><div>iterate</div><div> for each bb</div><div> maybe change an edge and some code</div><div> for each edge we changed:</div><div> incremental update DT</div><div> call some utilities, calling incremental update DT in each one</div><div> for each dead block</div><div> erase it</div><div> incremental update DT</div></div><div><br></div><div>These calls, unfortunately are not O(1) and will never be O(1) (doing so is provably impossible)<br></div><div>So it's not an O(N) cost you've added it, it's N^2 or N^3 in the worst case.</div><span class=""><div><br></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"><div dir="ltr"><div>There's only one corner-case where a recalculate occurs. The rest of the time it's incremental updates. There may still be opportunities to batch updates but I honestly have no idea if it will be more efficient. </div></div></blockquote><div><br></div></span><div>It will, because it will cause less updating in practice.</div><div><br></div><div>If jump threading changes edge A->B to A->C (IE insert new edge, remove old edge) and then changes edge A->C to A->D , you will perform two updates.</div><div>If those require full recalculation of the tree (internal to the updater), you will update the tree twice.</div><div>Batching will perform one.</div><div><br></div><div><br></div><div><br></div></div></div></div>
</blockquote></div><br></div>