From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id DE6203858D1E for ; Fri, 10 Nov 2023 09:19:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DE6203858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org DE6203858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:67c:2178:6::1d ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699607958; cv=none; b=mSrWB639CeM7a8MXRiGXLArgEa9UZYuCp+NOJ3siVCcDYR67i7L/efwo3146LktmmyAz4SEoL8bNn6034/zyjq5XZ24wdyF6qVnT1SjmxcpDTii31mlN92SxSg99LXRNLTq4o7uk8kg10YYNZRjNVxf9dCDlP6WpWPh0iOaksTA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699607958; c=relaxed/simple; bh=p1dJKRW4Z5ek8PLTYhkSSie2PgN8usgjfJbPbMcAL4s=; h=DKIM-Signature:DKIM-Signature:Date:From:To:Subject:Message-ID: MIME-Version; b=Vh2pAh3D6cks9W9PmbhI2B45rqjEv8BgYE3bbdYTzmKIPzBNkwrUyTGnIkFXg5Un8ca57uVWWQx/Xtr3+Lyb5HMRk2ye9rHvQ5HmleUjQ4T+gjrshx8hRu3a0Obx0DIkf/GcDU4P4HE2z0XDfV3Tt1m24wuM60wj4KZxghXVMiY= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 00BF01F8B9; Fri, 10 Nov 2023 09:19:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1699607955; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=4947QZ5qxD9zPJjKlk7YSnRjNd670NexuKXJc9cjk4o=; b=x8ANaAKgTvq/bOzQgtUAlnlLJrMcQjRNIF3shFPUVc7zskjBVaj2od/SNshP4D2yX/zAn2 KlhsL0xPfBM0FmjyHamPlCXe/sDkxjFxWaKARaTsQ2JDa9KYlVv9XaMXgoqMGnrQuSplQ5 uHZ29KggedWIBEzS6MH311rKSFkCEQA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1699607955; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=4947QZ5qxD9zPJjKlk7YSnRjNd670NexuKXJc9cjk4o=; b=rO/+vLMgK9DFjXlxEWpJRXDSQpssh36+BI541j/EZIZ5ChTr15LRTiNJLM3RaZG0hgBK4l awCt7l4dI/fWdgBg== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 857832C2D7; Fri, 10 Nov 2023 09:19:14 +0000 (UTC) Date: Fri, 10 Nov 2023 09:19:14 +0000 (UTC) From: Richard Biener To: Jakub Jelinek cc: "Joseph S. Myers" , Jason Merrill , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Add type-generic clz/ctz/clrsb/ffs/parity/popcount builtins [PR111309] In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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. > > > 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 SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)