[RFC] Fix recent popcount change is breaking

[RFC] Fix recent popcount change is breaking

[Kugan Vivekanandarajah]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

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?

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.

[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 988 bytes --]

diff --git a/gcc/tree-ssa-loop-niter.c b/gcc/tree-ssa-loop-niter.c
index f6fa2f7..2e2b9c6 100644
--- a/gcc/tree-ssa-loop-niter.c
+++ b/gcc/tree-ssa-loop-niter.c
@@ -21,6 +21,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "system.h"
 #include "coretypes.h"
 #include "backend.h"
+#include "target.h"
 #include "rtl.h"
 #include "tree.h"
 #include "gimple.h"
@@ -42,6 +43,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-chrec.h"
 #include "tree-scalar-evolution.h"
 #include "params.h"
+#include "optabs-libfuncs.h"
 #include "tree-dfa.h"
 
 
@@ -2570,6 +2572,10 @@ number_of_iterations_popcount (loop_p loop, edge exit,
 	   (long_long_integer_type_node))
     fn = builtin_decl_implicit (BUILT_IN_POPCOUNTLL);
 
+  /* Check if libfunc for popcount is available.  */
+  if (!optab_libfunc (popcount_optab,
+		      TYPE_MODE (TREE_TYPE (src))))
+    return false;
   /* ??? Support promoting char/short to int.  */
   if (!fn)
     return false;

Re: [RFC] Fix recent popcount change is breaking

[Richard Biener]

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.

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.

Re: [RFC] Fix recent popcount change is breaking

[Jakub Jelinek]

On Tue, Jul 10, 2018 at 03:17:48PM +0200, Richard Biener wrote:
> On Tue, Jul 10, 2018 at 3:06 PM Kugan Vivekanandarajah
> > 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.

Yeah, that is what we've done in the past in other cases, e.g. PR82981
is somewhat similar.  So perhaps just check the optab and use it only if it
is supported?

	Jakub

Re: [RFC] Fix recent popcount change is breaking

[Richard Biener]

On Tue, Jul 10, 2018 at 3:27 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Jul 10, 2018 at 03:17:48PM +0200, Richard Biener wrote:
> > On Tue, Jul 10, 2018 at 3:06 PM Kugan Vivekanandarajah
> > > 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.
>
> Yeah, that is what we've done in the past in other cases, e.g. PR82981
> is somewhat similar.  So perhaps just check the optab and use it only if it
> is supported?

Btw, given that we do not want to fail niter analysis we'd have to audit
all places that eventually code-generate it which isn't only SCEV-cprop ...

So not sure if it isn't better to user-control this in a way not depending
on target HW support...

Richard.

>
>         Jakub

Re: [RFC] Fix recent popcount change is breaking

[Jeff Law]

On 07/10/2018 07:17 AM, Richard Biener 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.
I can certainly live with that, but I think we should reach out to the
kernel developers to proactively make them aware of the requirement to
provide popcount.

Jeff

Re: [RFC] Fix recent popcount change is breaking

[Jeff Law]

On 07/10/2018 07:27 AM, Jakub Jelinek wrote:
> On Tue, Jul 10, 2018 at 03:17:48PM +0200, Richard Biener wrote:
>> On Tue, Jul 10, 2018 at 3:06 PM Kugan Vivekanandarajah
>>> 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.
> 
> Yeah, that is what we've done in the past in other cases, e.g. PR82981
> is somewhat similar.  So perhaps just check the optab and use it only if it
> is supported?
And I could live with this too.  Essentially I'm just looking to get the
issue raised and addressed now rather than waiting for stage3/stage4 :-)


jeff

Re: [RFC] Fix recent popcount change is breaking

[Kugan Vivekanandarajah]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

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.

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.

