<div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 6, 2012 at 9:10 AM, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:stoklund@2pi.dk" target="_blank" class="cremed">stoklund@2pi.dk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><br>
On Jul 6, 2012, at 1:28 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" class="cremed">chandlerc@gmail.com</a>> wrote:<br>
<br>
> This patch adds what seems to me to be a very efficient merge operation for LiveInterval to address the quadratic behavior seen in PR12652 when merging extremely large, extremely distant disjoint intervals created by ASan's strange code layout.<br>
<br>
</div>Thanks for fixing this, Chandler.<br>
<br>
It's good to get rid of the quadratic algorithm, but I am a bit concerned about the typical case because you are a) copying the whole thing on every call, and b) calling malloc/free at least once per call.<br>
<br>
Could you measure the compile time impact on something mundane, like compiling clang, please?<br></blockquote><div><br></div><div>Yea, I'll dig a bit into the common case.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> There is one other user of addRangeFrom left (join) and I think I can refactor this code a bit and share the merge logic for that routine as well. But I didn't want to do that in this patch.<br>
<br>
</div>I am not sure that join() should use the same algorithm. It gets called a lot with a very small RHS interval that merges into the existing large LHS without shifting things around. Your algorithm would turn O(logN) into O(N) in that case. (Plus the malloc thing).<br>
<br>
To exploit this common case (LHS large, RHS tiny, LHS.size() doesn't change), you could:<br></blockquote><div><br></div><div>Ah, I didn't realize that size not changing was the common case! It makes sense though. If size had been changing each time, just building a new vector doesn't seem so bad.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
1. Start with a binary search in case RHS starts in the middle of a huge LHS.<br>
<br>
2. Merge directly into LHS whenever possible, use NewRanges only as an overflow area.<br>
<br>
3. Grow LHS to the required size, and going backwards, merge NewRanges into LHS.<br></blockquote><div><br></div><div>Yea, I'll look at this (or at least something quite similar) so that we do as much as we can to leave the LHS in place when it doesn't need to grow.</div>
<div><br></div><div>I'll integrate this (as wel as your and Ben's comments) into a fresh patch shortly for you to look at before I commit.</div><div><br></div><div>I still think we should likely make join() use this code because we should make it handle that input pattern efficiently, but fall back on something that isn't super-linear when it hits edge cases.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> With this patch, the previously dominant entry on the profile (extendIntervalEndTo) disappears *completely* from the test case in the PR. I dunno how far down it is, but it's no longer significant. None of the merge logic appears to be significant. Instead overlaysFrom and findLocalKills start to dominate the profile.<br>
<br>
</div>The overlapsFrom() behavior is probably inevitable because of your test case. You are not only growing the number of basic blocks linearly, but also the number of globally live variables. Combined with the weird block layout, I doubt we can get rid of that quadratic behavior.<br>
<br>
Even an old-fashioned dataflow analysis scales as #variables x #blocks.<br></blockquote><div><br></div><div>Yea, I'm going to look at making ASan actually produce "normal" block layout.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> The performance which was previously quadratic is now roughly linear, but it's a rather unfortunate linear: 2x. Every doubling of the input size quadruples the compile time. Not sure how much time its worth driving this down versus teaching ASan to emit its basic blocks in a natural CFG order.<br>
<br>
</div>Hypermath? ;-)<br></blockquote><div><br></div><div>Ugh, sorry about that. Been staring at code too much. See my last email to Jay, I think that one actually explained matters much more clearly. =]</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
+/// \brief Helper function for merging in another LiveInterval's ranges.<br>
+///<br>
+/// This is a helper routine implementing an efficient merge of another<br>
+/// LiveIntervals ranges into the current interval. It accepts an optional<br>
+/// value number constraint for the RHS, and a new value number to assign to<br>
+/// each range merged into the interval.<br>
+void LiveInterval::mergeIntervalRanges(const LiveInterval &RHS,<br>
+ VNInfo *LHSValNo,<br>
+ const VNInfo *RHSValNo) {<br>
<br>
</div>Please explain more clearly what the ValNo arguments do. I couldn't make it out without reading the code.<br>
<div class="im"><br>
+ if (RHS.begin() == RHS.end())<br>
<br>
</div>RHS.empty()<br>
<div class="im"><br>
+ std::copy(LI, LE, std::back_inserter(NewRanges));<br>
<br>
</div>What Ben said.<br>
<div class="im"><br>
+ // Select the first range. We pick the earliest start point, and then the<br>
+ // largest range.<br>
+ bool MergeRight = *RI < *LI;<br>
+ LiveRange R = MergeRight ? *RI : *LI;<br>
+ if (MergeRight)<br>
+ R.valno = LHSValNo;<br>
+ ++(MergeRight ? RI : LI);<br>
<br>
</div>I think a good old-fashioned if-else would be easier to read.<br>
<div class="im"><br>
+ // Check if we have any ranges that this one might merge with.<br>
+ if (!NewRanges.empty()) {<br>
<br>
</div>Please flip this if around, so you can use continue and reduce indentation.<br>
<div class="im"><br>
+ assert(R.start >= LastR.end &&<br>
+ "Cannot overlap two LiveRanges with differing ValID's"<br>
+ " (did you def the same reg twice in a MachineInstr?)");<br>
<br>
</div>Please drop the last line of that assert text. I don't think it applies any more.<br>
<br>
Otherwise, LGTM.<br>
<span class="HOEnZb"><font color="#888888"><br>
/jakob<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" class="cremed">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>