[PATCH] Fix a performance problem in gep(gep ...) merging

[Xinliang David Li]

On Thu, Apr 16, 2015 at 8:11 AM, Hal Finkel <hfinkel at anl.gov> wrote:

>
>
> ------------------------------
>
> *From: *"Xinliang David Li" <xinliangli at gmail.com>
> *To: *"Hal Finkel" <hfinkel at anl.gov>
> *Cc: *"Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>,
> "reviews+d8911+public+8f2845face98911b" <
> reviews+D8911+public+8f2845face98911b at reviews.llvm.org>, "Nick Lewycky" <
> nicholas at mxc.ca>
> *Sent: *Wednesday, April 15, 2015 11:36:58 PM
> *Subject: *Re: [PATCH] Fix a performance problem in gep(gep ...) merging
>
>
>
> On Tue, Apr 14, 2015 at 10:58 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>
>> ------------------------------
>>
>> > From: "Xinliang David Li" <xinliangli at gmail.com>
>> > To: "Nick Lewycky" <nicholas at mxc.ca>
>> > Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>,
>> > reviews+D8911+public+8f2845face98911b at reviews.llvm.org
>> > Sent: Tuesday, April 14, 2015 10:23:36 AM
>> > Subject: Re: [PATCH] Fix a performance problem in gep(gep ...) merging
>> >
>> >
>> >
>> > IIUC, GEP merging can be harmful regardless of where the target is in
>> > loop context or not. Wei's heuristic filters out lots of harmful
>> > cases. A more general fix might need introduce more sophisticated
>> > cost analysis -- it is possible (as mentioned by Quentin) for this
>> > simple heuristic to filter out beneficial merging, but test cases
>> > are needed for that.
>>
>> One thing we need to keep in mind here is that the reduction in GEP depth
>> directly affects the precision of BasicAA (and other code that calls
>> GetUnderlyingObject/GetUnderlyingObjects. These things have small recursion
>> cutoffs (6 currently). computeKnownBits, which looks through GEPs to find
>> pointer alignments is another example. The aggressive merging strategy
>> supports these small cutoffs; if we reduce the amount of merging that we
>> do, we may need to re-tune these cutoffs.
>>
>
> In practice, how often do we see long chains of GEPs? 6 seems reasonably
> long. The alias analysis does not really depend on the merged form of GEP
> to be more precise (other than the arbitrary limitation imposed on long GEP
> chains).
>
>
> I suspect not often, but I don't really know. It might be a good idea to
> add a statistic to count how often we hit the limit. To be clear, I'm not
> against this change, but I wanted people to keep these recursion depth
> limits in mind as it might be relevant to adjusting for any regressions
> should they emerge.
>

Agreed. Also useful to do this with GEP merge disabled.

thanks,

David


> Interesting example.
>
> Thanks again,
> Hal
>
>
>
> In fact, there are cases where merged GEP actually confuses aliaser.
> Consider the following case:
>
> extern int aa[];
>
> int foo(int k, int n
> #ifdef NOMERGE
>         , int *bb
> #endif
>         )
> {
>    int i, s;
>    s = 0;
> #ifndef NOMERGE
>    int *bb = &aa[k];
> #endif
>    s = bb[n];
>    bb[n+1] = 20;
>    s+=bb[n];
>
>    return s;
> }
>
> When built with -DNOMERGE or without but with instcombine disabled, GVN is
> able to remove the second load. However, with instcombine turned on and GEP
> merged, the load remains after the store.
>
> David
>
>
>
>
>
>>
>>  -Hal
>>
>> >
>> >
>> > David
>> >
>> >
>> > On Tue, Apr 14, 2015 at 1:16 AM, Nick Lewycky < nicholas at mxc.ca >
>> > wrote:
>> >
>> >
>> > Quentin Colombet wrote:
>> >
>> >
>> > The thing with the "both constants" approach is that we may miss some
>> > cases where an addressing mode may apply. In particular, on x86,
>> > when one argument of the add is a constant, we would be able to
>> > match reg1 + reg2 + displacement.
>> >
>> > That being said, your numbers imply this is not a problem in
>> > practice, and most targets do not support reg1 + reg2 + reg3/imm
>> > addressing mode. Anyway, we could recover from that later on if
>> > needed.
>> >
>> > So LGTM.
>> >
>> > I might be misunderstanding why this patch helps, please bear with
>> > me. Is this not a special case of a general problem, mainly that
>> > we're merging something into a loop that was previously two
>> > operations one inside and one outside the loop? Why wouldn't this
>> > problem come up in other ways? We already have other problems where
>> > we introduce loop carried dependencies where ones previously didn't
>> > exist, shouldn't we approach this by adding an API to detect those
>> > and then making transforms conditional on that?
>> >
>> > Nick
>> >
>> >
>> >
>> >
>> >
>> >
>> > Thanks Wei!
>> > -Quentin
>> >
>> >
>> > REPOSITORY
>> > rL LLVM
>> >
>> > http://reviews.llvm.org/D8911
>> >
>> > EMAIL PREFERENCES
>> > http://reviews.llvm.org/ settings/panel/ emailpreferences/
>> >
>> >
>> >
>> >
>> > ______________________________ _________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/ mailman/listinfo/llvm-commits
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>>
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>
>
>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150416/cccd0b78/attachment.html>