<div dir="ltr">(and note, you may be able to hack around this by making the overdefined cache  an interval map from <dfs pair, value>, and only cache for split points/whatever, but that seemed more work than starting to move it towards predicateinfo)</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 15, 2017 at 3:59 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">Sorry, i normally only scan llvm-commits mail occasionally (gotta reod my filtering), i didn't notice this until joerg pointed it out to me.<div><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Feb 8, 2017 at 9:29 PM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Danny,<br>
<br>
I feel like there's something missing here.  In particular, I'm not sure I agree with your description of the problem.  See inline comments below.  Can you help me understand why this is needed?</blockquote></span><div>Sure.</div><div>Happy to also provide a testcase if it helps.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><br>
<br>
On 02/08/2017 07:22 AM, Daniel Berlin via llvm-commits wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: dannyb<br>
Date: Wed Feb  8 09:22:52 2017<br>
New Revision: 294463<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=294463&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=294463&view=rev</a><br>
Log:<br>
LVI: Add a per-value worklist limit to LazyValueInfo.<br>
<br>
Summary:<br>
LVI is now depth first, which is optimal for iteration strategy in<br>
terms of work per call.  However, the way the results get cached means<br>
it can still go very badly N^2 or worse right now.  The overdefined<br>
cache is per-block, because LVI wants to try to get different results<br>
for the same name in different blocks (IE solve the problem<br>
PredicateInfo solves).  This means even if we discover a value is<br>
overdefined after going very deep, it doesn't cache this information,<br>
causing it to end up trying to rediscover it again and again.  The<br>
same is true for values along the way.<br>
</blockquote></span>
This doesn't parse for me.  We have the OverdefinedCache and do cache the result of a (Value, BB) pair being overdefined.  Other than the fact the cache is split - which admitted is confusing - where's the problem here?</blockquote></span><div>Given<br></div><div>BB 1</div><div>BB 2</div><div>BB 3</div><div>BB 4</div><div><br></div><div>Overdefined in one is not equal to overdefined in all.</div><div>So we cache in one bb</div><div>then ask about the same value in the next bb.</div><div>So it keeps rediscovering the overdefinedness of it.<br></div><div>16000 times per value.</div><div><br></div><div>If the cache was made global to each name, this would fix it.<br></div><div>But it doesn't have a global cache precisely because of the naming issue.</div><div>IE it thinks it can get a better answer in BB 2 than BB1.</div><div>And it, in fact, can in blocks dominated byconditionals</div><div>That's the whole reason predicateinfo exists.</div><div>To change the name in these cases so that you can associate singular info with it and still get good results.</div><div><br></div><div>Splitting this cache  is a very bad way to solve the fact that you may be able to prove different values for different uses.<br></div><div>IMHO, better to rename those uses.</div><span class=""><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In practice, overdefined<br>
anywhere should mean overdefined everywhere (this is how, for example,<br>
SCCP works).<br>
<br>
Until we get around to reworking the overdefined cache, we need to<br>
limit the worklist size we process.  Note that permanently reverting<br>
the DFS strategy exploration seems the wrong strategy (temporarily<br>
seems fine if we really want).  BFS is clearly the wrong approach, it<br>
just gets luckier on some testcases.  It's also very hard to design<br>
an effective throttle for BFS. For DFS, the throttle is directly related<br>
to the depth of the CFG.  So really deep CFGs will get cutoff, smaller<br>
ones will not. As the CFG simplifies, you get better results.<br>
In BFS, the limit is it's related to the fan-out times average block size,<br>
which is harder to reason about or make good choices for.<br>
</blockquote></span>
I agree that a threshold for DFS makes much more sense than for BFS.  I think having the threshold is a good idea, though I'd still like to understand the test case.  I suspect we should be able to use a much higher threshold value.</blockquote><div><br></div></span><div>Yes, we can.</div><div>I took the lowest point, where, on all of my testcases, it changed nothing, and multiplied by 5 :)</div><div>I did some other testing, and the percentage of cases i found where we searched through 500 more blocks than last time, and found a better answer, was 0%.<br></div><div><br></div><div>Maybe you have much worse testcases.</div><span class=""><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Bug being filed about the overdefined cache, but it will require major<br>
surgery to fix it (plumbing predicateinfo through CVP or LVI).<br>
<br>
Note: I did not make this number configurable because i'm not sure<br>
anyone really needs to tweak this knob.  We run CVP 3 times. On the<br>
testcases i have the slow ones happen in the middle, where CVP is<br>
doing cleanup work other things are effective at.  Over the course of<br>
3 runs, we don't see to have any real loss of performance.<br>
</blockquote></span>
I have a very different pass pipeline than you do.  Please don't forget that when tuning.  Over-fitting the current O2 pipeline is a path we don't want to pursue.<br></blockquote><div><br></div></span><div>Su <br></div><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Did you explore other values for the cutoff?  In particular, if we used 10,000 would your test case scale badly?</blockquote><div><br></div></span><div>Yes.</div><div>The max value in this testcase is 16000 per-value, and there are thousands of values it hits this on.</div><div><br></div><div>Conversely, i can't find a testcase where we dfs'd through 500 things and still found a good answer.</div><div><br></div><div><br></div></div></div></div></div>
</blockquote></div><br></div>