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

[Richard Biener <rguenther@suse.de>]

On Fri, 10 Nov 2023, Jakub Jelinek wrote:

> 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.

googling doesn't find me stdc_leading_zeros - are those supposed to work
for non-_BitInt types as well and don't promote the argument in that
case?

If we are spcificially targeting those I wonder why we don't name
the builtins after those?  But yes, if promotion is undesirable
for implementing them then I agree.  IIRC _BitInt(n) is not subject
to integer promotions.

> 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
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)