[-- Attachment #2: p1.txt --]
[-- Type: text/plain, Size: 1965 bytes --]

diff --git a/gcc/testsuite/gcc.target/aarch64/popcount4.c b/gcc/testsuite/gcc.target/aarch64/popcount4.c
index e69de29..ee55b2e 100644
--- a/gcc/testsuite/gcc.target/aarch64/popcount4.c
+++ b/gcc/testsuite/gcc.target/aarch64/popcount4.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -mgeneral-regs-only" } */
+
+int PopCount (long b) {
+    int c = 0;
+
+    while (b) {
+	b &= b - 1;
+	c++;
+    }
+    return c;
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin_popcount" 0 "optimized" } } */
diff --git a/gcc/tree-scalar-evolution.c b/gcc/tree-scalar-evolution.c
index 69122f2..ec8e4ec 100644
--- a/gcc/tree-scalar-evolution.c
+++ b/gcc/tree-scalar-evolution.c
@@ -257,7 +257,9 @@ along with GCC; see the file COPYING3.  If not see
 #include "system.h"
 #include "coretypes.h"
 #include "backend.h"
+#include "target.h"
 #include "rtl.h"
+#include "optabs-query.h"
 #include "tree.h"
 #include "gimple.h"
 #include "ssa.h"
@@ -282,6 +284,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-fold.h"
 #include "tree-into-ssa.h"
 #include "builtins.h"
+#include "case-cfn-macros.h"
 
 static tree analyze_scalar_evolution_1 (struct loop *, tree);
 static tree analyze_scalar_evolution_for_address_of (struct loop *loop,
@@ -3500,6 +3503,23 @@ expression_expensive_p (tree expr)
     {
       tree arg;
       call_expr_arg_iterator iter;
+      tree fndecl = get_callee_fndecl (expr);
+
+      if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
+	{
+	  combined_fn cfn = as_combined_fn (DECL_FUNCTION_CODE (fndecl));
+	  switch (cfn)
+	    {
+	    CASE_CFN_POPCOUNT:
+	      /* Check if opcode for popcount is available.  */
+	      if (optab_handler (popcount_optab,
+				 TYPE_MODE (TREE_TYPE (CALL_EXPR_ARG (expr, 0))))
+		  == CODE_FOR_nothing)
+		return true;
+	    default:
+	      break;
+	    }
+	}
 
       if (!is_inexpensive_builtin (get_callee_fndecl (expr)))
 	return true;

Re: [RFC] Fix recent popcount change is breaking

[Andrew Pinski]

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.

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.

Re: [RFC] Fix recent popcount change is breaking

[Kugan Vivekanandarajah]

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.

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.

Re: [RFC] Fix recent popcount change is breaking

[Andrew Pinski]

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.

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.

Re: [RFC] Fix recent popcount change is breaking

[Kugan Vivekanandarajah]

[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]

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.

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.

[-- Attachment #2: p2.txt --]
[-- Type: text/plain, Size: 1641 bytes --]

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 820d6c2..59cf567 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -10619,6 +10619,18 @@ is_inexpensive_builtin (tree decl)
   else if (DECL_BUILT_IN_CLASS (decl) == BUILT_IN_NORMAL)
     switch (DECL_FUNCTION_CODE (decl))
       {
+      case BUILT_IN_POPCOUNT:
+      case BUILT_IN_POPCOUNTL:
+      case BUILT_IN_POPCOUNTLL:
+	  {
+	    tree arg = TYPE_ARG_TYPES (TREE_TYPE (decl));
+	    /* Check if opcode for popcount is available.  */
+	    if (optab_handler (popcount_optab,
+			       TYPE_MODE (TREE_VALUE (arg)))
+		== CODE_FOR_nothing)
+	      return false;
+	  }
+	return true;
       case BUILT_IN_ABS:
       CASE_BUILT_IN_ALLOCA:
       case BUILT_IN_BSWAP16:
@@ -10670,10 +10682,7 @@ is_inexpensive_builtin (tree decl)
       case BUILT_IN_VA_COPY:
       case BUILT_IN_TRAP:
       case BUILT_IN_SAVEREGS:
-      case BUILT_IN_POPCOUNTL:
-      case BUILT_IN_POPCOUNTLL:
       case BUILT_IN_POPCOUNTIMAX:
-      case BUILT_IN_POPCOUNT:
       case BUILT_IN_PARITYL:
       case BUILT_IN_PARITYLL:
       case BUILT_IN_PARITYIMAX:
diff --git a/gcc/testsuite/gcc.target/aarch64/popcount4.c b/gcc/testsuite/gcc.target/aarch64/popcount4.c
index e69de29..ee55b2e 100644
--- a/gcc/testsuite/gcc.target/aarch64/popcount4.c
+++ b/gcc/testsuite/gcc.target/aarch64/popcount4.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -mgeneral-regs-only" } */
+
+int PopCount (long b) {
+    int c = 0;
+
+    while (b) {
+	b &= b - 1;
+	c++;
+    }
+    return c;
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin_popcount" 0 "optimized" } } */

Re: [RFC] Fix recent popcount change is breaking

[Richard Biener]

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.

Re: [RFC] Fix recent popcount change is breaking

[Martin Liška]

On 07/11/2018 02:31 PM, Richard Biener wrote:
> Why not simply make popcountdi available in the kernel?  They do have
> implementations for other libgcc functions IIRC.

Can you please Kugan create Linux kernel bug for that? So that discussion
can happen?

Thanks,
Martin

Re: [RFC] Fix recent popcount change is breaking

[Richard Biener]

On July 27, 2018 3:33:59 PM GMT+02:00, "Martin Liška" <mliska@suse.cz> wrote:
>On 07/11/2018 02:31 PM, Richard Biener wrote:
>> Why not simply make popcountdi available in the kernel?  They do have
>> implementations for other libgcc functions IIRC.
>
>Can you please Kugan create Linux kernel bug for that? So that
>discussion
>can happen?

There's no discussion necessary, libgcc is the core compiler runtime. If you choose not to use it you have to provide your own implementation. 

Richard. 

>Thanks,
>Martin

Re: [RFC] Fix recent popcount change is breaking

[Kugan Vivekanandarajah]

Hi,

On 28 July 2018 at 01:13, Richard Biener <richard.guenther@gmail.com> wrote:
> On July 27, 2018 3:33:59 PM GMT+02:00, "Martin Liška" <mliska@suse.cz> wrote:
>>On 07/11/2018 02:31 PM, Richard Biener wrote:
>>> Why not simply make popcountdi available in the kernel?  They do have
>>> implementations for other libgcc functions IIRC.
>>
>>Can you please Kugan create Linux kernel bug for that? So that
>>discussion
>>can happen?
>
> There's no discussion necessary, libgcc is the core compiler runtime. If you choose not to use it you have to provide your own implementation.

Created a bug against kernel at
https://bugzilla.kernel.org/show_bug.cgi?id=200671

Thanks,
Kugan
>
> Richard.
>
>>Thanks,
>>Martin
>

Re: [RFC] Fix recent popcount change is breaking

[Bin.Cheng]

On Sat, Jul 28, 2018 at 7:36 AM Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
>
> Hi,
>
> On 28 July 2018 at 01:13, Richard Biener <richard.guenther@gmail.com> wrote:
> > On July 27, 2018 3:33:59 PM GMT+02:00, "Martin Liška" <mliska@suse.cz> wrote:
> >>On 07/11/2018 02:31 PM, Richard Biener wrote:
> >>> Why not simply make popcountdi available in the kernel?  They do have
> >>> implementations for other libgcc functions IIRC.
> >>
> >>Can you please Kugan create Linux kernel bug for that? So that
> >>discussion
> >>can happen?
> >
> > There's no discussion necessary, libgcc is the core compiler runtime. If you choose not to use it you have to provide your own implementation.
>
> Created a bug against kernel at
> https://bugzilla.kernel.org/show_bug.cgi?id=200671
Looks like it's concerned whether GCC would generate more inefficient
code now.  If that's true, it might be better to disable this somehow
for some cases?  otherwise, could someone reason about it in that
kernel bug in order to push forward a fix in kernel please?

Thanks,
bin
>
> Thanks,
> Kugan
> >
> > Richard.
> >
> >>Thanks,
> >>Martin
> >