[PATCH] [CaptureTracking] Avoid long compilation time on large basic blocks

[Daniel Berlin]

As an incremental solution, this looks like a good idea.

However, overall, this whole thing is, IMHO, the wrong solution.
Trying to compute a stateful problem with a stateless API is always
going to be slow :)
Caching does not really fix this.

The right way is to compute capturing as a stateful constraint/other problem,
once,  and if we discover we invalidate info too much, compute it again.

This is like calling it "lazy value info" and then computing it on
every variable, 5 times :)

At the point at which you compute it for everything, it's no longer
lazy, and it doesn't make a whole lot of sense to try to treat it as a
lazy problem.



On Tue, May 5, 2015 at 2:17 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Bruno Cardoso Lopes" <bruno.cardoso at gmail.com>
>> To: reviews+D7010+public+bde9f49f8dcc4a63 at reviews.llvm.org
>> Cc: nicholas at mxc.ca, "Hal Finkel" <hfinkel at anl.gov>, "Bob Wilson" <bob.wilson at apple.com>, "Rafael Ávila de Espíndola"
>> <rafael.espindola at gmail.com>, "Philip Reames" <listmail at philipreames.com>, "Commit Messages and Patches for LLVM"
>> <llvm-commits at cs.uiuc.edu>
>> Sent: Tuesday, May 5, 2015 1:57:10 PM
>> Subject: Re: [PATCH] [CaptureTracking] Avoid long compilation time on large basic blocks
>>
>> Hi,
>>
>> Sorry for the delay folks. Resuming this thread with more information
>> and patches...
>>
>> > I don't disagree that it might be difficult to re-use the cache
>> > inbetween calls to PointerMayBeCaptured (especially as the IR is
>> > being mutated -- we'd need to think about how this was laid out
>> > and what needed to actively invalidate it), but I thought the
>> > expensive part was really the repeated BB scans *within*
>> > PointerMayBeCaptured. That could be solved by having a local
>> > cache, right?
>>
>> Currently, PointerMayBeCaptured scans the BB the number of times
>> equal
>> to the number of uses of 'BeforeHere', which is currently capped at
>> 20
>> and bails out with Tracker->tooManyUses(). The main issue here is the
>> number of calls *to* PointerMayBeCaptured times the basic block scan.
>> In the testcase with 82k instructions, PointerMayBeCaptured is called
>> 130k times, leading to 'shouldExplore' taking 527k runs, this
>> currently takes ~12min.
>>
>> I tried the approach where I locally (within PointerMayBeCaptured )
>> number the instructions in the basic block using a DenseMap to cache
>> instruction positions/numbers. I experimented with two approaches:
>>
>> (1) Build the local cache once in the beginning and consult the cache
>> to gather position of instructions => Takes ~4min.
>> (2) Build the cache incrementally every time we need to scan an
>> unexplored part of the BB => Takes ~2min.
>>
>> I've attached the implementation for (2) in case there's any
>> interest.
>>
>> Considering the reduction from 12min to ~2min, this is a good gain,
>> but still looks to me like a long time to have a user waiting for the
>> compiler to finish, specially under -O1/-O2. I still believe the
>> limited scan approach is a better fix.
>
> Well, it's faster anyway, but also less precise.
>
>>
>> Let me know what you think.
>
> I think that this is really nice.
>
> +bool isPotentiallyReachableInner(SmallVectorImpl<BasicBlock *> &Worklist,
>
> I'd prefer to rename this function if it will now be externally accessible: 'Inner' is not particularly descriptive.
>
> +      // Start the search with the instruction found in the last lookup round.
>
> How do you know that subsequent calls to find specify instructions that always come later in the BB than those from previous calls?
>
> +    /// It also does not consider the scenario in 'isPotentiallyReachable'
> +    /// where 'A' comes after 'B' but may dominate through 'BB' successors.
>
> What does this mean exactly? Are you saying that the caller isPotentiallyReachable handles this case itself?
>
> +        if (isa<InvokeInst>(BeforeHere) || isa<PHINode>(I) || I == BeforeHere)
> +          return false;
>
> Why do we return false when I == BeforeHere?
>
> Thanks again,
> Hal
>
>>
>> Cheers,
>>
>>
>>
>> --
>> Bruno Cardoso Lopes
>> http://www.brunocardoso.cc
>>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory