[PATCH] [SimplifyCFG] Swap to using TargetTransformInfo for cost analysis.

[James Molloy]

Hi,

So am I hearing correctly that this patch seems OK?

Cheers,

James

On Tue Feb 10 2015 at 5:21:15 PM Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
> > From: "Andrea Di Biagio" <andrea.dibiagio at gmail.com>
> > To: "Hal Finkel" <hfinkel at anl.gov>
> > Cc: "James Molloy" <james at jamesmolloy.co.uk>, reviews+D7506+public+
> d608f0951474be02 at reviews.llvm.org,
> > "llvm-commits at cs.uiuc.edu for LLVM" <llvm-commits at cs.uiuc.edu>
> > Sent: Tuesday, February 10, 2015 11:15:39 AM
> > Subject: Re: [PATCH] [SimplifyCFG] Swap to using TargetTransformInfo for
> cost analysis.
> >
> > On Tue, Feb 10, 2015 at 5:13 PM, Andrea Di Biagio
> > <andrea.dibiagio at gmail.com> wrote:
> > > On Tue, Feb 10, 2015 at 5:12 PM, Hal Finkel <hfinkel at anl.gov>
> > > wrote:
> > >> ----- Original Message -----
> > >>> From: "Andrea Di Biagio" <andrea.dibiagio at gmail.com>
> > >>> To: "Hal Finkel" <hfinkel at anl.gov>
> > >>> Cc: "James Molloy" <james at jamesmolloy.co.uk>,
> > >>> reviews+D7506+public+d608f0951474be02 at reviews.llvm.org,
> > >>> "llvm-commits at cs.uiuc.edu for LLVM" <llvm-commits at cs.uiuc.edu>
> > >>> Sent: Tuesday, February 10, 2015 10:59:15 AM
> > >>> Subject: Re: [PATCH] [SimplifyCFG] Swap to using
> > >>> TargetTransformInfo for cost analysis.
> > >>>
> > >>> Hi James, Hal
> > >>>
> > >>> thanks for the feedback.
> > >>>
> > >>> In that case, I'll see if I can come up with a fix to improve the
> > >>> cost
> > >>> of intrinsic calls for x86.
> > >>> On x86, the cost of cttz/ctlz would be either TCC_Basic or
> > >>> TCC_Expensive based on whether we have certain features.
> > >>>
> > >>> @Hal: Does it mean that this patch would make the the code I
> > >>> added in
> > >>> CodeGenPrepare to speculate calls to cttz/ctlz redundant? I see
> > >>> that
> > >>> with this patch, some of the test cases in
> > >>> CodeGen/X86/lzcnt-tzcnt.ll
> > >>> are now optimized by SimplifyCFG. The only unoptimized cases are
> > >>> those
> > >>> where the 'then' block contains extra instructions. I guess D7507
> > >>> will
> > >>> take care of it.
> > >>>
> > >>> Please correct me if I am wrong.
> > >>
> > >> I think you're right; it should get the simple speculation case.
> > >> Will it also get the cases where we clear the 'undef on zero'
> > >> flag?
> > >>
> > >>  -Hal
> > >
> > > Yes, it would work because of this revision:
> >
> > Forget my last email...
> >
> > It will get the 'undef on zero' flag because of revision 227197.
> > http://llvm.org/viewvc/llvm-project?view=revision&revision=227197
>
> Ah, okay, Now I understand. Thanks!
>
> So, yes, it might make some of that logic redundant. However, we can also
> reuse the TLI interfaces by making TTI's default implementation use those
> interfaces for providing user costs for those intrinsics.
>
>  -Hal
>
> >
> > >
> > >>
> > >>>
> > >>> Thanks!
> > >>> Andrea
> > >>>
> > >>> On Tue, Feb 10, 2015 at 4:27 PM, Hal Finkel <hfinkel at anl.gov>
> > >>> wrote:
> > >>> > ----- Original Message -----
> > >>> >> From: "James Molloy" <james at jamesmolloy.co.uk>
> > >>> >> To: reviews+D7506+public+d608f0951474be02 at reviews.llvm.org,
> > >>> >> "james
> > >>> >> molloy" <james.molloy at arm.com>, hfinkel at anl.gov,
> > >>> >> "t p northover" <t.p.northover at gmail.com>, "Andrea DiBiagio"
> > >>> >> <Andrea_DiBiagio at sn.scee.net>
> > >>> >> Cc: llvm-commits at cs.uiuc.edu
> > >>> >> Sent: Tuesday, February 10, 2015 10:22:07 AM
> > >>> >> Subject: Re: [PATCH] [SimplifyCFG] Swap to using
> > >>> >> TargetTransformInfo for cost analysis.
> > >>> >>
> > >>> >>
> > >>> >> Hi Andrea,
> > >>> >>
> > >>> >> Performance on AArch64 showed no major swings. However, it
> > >>> >> sounds
> > >>> >> to
> > >>> >> me like your TargetTransformInfo isn't behaving correctly.
> > >>> >> That's
> > >>> >> TTI's entire point, and the x86 one should surely return
> > >>> >> "TCC_Expensive" for such intrinsic calls.
> > >>> >>
> > >>> >> I see that the default TTI does return TCC_Basic for almost
> > >>> >> all
> > >>> >> intrinsics, and that is arguable behaviour. But there's no
> > >>> >> reason
> > >>> >> you shouldn't override this. In fact, it seems the right thing
> > >>> >> to
> > >>> >> do.
> > >>> >>
> > >>> >
> > >>> > I agree, x86 should override these. I'll add that we're going
> > >>> > to
> > >>> > run into this more because we're starting to use the user-cost
> > >>> > interface for more things. The target's user-cost
> > >>> > implementations
> > >>> > are not as well tuned as the cost interfaces used by the
> > >>> > vectorizer.
> > >>> >
> > >>> >  -Hal
> > >>> >
> > >>> >>
> > >>> >> Cheers,
> > >>> >>
> > >>> >>
> > >>> >> James
> > >>> >>
> > >>> >> On Tue Feb 10 2015 at 2:53:52 PM Andrea Di Biagio <
> > >>> >> Andrea_DiBiagio at sn.scee.net > wrote:
> > >>> >>
> > >>> >>
> > >>> >> REPOSITORY
> > >>> >> rL LLVM
> > >>> >>
> > >>> >> http://reviews.llvm.org/D7506
> > >>> >>
> > >>> >> 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
> > >>> >>
> > >>> >
> > >>> > --
> > >>> > Hal Finkel
> > >>> > Assistant Computational Scientist
> > >>> > Leadership Computing Facility
> > >>> > Argonne National Laboratory
> > >>> > _______________________________________________
> > >>> > 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/20150210/68f63267/attachment.html>