On Fri, 2022-04-08 at 16:29 -0400, Antoni Boucher wrote: > David, it seems you missed this email that contains the updated patch > and a few questions. > > Attaching the patch again. > > Thanks for the reviews!
Thanks for the patch. I updated the patch to fix some minor nits: - Some whitespace fixes (tab vs spaces, overlong lines, continuation lines in .rst ". function::" clause) - I kept compatible_types, with gcc_jit_compatible_types becoming a thin wrapper around it. This avoids a bunch of casts in libgccjit.cc - Regenerated docs/_build/texinfo/libgccjit.texi Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. I've pushed the patch to trunk for GCC 12 as r12-8116-gaf80ea97b61847: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=af80ea97b61847d91da0d303e85faed437059092 Dave > > On Fri, 2022-01-21 at 11:22 -0500, Antoni Boucher via Jit wrote: > > David: this is the email I was talking about in my other email. > > Here's the updated patch. > > > > By the way, I find the usage of NUM_GCC_JIT_TYPES brittle. Would it > > be > > better to switch to a new enum value for that instead? > > > > See comments below. > > > > Le jeudi 20 mai 2021 à 15:25 -0400, David Malcolm a écrit : > > > 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? > > > > > > > > Not a review, just a comment. 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. > > > > uint16_type_node = make_or_reuse_type (16, 1); > > > > uint32_type_node = make_or_reuse_type (32, 1); > > > > uint64_type_node = make_or_reuse_type (64, 1); > > > > if (targetm.scalar_mode_supported_p (TImode)) > > > > uint128_type_node = make_or_reuse_type (128, 1); > > > > are always with the given precisions, perhaps jit should use > > > > signed_type_for (uint16_type_node) etc.? > > > > > > I seem to have mislaid Antoni's original email (sorry), so I'll > > > reply > > > to Jakub's. > > > > > > > 2021-05-18 Antoni Boucher <boua...@zoho.com> > > > > > > > > gcc/jit/ > > > > PR target/95325 > > > > * jit-playback.c: Add support for the sized integer > > > > types. > > > > * jit-recording.c: Add support for the sized integer > > > > types. > > > > * libgccjit.h (GCC_JIT_TYPE_UINT8_T, > > > > GCC_JIT_TYPE_UINT16_T, > > > > GCC_JIT_TYPE_UINT32_T, GCC_JIT_TYPE_UINT64_T, > > > > GCC_JIT_TYPE_UINT128_T, GCC_JIT_TYPE_INT8_T, > > > > GCC_JIT_TYPE_INT16_T, > > > > GCC_JIT_TYPE_INT32_T, GCC_JIT_TYPE_INT64_T, > > > > GCC_JIT_TYPE_INT128_T): > > > > New enum variants for gcc_jit_types. > > > > gcc/testsuite/ > > > > PR target/95325 > > > > * jit.dg/test-types.c: Add tests for sized integer > > > > types. > > > > > > First a high-level question, why not use (or extend) > > > gcc_jit_context_get_int_type instead? > > > > If I remember correctly, I believe I had some issues with this > > function, like having it return sometimes long long, and other times > > long for the same size. Maybe that was an issue with a global > > variable > > not cleaned up. > > > > > > > > Do we really need to extend enum gcc_jit_types? Is this a quality- > > > of- > > > life thing for users of the library? > > > > > > 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. > > > > > > 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? > > > > > > 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? > > > > I updated the function compatible_types to make them compare as > > equal. > > I believe that it's not used everywhere though, so a cast will be > > necessary in some cases. > > > > > > > > Various comments inline below... > > > > > > > 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_) > > > > case GCC_JIT_TYPE_UNSIGNED_INT: > > > > return unsigned_type_node; > > > > > > > > + case GCC_JIT_TYPE_UINT8_T: > > > > + return unsigned_intQI_type_node; > > > > + case GCC_JIT_TYPE_UINT16_T: > > > > + return uint16_type_node; > > > > + case GCC_JIT_TYPE_UINT32_T: > > > > + return uint32_type_node; > > > > + case GCC_JIT_TYPE_UINT64_T: > > > > + return uint64_type_node; > > > > + case GCC_JIT_TYPE_UINT128_T: > > > > + return uint128_type_node; > > > > + case GCC_JIT_TYPE_INT8_T: > > > > + return intQI_type_node; > > > > + case GCC_JIT_TYPE_INT16_T: > > > > + return intHI_type_node; > > > > + case GCC_JIT_TYPE_INT32_T: > > > > + return intSI_type_node; > > > > + case GCC_JIT_TYPE_INT64_T: > > > > + return intDI_type_node; > > > > + case GCC_JIT_TYPE_INT128_T: > > > > + return intTI_type_node; > > > > + > > > > > > Jakub has already commented that 128 bit types might not be > > > available. > > > > > > 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. > > > > > > 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. > > > 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. > > > > > > > > > > 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 > > > > () > > > > case GCC_JIT_TYPE_UNSIGNED_LONG_LONG: > > > > size = LONG_LONG_TYPE_SIZE; > > > > break; > > > > + case GCC_JIT_TYPE_UINT8_T: > > > > + case GCC_JIT_TYPE_UINT16_T: > > > > + case GCC_JIT_TYPE_UINT32_T: > > > > + case GCC_JIT_TYPE_UINT64_T: > > > > + case GCC_JIT_TYPE_UINT128_T: > > > > + case GCC_JIT_TYPE_INT8_T: > > > > + case GCC_JIT_TYPE_INT16_T: > > > > + case GCC_JIT_TYPE_INT32_T: > > > > + case GCC_JIT_TYPE_INT64_T: > > > > + case GCC_JIT_TYPE_INT128_T: > > > > + size = 128; > > > > + break; > > > > > > This gives 128 bits as the size for all of these types, which seems > > > wrong. > > > > > > > case GCC_JIT_TYPE_FLOAT: > > > > size = FLOAT_TYPE_SIZE; > > > > break; > > > > case GCC_JIT_TYPE_FLOAT: > > > > > > > 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 > > > > GCC_JIT_TYPE_LONG_LONG, /* signed */ > > > > GCC_JIT_TYPE_UNSIGNED_LONG_LONG, > > > > > > > > + GCC_JIT_TYPE_UINT8_T, > > > > + GCC_JIT_TYPE_UINT16_T, > > > > + GCC_JIT_TYPE_UINT32_T, > > > > + GCC_JIT_TYPE_UINT64_T, > > > > + GCC_JIT_TYPE_UINT128_T, > > > > + GCC_JIT_TYPE_INT8_T, > > > > + GCC_JIT_TYPE_INT16_T, > > > > + GCC_JIT_TYPE_INT32_T, > > > > + GCC_JIT_TYPE_INT64_T, > > > > + GCC_JIT_TYPE_INT128_T, > > > > + > > > > /* Floating-point types */ > > > > > > > > GCC_JIT_TYPE_FLOAT, > > > > > > 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. > > > > > > 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. > > > > Fixed. > > > > > > > > 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. I'm not > > > sure > > > how to do that. > > > > Now this patch adds new functions. Does that solve this issue? > > > > > > > > > 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 > > > > > > [...snip...] > > > > > > The tests seem reasonable, but as noted by Jakub the 128-bit > > > support > > > needs to be conditionalized. > > > > > > Hope this is constructive > > > Dave > > > > > >
