From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 319683858D1E for ; Fri, 10 Nov 2023 09:10:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 319683858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 319683858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699607445; cv=none; b=Wjn7PZ/mN7N0WvuijRy30lDDcvIZaWl11m5RmR6VU4l32iZY2uyqY8BbDXDKBaM+NPo3cNaP4lT/op/a2kOGwJR9NDp3VEDGrpKnQM6VMo+xpfrJW0v58PQCEN/hsXFF/XmGMRFzPdsPRFbhr0CkUNR+Xu7emWVWrxnwKEhlU6c= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699607445; c=relaxed/simple; bh=ORgbKyWHG4G1q7XKeFTbMxfzRgCk3o7lxo7zN7i+5DA=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=aV6NQHMO8yTuZq1ERDuvQSBGwPDPqvV5vrmTVyIOncZcVx2VP+4gIJlMEYi6Mdk8h4yeC6g+SjxD9KrUyClVSicOR+h6ODzBx/KyIGANwgG8jg8o+HQHlCbJO+g4cZr9c2WHPeK7JU0LPqM98zGFIz+kD/vtu56YF6X3xVeNQoQ= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1699607442; h=from:from:reply-to:reply-to:subject:subject: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=c6vWEWSO20fZuQcRcFjejCX5QjaL0JvmJkcsEgI2X58=; b=etUSX9EtKM7C+vnNzRRnF91MsaqsnyRTtUKHQfnx3g5hHN9fjOJE8oaxqG7w3Kil2i8mW6 IDg7lNrC13Ps2uZ+Z/6QSptCUM5LleS+ISzWP7E3hdvS87RyJaQUxK/QdfNrFrGkK/tKnw ifZOqceqzDCCQ89SLEk8IjJ1BrOBF54= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-663-i4z5MyfbN-q9Fms0Q4izeg-1; Fri, 10 Nov 2023 04:10:41 -0500 X-MC-Unique: i4z5MyfbN-q9Fms0Q4izeg-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B9C9C82DFA2; Fri, 10 Nov 2023 09:10:40 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.81]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 61F88492BE7; Fri, 10 Nov 2023 09:10:40 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 3AA9Abnr1118562 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 10 Nov 2023 10:10:38 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 3AA9AaNx1118561; Fri, 10 Nov 2023 10:10:36 +0100 Date: Fri, 10 Nov 2023 10:10:35 +0100 From: Jakub Jelinek To: Richard Biener 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] Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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, 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. 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