[llvm-commits] PATCH: Fix quadratic behavior in PR12652

[Jakob Stoklund Olesen]

On Jul 6, 2012, at 1:28 AM, Chandler Carruth <chandlerc at gmail.com> wrote:

> 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.

Thanks for fixing this, Chandler.

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.

Could you measure the compile time impact on something mundane, like compiling clang, please?

> 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.

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).

To exploit this common case (LHS large, RHS tiny, LHS.size() doesn't change), you could:

1. Start with a binary search in case RHS starts in the middle of a huge LHS.

2. Merge directly into LHS whenever possible, use NewRanges only as an overflow area.

3. Grow LHS to the required size, and going backwards, merge NewRanges into LHS.

> 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.

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.

Even an old-fashioned dataflow analysis scales as #variables x #blocks.

> 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.

Hypermath? ;-)

+/// \brief Helper function for merging in another LiveInterval's ranges.
+///
+/// This is a helper routine implementing an efficient merge of another
+/// LiveIntervals ranges into the current interval. It accepts an optional
+/// value number constraint for the RHS, and a new value number to assign to
+/// each range merged into the interval.
+void LiveInterval::mergeIntervalRanges(const LiveInterval &RHS,
+                                       VNInfo *LHSValNo,
+                                       const VNInfo *RHSValNo) {

Please explain more clearly what the ValNo arguments do. I couldn't make it out without reading the code.

+  if (RHS.begin() == RHS.end())

RHS.empty()

+      std::copy(LI, LE, std::back_inserter(NewRanges));

What Ben said.

+    // Select the first range. We pick the earliest start point, and then the
+    // largest range.
+    bool MergeRight = *RI < *LI;
+    LiveRange R = MergeRight ? *RI : *LI;
+    if (MergeRight)
+      R.valno = LHSValNo;
+    ++(MergeRight ? RI : LI);

I think a good old-fashioned if-else would be easier to read.

+    // Check if we have any ranges that this one might merge with. 
+    if (!NewRanges.empty()) {

Please flip this if around, so you can use continue and reduce indentation.

+        assert(R.start >= LastR.end &&
+               "Cannot overlap two LiveRanges with differing ValID's"
+               " (did you def the same reg twice in a MachineInstr?)");

Please drop the last line of that assert text. I don't think it applies any more.

Otherwise, LGTM.

/jakob