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

[David Malcolm <dmalcolm@redhat.com>]

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.

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