[PATCH] libgccjit: Add support for bitcasts [PR104071]
David Malcolm
dmalcolm@redhat.com
Tue Jan 18 23:06:15 GMT 2022
On Mon, 2022-01-17 at 19:30 -0500, Antoni Boucher via Gcc-patches
wrote:
> I was missing the define, so I added it.
> Here's the new patch with it.
Thanks for the patch.
> Le lundi 17 janvier 2022 à 17:18 -0500, Antoni Boucher via Jit a
> écrit :
> > Hi.
> > This patch add support for bitcasts in libgccjit.
> >
> > It passes the JIT tests, but since I added a function in tree.c, I
> > wonder if I should run the whole testsuite.
We're in stage 4 for GCC 12 now, so we need to be especially careful
and conservative about every change. A strict reading on the rules is
that we shouldn't be adding new features - but if they're confined to
libgccjit we may be able to get release manager approval.
> >
> > David, you can now disregard my question in my email about 128-bit
> > integers regarding my issue with initialize_sizetypes being called
> > multiple times because this patch fix this issue.
> > I turns out there was a cache of types that needed to be cleared
> > when
> > you initialize the JIT.
> >
> > The check for sizes is pending, because it requires the updates to
> > get_size I made in my patch for 128-bit integers.
Sorry, I seem to have mislaid that patch; do you have the "Subject"
line handy?
Do you have a list of the patches I need to review?
As for this patch, overall I like it, but there are various nits...
> >
> > Thanks for the review!
> 2022-01-17 Antoni Boucher <bouanto@zoho.com>
>
> gcc/jit/
> PR target/104071
Should be "jit" rather than "target".
Various source files are now .cc rather than .c after yesterday's big
renaming.
> * docs/topics/compatibility.rst (LIBGCCJIT_ABI_20): New ABI tag.
> * docs/topics/expressions.rst: Add documentation for the
> function gcc_jit_context_new_bitcast.
> * dummy-frontend.c: clear the cache of non-standard integer
> types to avoid having issues with some optimizations of
> bitcast where the SSA_NAME will have a size of a cached
> integer type that should have been invalidated, causing a
> comparison of integer constant to fail.
> * jit-playback.c: New function (new_bitcast).
> * jit-playback.h: New function (new_bitcast).
> * jit-recording.c: New functions (new_bitcast,
> bitcast::replay_into, bitcast::visit_children,
> bitcast::make_debug_string, bitcast::write_reproducer).
> * jit-recording.h: New calss (bitcast) and new function
> (new_bitcast, bitcast::replay_into, bitcast::visit_children,
> bitcast::make_debug_string, bitcast::write_reproducer,
> bitcast::get_precedence).
> * libgccjit.c: New function (gcc_jit_context_new_bitcast)
> * libgccjit.h: New function (gcc_jit_context_new_bitcast)
> * libgccjit.map (LIBGCCJIT_ABI_20): New ABI tag.
>
> gcc/testsuite/
> PR target/104071
> * jit.dg/all-non-failing-tests.h: Add new test-bitcast.
> * jit.dg/test-bitcast.c: New test.
>
> gcc/
> PR target/104071
> * tree.c: New function (clear_nonstandard_integer_type_cache).
> * tree.h: New function (clear_nonstandard_integer_type_cache).
> ---
> gcc/jit/docs/topics/compatibility.rst | 9 +++
> gcc/jit/docs/topics/expressions.rst | 17 +++++
> gcc/jit/dummy-frontend.c | 2 +
> gcc/jit/jit-playback.c | 13 ++++
> gcc/jit/jit-playback.h | 5 ++
> gcc/jit/jit-recording.c | 66 ++++++++++++++++++++
> gcc/jit/jit-recording.h | 32 ++++++++++
> gcc/jit/libgccjit.c | 28 +++++++++
> gcc/jit/libgccjit.h | 15 +++++
> gcc/jit/libgccjit.map | 6 ++
> gcc/testsuite/jit.dg/all-non-failing-tests.h | 10 +++
> gcc/testsuite/jit.dg/test-bitcast.c | 60 ++++++++++++++++++
> gcc/tree.c | 8 +++
> gcc/tree.h | 1 +
> 14 files changed, 272 insertions(+)
> create mode 100644 gcc/testsuite/jit.dg/test-bitcast.c
>
> diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
> index 16cebe31a10..b5a6b704dda 100644
> --- a/gcc/jit/docs/topics/compatibility.rst
> +++ b/gcc/jit/docs/topics/compatibility.rst
> @@ -302,3 +302,12 @@ thread-local storage model of a variable:
> section of a variable:
>
> * :func:`gcc_jit_lvalue_set_link_section`
> +
> +.. _LIBGCCJIT_ABI_20:
> +
> +``LIBGCCJIT_ABI_20``
> +-----------------------
> +``LIBGCCJIT_ABI_20`` covers the addition of an API entrypoint to bitcast a
> +value from one type to another:
> +
> + * :func:`gcc_jit_context_new_bitcast`
> diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst
> index 791a20398ca..1328a53f70f 100644
> --- a/gcc/jit/docs/topics/expressions.rst
> +++ b/gcc/jit/docs/topics/expressions.rst
> @@ -649,6 +649,23 @@ Type-coercion
> * int <-> bool
> * P* <-> Q*, for pointer types P and Q
>
> +.. function:: gcc_jit_rvalue *\
> + gcc_jit_context_new_bitcast (gcc_jit_context *ctxt,\
> + gcc_jit_location *loc,\
> + gcc_jit_rvalue *rvalue,\
> + gcc_jit_type *type)
> +
> + Given an rvalue of T, bitcast it to another type.
I think it's worth defining what "bitcast" means in the docs here;
presumably you mean something like "generating a new rvalue by
interpreting the bits of the input rvalue according to the layout of
type" or somesuch.
> +
> + The type of rvalue must be the same size as the size of ``type``.
> +
> + This entrypoint was added in :ref:`LIBGCCJIT_ABI_20`; you can test for
> + its presence using
> +
> + .. code-block:: c
> +
> + #ifdef LIBGCCJIT_HAVE_gcc_jit_context_new_bitcast
> +
> Lvalues
> -------
>
> diff --git a/gcc/jit/dummy-frontend.c b/gcc/jit/dummy-frontend.c
> index 84ff359bfe3..c3da97642e3 100644
> --- a/gcc/jit/dummy-frontend.c
> +++ b/gcc/jit/dummy-frontend.c
> @@ -592,6 +592,8 @@ jit_langhook_init (void)
> global_dc->begin_diagnostic = jit_begin_diagnostic;
> global_dc->end_diagnostic = jit_end_diagnostic;
>
> + clear_nonstandard_integer_type_cache ();
> +
I've been putting code to cleanup global state in foo.c into
foo_c_finalize functions; I'm testing a patch right now to rename these
to foo_cc_finalize functions to reflect the renaming of .c to .cc
These cleanup functions are called from toplev::finalize.
I think it would be better to invoke this cleanup from a new
tree_cc_finalize that follows that pattern.
> build_common_tree_nodes (false);
>
> /* I don't know why this has to be done explicitly. */
[...snip...]
> diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> index 03704ef10b8..cd8516d1c4d 100644
> --- a/gcc/jit/libgccjit.c
> +++ b/gcc/jit/libgccjit.c
> @@ -2405,6 +2405,34 @@ gcc_jit_context_new_cast (gcc_jit_context *ctxt,
> return static_cast <gcc_jit_rvalue *> (ctxt->new_cast (loc, rvalue, type));
> }
>
> +/* Public entrypoint. See description in libgccjit.h.
> +
> + After error-checking, the real work is done by the
> + gcc::jit::recording::context::new_bitcast method in jit-recording.c. */
> +
> +gcc_jit_rvalue *
> +gcc_jit_context_new_bitcast (gcc_jit_context *ctxt,
> + gcc_jit_location *loc,
> + gcc_jit_rvalue *rvalue,
> + gcc_jit_type *type)
> +{
> + RETURN_NULL_IF_FAIL (ctxt, NULL, loc, "NULL context");
> + JIT_LOG_FUNC (ctxt->get_logger ());
> + /* LOC can be NULL. */
> + RETURN_NULL_IF_FAIL (rvalue, ctxt, loc, "NULL rvalue");
> + RETURN_NULL_IF_FAIL (type, ctxt, loc, "NULL type");
> + // TODO: check the sizes.
> + /*RETURN_NULL_IF_FAIL_PRINTF3 (
> + is_valid_cast (rvalue->get_type (), type),
> + ctxt, loc,
> + "cannot cast %s from type: %s to type: %s",
> + rvalue->get_debug_string (),
> + rvalue->get_type ()->get_debug_string (),
> + type->get_debug_string ());*/
I belive that in general we can't do the checking for equal size until
we have trees i.e. at playback time. I suspect that if the user gets
this wrong, it will lead to an internal compiler error at playback
time.
Please can you add a testcase that tests mismatching sizes e.g. trying
to interpret an int as a array of 4096 bytes, or something similar
that's wildly wrong (ideally coverage for casting in both directions).
Ideally we can generate a useful error message, or, at least, not
crash.
> +
> + return static_cast <gcc_jit_rvalue *> (ctxt->new_bitcast (loc, rvalue, type));
> +}
> +
> /* Public entrypoint. See description in libgccjit.h.
>
> After error-checking, the real work is done by the
[...snip...]
> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index 29afe064db6..656351edce1 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> @@ -77,6 +77,13 @@
> /* test-builtin-unreachable.c: We don't add this one, since it touches
> the optimization level of the context as a whole. */
>
> +/* test-bitcast.c */
> +#define create_code create_code_bitcast
> +#define verify_code verify_code_bitcast
> +#include "test-bitcast.c"
> +#undef create_code
> +#undef verify_code
> +
> /* test-calling-external-function.c */
> #define create_code create_code_calling_external_function
> #define verify_code verify_code_calling_external_function
> @@ -400,6 +407,9 @@ const struct testcase testcases[] = {
> {"builtin-memcpy",
> create_code_builtin_memcpy,
> verify_code_builtin_memcpy},
> + {"bitcast",
> + create_code_bitcast,
> + verify_code_bitcast},
> {"calling_external_function",
> create_code_calling_external_function,
> verify_code_calling_external_function},
> diff --git a/gcc/testsuite/jit.dg/test-bitcast.c b/gcc/testsuite/jit.dg/test-bitcast.c
> new file mode 100644
> index 00000000000..2d70622051a
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-bitcast.c
> @@ -0,0 +1,60 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "libgccjit.h"
> +
> +#include "harness.h"
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> + /* Let's try to inject the equivalent of:
> +int
> +my_bitcast (double x)
> +{
> + return bitcast(x, int);
> +}
> + */
> + gcc_jit_type *int_type =
> + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> + gcc_jit_type *double_type =
> + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_FLOAT);
This says "double_type" but is GCC_JIT_TYPE_FLOAT.
I don't think we're guaranteed that sizeof(int) == sizeof(float) on all targets.
> +
> + gcc_jit_param *x =
> + gcc_jit_context_new_param (
> + ctxt,
> + NULL,
> + double_type, "x");
> + gcc_jit_param *params[1] = {x};
> + gcc_jit_function *func =
> + gcc_jit_context_new_function (ctxt,
> + NULL,
> + GCC_JIT_FUNCTION_EXPORTED,
> + int_type,
> + "my_bitcast",
> + 1, params, 0);
> +
> + gcc_jit_block *initial =
> + gcc_jit_function_new_block (func, "initial");
> +
> + gcc_jit_block_end_with_return(initial, NULL,
> + gcc_jit_context_new_bitcast(ctxt,
> + NULL,
> + gcc_jit_param_as_rvalue(x),
> + int_type
> + ));
> +}
> +
> +void
> +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> +{
> + typedef int (*my_bitcast_fn_type) (double);
> + CHECK_NON_NULL (result);
> + my_bitcast_fn_type my_bitcast =
> + (my_bitcast_fn_type)gcc_jit_result_get_code (result,
"my_bitcast");
> + CHECK_NON_NULL (my_bitcast);
> + int val = my_bitcast (-5.1298714);
> + note ("my_bitcast returned: %d", val);
> + CHECK_VALUE (val, 35569201);
> +}
> diff --git a/gcc/tree.c b/gcc/tree.c
> index d98b77db50b..e4bf4e84675 100644
> --- a/gcc/tree.c
> +++ b/gcc/tree.c
> @@ -6963,6 +6963,14 @@ build_reference_type (tree to_type)
> (HOST_BITS_PER_WIDE_INT > 64 ? HOST_BITS_PER_WIDE_INT : 64)
> static GTY(()) tree nonstandard_integer_type_cache[2 *
MAX_INT_CACHED_PREC + 2];
>
> +void clear_nonstandard_integer_type_cache (void)
> +{
> + for (size_t i = 0 ; i < 2 * MAX_INT_CACHED_PREC + 2 ; i++)
> + {
> + nonstandard_integer_type_cache[i] = NULL;
> + }
> +}
> +
As noted above (for gcc/jit/dummy-frontend.c), make this static, and
call it from a new tree_cc_finalize function, and call that from
toplev::finalize.
> /* Builds a signed or unsigned integer type of precision PRECISION.
> Used for C bitfields whose precision does not match that of
> built-in target types. */
> diff --git a/gcc/tree.h b/gcc/tree.h
> index 318019c4dc5..640b492802c 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -5385,6 +5385,7 @@ extern bool real_minus_onep (const_tree);
> extern void init_ttree (void);
> extern void build_common_tree_nodes (bool);
> extern void build_common_builtin_nodes (void);
> +extern void clear_nonstandard_integer_type_cache (void);
...and get rid of this in favor of a tree_cc_finalize.
> extern tree build_nonstandard_integer_type (unsigned HOST_WIDE_INT,
int);
> extern tree build_nonstandard_boolean_type (unsigned HOST_WIDE_INT);
> extern tree build_range_type (tree, tree, tree);
> --
> 2.26.2.7.g19db9cfb68.dirty
>
Hope this is constructive; thanks again for the patch
Dave
More information about the Gcc-patches
mailing list