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

[Chandler Carruth]

On Fri, Jul 6, 2012 at 9:10 AM, Jakob Stoklund Olesen <stoklund at 2pi.dk>wrote:

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

Yea, I'll dig a bit into the common case.


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

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.


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

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.

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.

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.


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

Yea, I'm going to look at making ASan actually produce "normal" block
layout.


>
> > 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? ;-)
>

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


>
> +/// \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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120706/99236f7a/attachment.html>