Re: [RFC] Fix recent popcount change is breaking

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Jul 11, 2018 at 1:26 PM Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
>
> Hi Andrew,
>
> On 11 July 2018 at 15:43, Andrew Pinski <pinskia@gmail.com> wrote:
> > On Tue, Jul 10, 2018 at 6:35 PM Kugan Vivekanandarajah
> > <kugan.vivekanandarajah@linaro.org> wrote:
> >>
> >> Hi Andrew,
> >>
> >> On 11 July 2018 at 11:19, Andrew Pinski <pinskia@gmail.com> wrote:
> >> > On Tue, Jul 10, 2018 at 6:14 PM Kugan Vivekanandarajah
> >> > <kugan.vivekanandarajah@linaro.org> wrote:
> >> >>
> >> >> On 10 July 2018 at 23:17, Richard Biener <richard.guenther@gmail.com> wrote:
> >> >> > On Tue, Jul 10, 2018 at 3:06 PM Kugan Vivekanandarajah
> >> >> > <kugan.vivekanandarajah@linaro.org> wrote:
> >> >> >>
> >> >> >> Hi,
> >> >> >>
> >> >> >> Jeff told me that the recent popcount built-in detection is causing
> >> >> >> kernel build issues as
> >> >> >> ERROR: "__popcountsi2"
> >> >> >> [drivers/net/wireless/broadcom/brcm80211/brcmfmac/brcmfmac.ko] undefined!
> >> >> >>
> >> >> >> I could also reproduce this. AFIK, we should check if the libfunc is
> >> >> >> defined while checking popcount?
> >> >> >>
> >> >> >> I am testing the attached RFC patch. Is this reasonable?
> >> >> >
> >> >> > It doesn't work that way, all targets have this libfunc in libgcc.  This means
> >> >> > the kernel has to provide it.  The only thing you could do is restrict
> >> >> > replacement of CALL_EXPRs (in SCEV cprop) to those the target
> >> >> > natively supports.
> >> >>
> >> >> How about restricting it in expression_expensive_p ? Is that what you
> >> >> wanted. Attached patch does this.
> >> >> Bootstrap and regression testing progressing.
> >> >
> >> > Seems like that should go into is_inexpensive_builtin  instead which
> >> > is just tested right below.
> >>
> >> I hought about that. is_inexpensive_builtin is used in various other
> >> places including some inlining decision so wasn't sure if it is the
> >> right thing. Happy to change it if that is the right thing to do.
> >
> > I audited all of the users (and their users if it is used in a
> > wrapper) and found that is_inexpensive_builtin should return false for
> > this builtin if it is a function call in the end; there are other
> > builtins which should be checked the similar way but I think we should
> > not going to force you to do the similar thing for those builtins.
>
> Attached patch does this. Testing is progressing. Is This OK if no regression.

As said this isn't a complete fix given others may code-generate expressions
with niter, for example vectorization.

Also the table-based popcount implementation in libgcc is probably
faster and the popcount call at least smaller than an open-coded variant.

So I'm not sure if this is an appropriate fix.

Why not simply make popcountdi available in the kernel?  They do have
implementations for other libgcc functions IIRC.

Richard.

> Thanks,
> Kugan
>
>
> >
> > Thanks,
> > Andrew
> >
> >>
> >> Thanks,
> >> Kugan
> >> >
> >> > Thanks,
> >> > Andrew
> >> >
> >> >>
> >> >> Thanks,
> >> >> Kugan
> >> >>
> >> >> >
> >> >> > Richard.
> >> >> >
> >> >> >> Thanks,
> >> >> >> Kugan
> >> >> >>
> >> >> >> gcc/ChangeLog:
> >> >> >>
> >> >> >> 2018-07-10  Kugan Vivekanandarajah  <kuganv@linaro.org>
> >> >> >>
> >> >> >>     * tree-ssa-loop-niter.c (number_of_iterations_popcount): Check
> >> >> >>     if libfunc for popcount is available.