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

[Hal Finkel]

----- Original Message -----

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

> > ----- Original Message -----
> 

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

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/c67ad78c/attachment.html>