Re: [PATCH] tree-ssa-math-opts: popcount (X) == 1 to (X ^ (X - 1)) > (X - 1) optimization for direct optab [PR90693]

[Jeff Law <jeffreyalaw@gmail.com>]

On 11/18/23 03:27, Jakub Jelinek wrote:
> Hi!
> 
> On Fri, Nov 17, 2023 at 03:01:04PM +0100, Jakub Jelinek wrote:
>> As a follow-up, I'm considering changing in this routine the popcount
>> call to IFN_POPCOUNT with 2 arguments and during expansion test costs.
> 
> Here is the follow-up which does the rtx costs testing.
> While having to tweak internal-fn.def so that POPCOUNT can have a custom
> expand_POPCOUNT, I have noticed we are inconsistent, some DEF_INTERNAL*
> macros (most of them) were undefined at the end of internal-fn.def (but in
> some cases uselessly undefined again after inclusion), while others were not
> (and sometimes undefined after the inclusion).  I've changed it to always
> undefine at the end of internal-fn.def.
> 
> Ok for trunk if it passes bootstrap/regtest?
> 
> 2023-11-18  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/90693
> 	* tree-ssa-math-opts.cc (match_single_bit_test): Mark POPCOUNT with
> 	result only used in equality comparison against 1 with direct optab
> 	support as .POPCOUNT call with 2 arguments.
> 	* internal-fn.h (expand_POPCOUNT): Declare.
> 	* internal-fn.def: Document missing DEF_INTERNAL* macros and make sure
> 	they are all undefined at the end.
> 	(DEF_INTERNAL_INT_EXT_FN): New macro.
> 	(POPCOUNT): Use it instead of DEF_INTERNAL_INT_FN.
> 	* internal-fn.cc (lookup_hilo_internal_fn, lookup_evenodd_internal_fn,
> 	widening_fn_p, get_len_internal_fn): Don't undef DEF_INTERNAL_*FN
> 	macros after inclusion of internal-fn.def.
> 	(DEF_INTERNAL_INT_EXT_FN): Define to nothing before inclusion to
> 	define expanders.
> 	(expand_POPCOUNT): New function.
> 

> +  unsigned cmp_cost = seq_cost (cmp_insns, speed_p);
> +  if (popcount_cost < cmp_cost)
> +    emit_insn (popcount_insns);
> +  else
> +    {
> +      emit_insn (cmp_insns);
> +      plhs = expand_normal (lhs);
> +      if (GET_MODE (cmp) != GET_MODE (plhs))
> +	cmp = convert_to_mode (GET_MODE (plhs), cmp, 1);
> +      emit_move_insn (plhs, cmp);
> +    }
Did you want <= for the test to use popcount?  That seems like a better 
choice in that scenario to me as the popcount is likely smaller.

OK for the trunk as-is or using a <= test.

jeff