Re: [PATCH] Add type-generic clz/ctz/clrsb/ffs/parity/popcount builtins [PR111309]

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Nov 10, 2023 at 08:09:26AM +0000, Richard Biener wrote:
> > The following patch adds 6 new type-generic builtins,
> > __builtin_clzg
> > __builtin_ctzg
> > __builtin_clrsbg
> > __builtin_ffsg
> > __builtin_parityg
> > __builtin_popcountg
> > The g at the end stands for generic because the unsuffixed variant
> > of the builtins already have unsigned int or int arguments.
> > 
> > The main reason to add these is to support arbitrary unsigned (for
> > clrsb/ffs signed) bit-precise integer types and also __int128 which
> > wasn't supported by the existing builtins, so that e.g. <stdbit.h>
> > type-generic functions could then support not just bit-precise unsigned
> > integer type whose width matches a standard or extended integer type,
> > but others too.
> > 
> > None of these new builtins promote their first argument, so the argument
> > can be e.g. unsigned char or unsigned short or unsigned __int20 etc.
> 
> But is that a good idea?  Is that how type generic functions work in C?

Most current type generic functions deal just with floating point args and
don't promote there float to double as the ... promotions would normally do:
__builtin_signbit
__builtin_fpclassify
__builtin_isfinite
__builtin_isinf_sign
__builtin_isinf
__builtin_isnan
__builtin_isnormal
__builtin_isgreater
__builtin_isgreaterequal
__builtin_isless
__builtin_islessequal
__builtin_islessgreater
__builtin_isunordered
__builtin_iseqsig
__builtin_issignaling

__builtin_clear_padding is uninteresting, because the argument must be a
pointer.

Then
__builtin_add_overflow
__builtin_sub_overflow
__builtin_mul_overflow
do promote the first 2 arguments, but that doesn't really matter, because
all we care about is the argument values, not their type.

And finally
__builtin_add_overflow_p
__builtin_sub_overflow_p
__builtin_mul_overflow_p
do promote the first 2 arguments, and don't promote the third one, which is
the only one where we care about the type, so that is the behavior that
I've used also for the new builtins.  I think if we added e.g.
__builtin_classify_type now and not more than 3 decades ago it would behave
like that too.
Only not promoting the argument will make it directly usable in the
stdc_leading_zeros, stdc_leading_ones, stdc_trailing_zeros, stdc_trailing_ones,
stdc_first_leading_zero, ..., stdc_count_zeros, stdc_count_ones, ...
C23 stdbit.h type-generic macros, otherwise one would need to play with
_Generic and special-case there unsigned char and unsigned short (which
normally promote to int), but e.g. unsigned _BitInt(8) doesn't.
I expect Joseph will have compatibility version of the macro for when these
builtins aren't supported, but given the standard says that {un,}signed _BitInt
with width matching standard/extended integer types other than bool needs to
be handled also, either it will not use _Generic at all and just go after
sizeof (argument), or maybe will use _Generic and for the default case will
go after sizeof.  Seems clang returns -1 for __builtin_classify_type (0uwb)
rather than 18 GCC returns, so one can't portably distinguish bitints.

> I think it introduces non-obvious/unexpected behavior in user code.

If we keep the patch behavior of requiring unsigned
standard/extended/bit-precise types other than bool for the
clz/ctz/parity/popcount cases, the choice is between always erroring on
__builtin_clzg ((unsigned char) 1) - where the promoted argument is signed,
and accepting it as unsigned char case without promotion, so I think users
would be more confused to see an error on that.
If we'd switch to accepting both signed and unsigned
standard/extended/bit-precise integer types other than bool for all the
builtins, whether we promote or not doesn't matter for ctz/parity/popcount
but does for clz.
The clrsb/ffs cases accept signed argument on the other side, so both
promoted and unpromoted argument would mean something and be accepted,
in the ffs case it again doesn't really matter for the result, but for clrsb
is significant.
Would it help to just document it that the argument isn't promoted?

We document that for __builtin_*_overflow_p:
"The value of the third argument is ignored, just the side effects in the third argument
are evaluated, and no integral argument promotions are performed on the last
argument."

> If people do not want to "compensate" for this maybe insted also add
> __builtin_*{8,16} (like we have for the bswap variants)?

