Re: [PATCH] libgccjit: Add support for bitcasts [PR104071]

[David Malcolm <dmalcolm@redhat.com>]

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