From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sender4-pp-o91.zoho.com (sender4-pp-o91.zoho.com [136.143.188.91]) by sourceware.org (Postfix) with ESMTPS id 3832C3858406; Tue, 4 Jan 2022 03:13:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3832C3858406 ARC-Seal: i=1; a=rsa-sha256; t=1641266006; cv=none; d=zohomail.com; s=zohoarc; b=fQfn0wncDTojTeefHL6wjkGeThOgcUh1tD3o2rgI5jWUK1UwGGK43jQ+3en/zDJdFelJ0up4VELYS+TDdtlpybD5DsH0AwcvJBPpDKfVIGjucFhTsLQCtolRqPG/36GPFRdYeT/tLmN+ln1/jDj6hUWe+IWlVCO2eMnThJS6EFY= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1641266006; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:To; bh=a0ia8E4BiMLK8VMpn++MuVlk2lPVk2a2sb4CjJUJ6sE=; b=RQ2PLG5FjfaP6W2/2k+TLzXitt8EEuifDO7tHYRxr+G46GxDOpcjNlmdJ+SjSMeSOw5mlvx6ZWmRkXVY/3+1VOvWj8qQnmaU9rGW3Ty2N5vI15jEX6DL8Kg/kecR3diMpICR+N/+ALSSpRMUntZ87ix7vsel4sYnhpfCb12nUW4= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass header.i=zoho.com; spf=pass smtp.mailfrom=bouanto@zoho.com; dmarc=pass header.from= Received: from [192.168.1.174] (38.87.11.6 [38.87.11.6]) by mx.zohomail.com with SMTPS id 1641266004356839.7780794931887; Mon, 3 Jan 2022 19:13:24 -0800 (PST) Message-ID: <76839a3787bbf6d7a50c8d7791679323088209cf.camel@zoho.com> Subject: Re: [PATCH] libgccjit: Add support for sized integer types, including 128-bit integers [PR95325] From: Antoni Boucher To: David Malcolm Cc: jit@gcc.gnu.org, gcc-patches@gcc.gnu.org Date: Mon, 03 Jan 2022 22:13:22 -0500 In-Reply-To: <5f858ec27ded31ddddf4564f3a3b20c2e29b015f.camel@redhat.com> References: <92a0bd2aad201de75a71d0a00414e518cf1716e6.camel@zoho.com> <20210518125306.GP1179226@tucnak> <5f858ec27ded31ddddf4564f3a3b20c2e29b015f.camel@redhat.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.42.2 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-ZohoMailClient: External X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Jan 2022 03:13:33 -0000 Hi, David! One question below as I'm still not done with using this patch in rustc_codegen_gcc. Le jeudi 20 mai 2021 =C3=A0 15:25 -0400, David Malcolm a =C3=A9crit=C2=A0: > On Tue, 2021-05-18 at 14:53 +0200, Jakub Jelinek via Jit wrote: > > On Tue, May 18, 2021 at 08:23:56AM -0400, Antoni Boucher via Gcc- > > patches wrote: > > > Hello. > > > This patch add support for sized integer types. > > > Maybe it should check whether the size of a byte for the current > > > platform is 8 bits and do other checks so that they're only > > > available > > > when it makes sense. > > > What do you think? > >=20 > > Not a review, just a comment.=C2=A0 The 128-bit integral types are > > available > > only on some targets, the test e.g. the C/C++ FE do for those is > > targetm.scalar_mode_supported_p (TImode) > > and so even libgccjit shouldn't provide those types > > unconditionally. > > Similarly for the tests (though it could be guarded with e.g > > #ifdef __SIZEOF_INT128__ > > in that case). > > Also, while currently all in tree targets have BITS_PER_UNIT 8 and > > therefore QImode is 8-bit, HImode 16-bit, SImode 32-bit and DImode > > 64- > > bit, > > in the past and maybe in he future there can be targets that could > > have > > e.g. 16-bit or 32-bit QImode and then there wouldn't be any > > uint8_t/int8_t > > and int16_t would be intQImode_type_node etc. > > =C2=A0 uint16_type_node =3D make_or_reuse_type (16, 1); > > =C2=A0 uint32_type_node =3D make_or_reuse_type (32, 1); > > =C2=A0 uint64_type_node =3D make_or_reuse_type (64, 1); > > =C2=A0 if (targetm.scalar_mode_supported_p (TImode)) > > =C2=A0=C2=A0=C2=A0 uint128_type_node =3D make_or_reuse_type (128, 1); > > are always with the given precisions, perhaps jit should use > > signed_type_for (uint16_type_node) etc.? >=20 > I seem to have mislaid Antoni's original email (sorry), so I'll reply > to Jakub's. >=20 > > 2021-05-18=C2=A0 Antoni Boucher=C2=A0 > >=20 > > =C2=A0=C2=A0=C2=A0 gcc/jit/ > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 PR t= arget/95325 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * ji= t-playback.c: Add support for the sized integer > > types. > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * ji= t-recording.c: Add support for the sized integer > > types. > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * li= bgccjit.h (GCC_JIT_TYPE_UINT8_T, > > GCC_JIT_TYPE_UINT16_T, > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 GCC_= JIT_TYPE_UINT32_T, GCC_JIT_TYPE_UINT64_T, > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 GCC_= JIT_TYPE_UINT128_T, GCC_JIT_TYPE_INT8_T, > > GCC_JIT_TYPE_INT16_T, > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 GCC_= JIT_TYPE_INT32_T, GCC_JIT_TYPE_INT64_T, > > GCC_JIT_TYPE_INT128_T): > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 New = enum variants for gcc_jit_types. > > =C2=A0=C2=A0=C2=A0 gcc/testsuite/ > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 PR t= arget/95325 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * ji= t.dg/test-types.c: Add tests for sized integer > > types. >=20 > First a high-level question, why not use (or extend) > gcc_jit_context_get_int_type instead? >=20 > Do we really need to extend enum gcc_jit_types?=C2=A0 Is this a quality- > of- > life thing for users of the library? >=20 > That said, recording::context::get_int_type is currently a bit of a > hack, and maybe could probably be improved by using the new enum > values > the patch adds. >=20 > IIRC, libgccjit.c does type-checking by comparing recording::type > pointer values; does this patch gives us multiple equivalent types > that > ought to compare as equal? >=20 > If a user gets a type via GCC_JIT_TYPE_INT and gets "another" type > via > GCC_JIT_TYPE_INT32_T and they happen to be the same on the current > target, should libgccjit complain if you use "int" when you meant > "int32_t", or accept it? >=20 > Various comments inline below... >=20 > > diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c > > index c6136301243..40630aa1ab8 100644 > > --- a/gcc/jit/jit-playback.c > > +++ b/gcc/jit/jit-playback.c > > @@ -193,6 +193,27 @@ get_tree_node_for_type (enum gcc_jit_types > > type_) > > =C2=A0=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UNSIGNED_INT: > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return unsigned_type_node; > > =C2=A0 > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UINT8_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return unsigned_intQI_type_node; > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UINT16_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return uint16_type_node; > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UINT32_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return uint32_type_node; > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UINT64_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return uint64_type_node; > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UINT128_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return uint128_type_node; > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_INT8_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return intQI_type_node; > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_INT16_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return intHI_type_node; > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_INT32_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return intSI_type_node; > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_INT64_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return intDI_type_node; > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_INT128_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return intTI_type_node; > > + >=20 > Jakub has already commented that 128 bit types might not be > available. >=20 > Ideally we'd report that they're not available as soon as the user > tries to use them, in gcc_jit_context_get_type, but unfortunately it > looks like the test requires us to use > targetm.scalar_mode_supported_p, > and that requires us to hold the jit mutex and thus be at playback > time. I'd need a way to check if 128-bit integers are supported and if not, use something else. The fact that this check is only available at playback makes this tricky. I tried the hack of creating a context, creating a 128-bit integer type and checking for errors, but even this won't work, because creating a new context will call initialize_sizetypes, which will overwrite the types and cause some issues later, for instance having VIEW_CONVERT_EXPR fails some checks (https://github.com/gcc-mirror/gcc/blob/master/gcc/tree-cfg.c#L3203) because the tree for the sizes will be different (even though the value of those trees is the same). Any thoughts on how best to solve this issue? >=20 > So I think get_tree_node_for_type should take a context, and add an > error on the context if there's a failure, returning NULL.=20 > playback::context::get_type is the only caller currently and has > handling for an unrecognized value, so I think that logic needs to be > moved to get_tree_node_for_type so that the user can distinguish > between unrecognized types versus types that are unsupported on this > target. >=20 >=20 > > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c > > index 117ff70114c..b67ae8bfb78 100644 > > --- a/gcc/jit/jit-recording.c > > +++ b/gcc/jit/jit-recording.c > > @@ -2247,6 +2247,18 @@ recording::memento_of_get_type::get_size () > > =C2=A0=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UNSIGNED_LONG_LONG: > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 size =3D LONG_LONG_TYPE_SIZE; > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break; > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UINT8_T: > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UINT16_T: > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UINT32_T: > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UINT64_T: > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_UINT128_T: > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_INT8_T: > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_INT16_T: > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_INT32_T: > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_INT64_T: > > +=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_INT128_T: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 size =3D 128; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break; >=20 > This gives 128 bits as the size for all of these types, which seems > wrong. >=20 > > =C2=A0=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_FLOAT: > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 size =3D FLOAT_TYPE_SIZE; > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break; > > =C2=A0=C2=A0=C2=A0=C2=A0 case GCC_JIT_TYPE_FLOAT: >=20 > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h > > index 5c722c2c57f..5d88033a2ab 100644 > > --- a/gcc/jit/libgccjit.h > > +++ b/gcc/jit/libgccjit.h > > @@ -548,6 +548,17 @@ enum gcc_jit_types > > =C2=A0=C2=A0 GCC_JIT_TYPE_LONG_LONG, /* signed */ > > =C2=A0=C2=A0 GCC_JIT_TYPE_UNSIGNED_LONG_LONG, > > =C2=A0 > > +=C2=A0 GCC_JIT_TYPE_UINT8_T, > > +=C2=A0 GCC_JIT_TYPE_UINT16_T, > > +=C2=A0 GCC_JIT_TYPE_UINT32_T, > > +=C2=A0 GCC_JIT_TYPE_UINT64_T, > > +=C2=A0 GCC_JIT_TYPE_UINT128_T, > > +=C2=A0 GCC_JIT_TYPE_INT8_T, > > +=C2=A0 GCC_JIT_TYPE_INT16_T, > > +=C2=A0 GCC_JIT_TYPE_INT32_T, > > +=C2=A0 GCC_JIT_TYPE_INT64_T, > > +=C2=A0 GCC_JIT_TYPE_INT128_T, > > + > > =C2=A0=C2=A0 /* Floating-point types=C2=A0 */ > > =C2=A0 > > =C2=A0=C2=A0 GCC_JIT_TYPE_FLOAT, >=20 > The patch adds the new enum values between existing values of enum > gcc_jit_types, effectively changing the ABI, since e.g. the numerical > value of GCC_JIT_TYPE_FLOAT changes with this, and there's no way of > telling which enum values a binary linked against libgccjit.so is > going > to supply when it calls into libgccjit. >=20 > If we're going to extend the enum, the new values need to be added to > the end, extending the ABI rather than changing it. >=20 > I don't think the patch provides a way to detect that the client code > is using the new ABI and thus mark it in the metadata.=C2=A0 I'm not sure > how to do that. >=20 > > diff --git a/gcc/testsuite/jit.dg/test-types.c > > b/gcc/testsuite/jit.dg/test-types.c > > index 8debcd7eb82..9c66284f193 100644 > > --- a/gcc/testsuite/jit.dg/test-types.c > > +++ b/gcc/testsuite/jit.dg/test-types.c >=20 > [...snip...] >=20 > The tests seem reasonable, but as noted by Jakub the 128-bit support > needs to be conditionalized. >=20 > Hope this is constructive > Dave >=20