Note, clang has __builtin_clzs and __builtin_ctzs for unsigned short (but
not the other 4), but nothing for the unsigned char cases.
I was just hoping we don't need to add further variants if we have
type-generic ones.

> Otherwise this looks reasonable.  I'm not sure why we need separate
> CFN_CLZ and CFN_BUILT_IN_CLZG?  (why CFN_BUILT_IN_CLZG and not CFN_CLZG?)
> That is, I'm confused about
> 
>      CASE_CFN_CLRSB:
> +    case CFN_BUILT_IN_CLRSBG:
> 
> why does CASE_CFN_CLRSB not include CLRSBG?  It includes IFN_CLRSB, no?
> And IFN_CLRSB already has the two and one arg case and thus encompasses
> some BUILT_IN_CLRSBG cases?

gencfn-macros.cc is aware of just normal float suffixes (F, nothing, L;
then under different names of the macros other variants for float
suffixes), and int suffixes (nothing, L, LL, IMAX), it doesn't know anything
about the G suffix.  We could teach it to under a different suffix add the
G case too, but I didn't think it was necessary because the *G builtins are
meant to be folded away into something else as soon as possible, worst case
during gimplification, so nothing after that ought to care about them.
It is just the fold-const-call.cc case where in constant expressions I think
we want to fold them into constants and having new macros just to use them
once (and don't want to use them in the 2-6 other places depending on the
builtin) seemed unnecessary.

> Besides the above question I'd say OK (I assume Josephs reply is a
> general ack from his side).

Joseph, what are your thoughts on the above?

Incremental patch to document the lack of integral argument promotion:

--- gcc/doc/extend.texi.jj	2023-11-09 09:17:40.240182342 +0100
+++ gcc/doc/extend.texi	2023-11-10 09:57:45.396215654 +0100
@@ -14962,13 +14962,15 @@ Similar to @code{__builtin_parity}, exce
 
 @defbuiltin{int __builtin_ffsg (...)}
 Similar to @code{__builtin_ffs}, except the argument is type-generic
-signed integer (standard, extended or bit-precise).
+signed integer (standard, extended or bit-precise).  No integral argument
+promotions are performed on the argument.  
 @enddefbuiltin
 
 @defbuiltin{int __builtin_clzg (...)}
 Similar to @code{__builtin_clz}, except the argument is type-generic
 unsigned integer (standard, extended or bit-precise) and there is
-optional second argument with int type.  If two arguments are specified,
+optional second argument with int type.  No integral argument promotions
+are performed on the first argument.  If two arguments are specified,
 and first argument is 0, the result is the second argument.  If only
 one argument is specified and it is 0, the result is undefined.
 @enddefbuiltin
@@ -14976,24 +14978,28 @@ one argument is specified and it is 0, t
 @defbuiltin{int __builtin_ctzg (...)}
 Similar to @code{__builtin_ctz}, except the argument is type-generic
 unsigned integer (standard, extended or bit-precise) and there is
-optional second argument with int type.  If two arguments are specified,
+optional second argument with int type.  No integral argument promotions
+are performed on the first argument.  If two arguments are specified,
 and first argument is 0, the result is the second argument.  If only
 one argument is specified and it is 0, the result is undefined.
 @enddefbuiltin
 
 @defbuiltin{int __builtin_clrsbg (...)}
 Similar to @code{__builtin_clrsb}, except the argument is type-generic
-signed integer (standard, extended or bit-precise).
+signed integer (standard, extended or bit-precise).  No integral argument
+promotions are performed on the argument.  
 @enddefbuiltin
 
 @defbuiltin{int __builtin_popcountg (...)}
 Similar to @code{__builtin_popcount}, except the argument is type-generic
-unsigned integer (standard, extended or bit-precise).
+unsigned integer (standard, extended or bit-precise).  No integral argument
+promotions are performed on the argument.  
 @enddefbuiltin
 
 @defbuiltin{int __builtin_parityg (...)}
 Similar to @code{__builtin_parity}, except the argument is type-generic
-unsigned integer (standard, extended or bit-precise).
+unsigned integer (standard, extended or bit-precise).  No integral argument
+promotions are performed on the argument.  
 @enddefbuiltin
 
 @defbuiltin{double __builtin_powi (double, int)}


	Jakub