[Bug c/105253] New: __popcountdi2 calls generated in kernel code with gcc12

[Bug c/105253] New: __popcountdi2 calls generated in kernel code with gcc12

[raj.khem at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

            Bug ID: 105253
           Summary: __popcountdi2 calls generated in kernel code with
                    gcc12
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: raj.khem at gmail dot com
  Target Milestone: ---

Created attachment 52794
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52794&action=edit
test case

gcc 12 when using -march=core2 generates __popcountdi2 calls which go
unresolved in kernel and build fails. When I use -march=corei7 then the calls
are optimized away.
Interestingly, gcc-11 did not emit these calls even with -march=core2,

a similar error is seen when compiling kernel for arm

arm-yoe-linux-gnueabi-ld.bfd: arch/arm/probes/kprobes/actions-common.o: in
function `simulate_ldm1stm1':
actions-common.c:(.kprobes.text+0x74): undefined reference to `__popcountsi2'

Attached testcase demonstrates the behaviour on x86_64. Is it expected behavior
in gcc 12 ?

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|x86_64                      |

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Two things. First the error message is for arm target but you then reference
x86_64 target options.

Second there is a loop for popcount which gets translated but that was guarded
by a check for optab for popcount existing but maybe that changed.

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://bugzilla.kernel.org
                   |                            |/show_bug.cgi?id=200671

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://bugzilla.kernel.org/show_bug.cgi?id=200671

Looks like a bug was filed in the kernel for this but no action.

Is this with -Os?

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[raj.khem at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

--- Comment #3 from Khem Raj <raj.khem at gmail dot com> ---
(In reply to Andrew Pinski from comment #1)
> Two things. First the error message is for arm target but you then reference
> x86_64 target options.
> 
> Second there is a loop for popcount which gets translated but that was
> guarded by a check for optab for popcount existing but maybe that changed.

yes the issue is seen on both arm and x86_64, So test case is from x86_64 but
the error message is from arm build. I can reduce that too tomorrow. Its with
-O2, I have a shell script in tarball which has all the options used to
compile.

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
With 12.0.1 20220213, I didn't get any popcountdi2 calls.

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
We're not checking for an optab here (that's on purpose IIRC).  The code using
the niter analysis result is responsible to do cost checking.  The builtin
is recognized via number_of_iterations_popcount.

If you use GCC you have to link against libgcc.a at least.

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Note, there is some code to not do this if we'd not expand the builtin inline
since r9-4030-g06a6b46a16f9287a98aa , so I wonder what changed.

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #6)
> Note, there is some code to not do this if we'd not expand the builtin inline
> since r9-4030-g06a6b46a16f9287a98aa , so I wonder what changed.

That only applies to final value replacement - the niter result might be used
by other passes.

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Reduced to just -O2:
int
foo (unsigned long long *p)
{
  int i, cnt = 0;
  unsigned long long elem;
  for (i = 0; i < (256 / 64); i++)
    {
      elem = p[i];
      for (; elem; cnt++)
        elem &= elem - 1;
    }
  return cnt;
}
Started with r12-8041-g973a2ce71f8dab559fbbfc34b59e39e047df74a6
Wonder how that affects the testcase...

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Ah, I see, expression_expensive_p is called, but number_of_iterations_popcount
when creating a CALL_EXPR only cares about TYPE_PRECISION and happily creates
__builtin_popcountl with unsigned long long argument because it has the same
precision.  tree_builtin_call_types_compatible_p says it is not valid builtin
call and so we don't consider it expensive.

Either we make number_of_iterations_popcount more accurate on the types, or
better in tree_builtin_call_types_compatible_p use something like:
  bool in_gimple_form = (cfun && (cfun->curr_properties & PROP_gimple)) != 0;
and if in_gimple_form, use useless_type_conversion_p instead of the
TYPE_MAIN_VARIANT / tree_nop_conversion_p checks.
That is my strong preference...

What do you think?

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
   Last reconfirmed|                            |2022-04-13
     Ever confirmed|0                           |1

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 52796
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52796&action=edit
gcc12-pr105253.patch

Now in (untested) patch form.
Note, the kernel still should provide __popcount[sd]i2, but in this case the
intent was that we don't emit calls and only hw instructions.

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
This is a regression (as in we don't want such a call in that case for
performance reasons and in the past we didn't emit it).

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0
           Priority|P3                          |P1

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #9)
> Ah, I see, expression_expensive_p is called, but
> number_of_iterations_popcount when creating a CALL_EXPR only cares about
> TYPE_PRECISION and happily creates
> __builtin_popcountl with unsigned long long argument because it has the same
> precision.  tree_builtin_call_types_compatible_p says it is not valid
> builtin call and so we don't consider it expensive.

Shouldn't it run into

      if (!is_inexpensive_builtin (get_callee_fndecl (expr)))
        return true;

then?  Or does that simply consider all popcount as inexpensive?
Yes it does.

I think we should change the above to

diff --git a/gcc/tree-scalar-evolution.cc b/gcc/tree-scalar-evolution.cc
index 44157265ce8..b53d7aaa71d 100644
--- a/gcc/tree-scalar-evolution.cc
+++ b/gcc/tree-scalar-evolution.cc
@@ -3420,12 +3420,15 @@ expression_expensive_p (tree expr, hash_map<tree,
uint64_t> &cache,
                  break;
              return true;
            }
+         break;
+
        default:
+         if (cfn == CFN_LAST
+             || !is_inexpensive_builtin (get_callee_fndecl (expr)))
+           return true;
          break;
        }

-      if (!is_inexpensive_builtin (get_callee_fndecl (expr)))
-       return true;
       FOR_EACH_CALL_EXPR_ARG (arg, iter, expr)
        if (expression_expensive_p (arg, cache, op_cost))
          return true;

> Either we make number_of_iterations_popcount more accurate on the types, or
> better in tree_builtin_call_types_compatible_p use something like:
>   bool in_gimple_form = (cfun && (cfun->curr_properties & PROP_gimple)) != 0;
> and if in_gimple_form, use useless_type_conversion_p instead of the
> TYPE_MAIN_VARIANT / tree_nop_conversion_p checks.
> That is my strong preference...
> 
> What do you think?

Hmm, but still tree_nop_conversion_p would work there, no?

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #10)
> Created attachment 52796 [details]
> gcc12-pr105253.patch
> 
> Now in (untested) patch form.
> Note, the kernel still should provide __popcount[sd]i2, but in this case the
> intent was that we don't emit calls and only hw instructions.

Meh.  Adding (useless) conversions in niter analysis to make the predicate
happy would be against what we do everywhere else where we simply rewrite
GIMPLE back to GENERIC without considering GENERICs stronger type system :/

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:29c46490de4616b911fccb34a9479f768fb51e94

commit r12-8141-g29c46490de4616b911fccb34a9479f768fb51e94
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Apr 13 15:44:51 2022 +0200

    tree.cc: Use useless_type_conversion_p in
tree_builtin_call_types_compatible_p while in gimple form [PR105253]

    tree_builtin_call_types_compatible_p uses TYPE_MAIN_VARIANT comparisons
    or tree_nop_conversion_p to ensure a builtin has correct GENERIC arguments.
    Unfortunately this regressed when get_call_combined_fn is called during
    GIMPLE optimizations.  E.g. when number_of_iterations_popcount is called,
    it doesn't ensure TYPE_MAIN_VARIABLE compatible argument type, it picks
    __builtin_popcount{,l,ll} based just on types' precision and doesn't
    fold_convert the arg to the right type.  We are in GIMPLE, such conversions
    are useless...
    So, either we'd need to fix number_of_iterations_popcount to add casts
    and inspect anything else that creates CALL_EXPRs late, or we can
    in tree_builtin_call_types_compatible_p just use the GIMPLE type
    comparisons (useless_type_conversion_p) when we are in GIMPLE form and
    the TYPE_MAIN_VARIANT comparison or tree_nop_conversion_p test otherwise.

    I think especially this late in stage4 the latter seems safer to me.

    2022-04-13  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/105253
            * tree.cc (tree_builtin_call_types_compatible_p): If PROP_gimple,
            use useless_type_conversion_p checks instead of TYPE_MAIN_VARIANT
            comparisons or tree_nop_conversion_p checks.

            * gcc.target/i386/pr105253.c: New test.

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.

[Bug middle-end/105253] __popcountdi2 calls generated in kernel code with gcc12

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105253

Hongtao.liu <crazylht at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |crazylht at gmail dot com

--- Comment #16 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Jakub Jelinek from comment #11)
> This is a regression (as in we don't want such a call in that case for
> performance reasons and in the past we didn't emit it).

Yes, just note it regresses 531.deepsjeng_r by 2% for -O2 -march=x86-64 since
below function.

typedef unsigned long long BITBOARD;
int PopCount (BITBOARD b) {
    int c = 0;

    while (b) {
        b &= b - 1;
        c++;
    }

    return c;
}

popcnt is supported by x86-64_v2.