Re: [PATCH] libgccjit: Add support for sized integer types, including 128-bit integers [PR95325]

[David Malcolm <dmalcolm@redhat.com>]

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