[PATCH] libgccjit: Add support for sized integer types, including 128-bit integers [PR95325]
Antoni Boucher
bouanto@zoho.com
Fri Jan 21 16:22:22 GMT 2022
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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libgccjit-Add-support-for-sized-integer-types-includ.patch
Type: text/x-patch
Size: 31099 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/jit/attachments/20220121/bfea0370/attachment-0001.bin>
More information about the Jit
mailing list