[PATCH] D36059: [memops] Add a new pass to inject fast-path code for specific library function calls.

[Xinliang David Li via llvm-commits]

On Tue, Aug 1, 2017 at 12:53 AM, Chandler Carruth via Phabricator <
reviews at reviews.llvm.org> wrote:

> chandlerc added inline comments.
>
>
> ================
> Comment at: lib/Transforms/Scalar/FastPathLibCalls.cpp:90
> +  /// Checks whether a value is known non-zero at a particular location.
> +  bool isKnownNonZero(Value *V, Instruction &I) {
> +    // If value tracking knows enough, we're done.
> ----------------
> davidxl wrote:
> > chandlerc wrote:
> > > davidxl wrote:
> > > > chandlerc wrote:
> > > > > davidxl wrote:
> > > > > > Is it better to rely on instcombine and cfg simplification to
> get rid of the redundant zero guard which can be more general?
> > > > > Sadly, they're *less* powerful than this. You'd need PRE or
> something similar to get it, powered by PredicateInfo and GVN. Maybe
> JumpThreading and LVI could get it?
> > > > >
> > > > > All of these seem really heavy weight to run this late in the
> pipeline. =/
> > > > Cases like the following can be handled by -instcombine +
> -simplfycfg.   Wrapping the second redundant test into another check also
> works fine.
> > > >
> > > > What are the interesting cases that can not be handled?
> > > >
> > > > efine void @set1_nonzero1(i8* %ptr, i64 %size) {
> > > > ; CHECK-LABEL: define void @set1_nonzero1(
> > > > entry:
> > > >   %zero_cond = icmp eq i64 %size, 0
> > > >   br i1 %zero_cond, label %exit, label %test
> > > >
> > > > test:
> > > >   %nonzero_cond = icmp ne i64 %size, 0
> > > >   br i1 %nonzero_cond, label %call, label %exit
> > > >
> > > > call:
> > > >   call void @llvm.memset.p0i8.i64(i8* %ptr, i8 15, i64 %size, i32 1,
> i1 false)
> > > >   ret void
> > > >
> > > > exit:
> > > >   ret void
> > > > }
> > > >
> > > > declare void @llvm.memset.p0i8.i64(i8* writeonly, i8, i64, i32, i1)
> > > I'm honestly surprised instcombine+simplify-cfg would get this. It
> seems outside of their purview to do this kind of predicate analysis... I
> couldn't find where it does this in a cursory glance through the code.
> > >
> > > Anyways, we don't run instcombine after this pass, but only inst
> simplify. Not sure we want to add that expensive of a pass when this code
> can just handle the specific cases it wants.
> > instcombine just converts the second predicate into true/false.
> >
> > However, it seems simplifycfg alone can clean it up.    This pass can
> potentially be moved ahead to share some of the common cleanups?
> I'm not sure how it converts the second predicate into a constant in the
> hard cases though.
>
>
> Anyways, see my response to Craig regarding simplify-cfg -- it's doing a
> *much* more limited form of this. I can add a test case that shows this at
> least.
>

Fine.  Longer term, the code that does redundant predicate elimination here
should perhaps be extracted and put into some common passes. We should
probably also file bugs tracking such missing opportunities.

>
>
> As for moving this pass to share cleanups, see my comments to Mehdi about
> the challenges of changing the position of the pass. I don't see a great
> way to get it in front of instcombine, but maybe there is one.
>

Ok.   I am fine with the self contained approach for now. Perhaps
introducing a flag to make the behavior off by default for now so that
folks have more time to test it out?    Or if size is the only concern, we
can turn this on for O3.

David



>
> https://reviews.llvm.org/D36059
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170801/e17508f3/attachment.html>