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

[Antoni Boucher <bouanto@zoho.com>]

Hi, David!

One question below as I'm still not done with using this patch in
rustc_codegen_gcc.

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

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?

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