Re: [PATCH] vect: Add a popcount fallback.

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

On Wed, Aug 9, 2023 at 12:23 PM Robin Dapp <rdapp.gcc@gmail.com> wrote:
>
> > We seem to be looking at promotions of the call argument, lhs_type
> > is the same as the type of the call LHS.  But the comment mentions .POPCOUNT
> > and the following code also handles others, so maybe handling should be
> > moved.  Also when we look to vectorize popcount (x) instead of popcount((T)x)
> > we can simply promote the result accordingly.
>
> IMHO lhs_type is the type of the conversion
>
>   lhs_oprnd = gimple_assign_lhs (last_stmt);
>   lhs_type = TREE_TYPE (lhs_oprnd);
>
> and rhs/unprom_diff has the type of the call's input argument
>
>   rhs_oprnd = gimple_call_arg (call_stmt, 0);
>   vect_look_through_possible_promotion (vinfo, rhs_oprnd, &unprom_diff);
>
> So we can potentially have
>   T0 arg
>   T1 in = (T1)arg
>   T2 ret = __builtin_popcount (in)
>   T3 lhs = (T3)ret
>
> and we're checking if precision (T0) == precision (T3).

Looks like so.  Note T1 == T2.  What we're really after is
changing T1/T2 and the actual popcount used closer to
T0/T3, like in case T0 was 'char' and T3 was 'long' we
could still use popcountqi and then widen to T3 (or the
other way around).  So yes, I think requiring that T0 and T3
are equal isn't necessary.

> This will never be true for a proper __builtin_popcountll except if
> the return value is cast to uint64_t (which I just happened to do
> in my test...).  Therefore it still doesn't really make sense to me.
>
> Interestingly though, it helps for an aarch64 __builtin_popcountll
> testcase where we abort here and then manage to vectorize via
> vectorizable_call.  When we skip this check, recognition succeeds
> and replaces the call with the pattern.  Then scalar costs are lower
> than in the vectorizable_call case because __builtin_popcountll is
> not STMT_VINFO_RELEVANT_P anymore (not live or so?).
> Then, vectorization costs are too high compared to the wrong scalar
> costs and we don't vectorize... Odd, might require fixing separately.
> We might need to calculate the scalar costs in advance?
>
> > It looks like vect_recog_popcount_clz_ctz_ffs_pattern is specifcally for
> > the conversions, so your fallback should possibly apply even when not
> > matching them.
>
> Mhm, yes it appears to only match when casting the return value to
> something else than an int.  So we'd need a fallback in vectorizable_call?
> And it would potentially look a bit out of place there only handling
> popcount and not ctz, clz, ...  Not sure if it is worth it then?

I'd keep the handling as pattern just also match on popcount directly
when not converted.

>
> Regards
>  Robin
>