David, it seems you missed this email that contains the updated patch
and a few questions.

Attaching the patch again.

Thanks for the reviews!

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