<div dir="ltr"><div>Ping ..</div><div><br></div>Chandler,  do you have time to reply to Bob's question?  What this patch does essentially is to enforce a global limit on for MemDep queries instead of using a per-query local limit as is done today. <div><br></div><div>Your suggestion of having a fixed limit in DSE loop may not work well in some cases and lead to more pessimistic result. The main problem with that approach is that it does not know how expensive (how many instructions are touched in backward walking) in each individual query. If there is a way to communicate this cost information back to the caller of MD interface, that will be fine too -- but that is essentially the same as what this patch does.</div><div><br></div><div>We have been beaten badly by this problem in both sanitizer builds and FDO builds.  With Sanitizer buiild, we can throw in O0 as a walkaround (but also suffer in runtime), but for FDO this is not an option.</div><div><br></div><div>David</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 12, 2016 at 2:59 PM, Bob Haarman <span dir="ltr"><<a href="mailto:robbert@fb.com" target="_blank">robbert@fb.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">inglorion added a comment.<br>
<br>
Thank you for all your comments, folks. I will be happy to improve this patch so we can get stop the long compiles while a better solution is being worked on.<br>
<br>
@chandlerc, I am not 100% sure I understand your suggestion. If I understand you correctly, you are saying that you think it is bad API design to add the getDefaultBlockScanLimit to MemDep and allow clients of MemDep to pass in a limit to getPointerDependencyFrom. Ok. Then you say "we should first bound the DSE scan when it has to call through to MemDep" and "you should just add a vanilla input here" (in getSimplePointerDependencyFrom). What do you mean? Perhaps you could illustrate the API you're looking for with some pseudocode.<br>
<br>
For what it's worth, what I am trying to accomplish with this patch is essentially to better enforce the limit that MemDep already has. Without the patch, we limit the number of instructions we will examine in a single call to getSimplePointerDependencyFrom to (by default) the last 100 instructions before the one where the search starts. In DSE, we then look at the result to determine if it lets us determine whether we can definitely perform or definitely not perform the optimization, but, in some cases, the result actually doesn't tell us one way or another, so we call into MemDep again to get the next possibly relevant instruction. This then searches up to 100 instructions from the location of the previous result. Since this can continue indefinitely, we can end up way more than 100 instructions away from where the search originally started, which is what causes the long compiles. This patch limits the search to 100 instructions (or whatever the parameter is set to) from where the DSE pass started the search, effectively closing the loophole that let us run past the limit the original search had.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D15537" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15537</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>