Hi.
Here's the updated patch.

See comments below.

Le mardi 18 janvier 2022 à 18:06 -0500, David Malcolm a écrit :
> 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.

Ok, if the 4 patches currently being reviewed (and listed here:
https://github.com/antoyo/libgccjit-patches) were included in gcc 12,
I'd be able to build rustc_codegen_gcc with an unpatched gcc.

It is to be noted however, that I'll need more patches for future work.
Off the top of my head, I'll at least need a patch for the inline
attribute, try/catch and target-specific builtins.
The last 2 features will probably take some time to implement, so I'll
let you judge if you think it's worth merging the 4 patches currently
being reviewed for gcc 12.

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

I recently sent an email with that patch updated, but here's the
subject line:
[PATCH] libgccjit: Add support for sized integer types, including 128-
bit integers [PR95325]

> 
> Do you have a list of the patches I need to review?

Yes, on this repo:
https://github.com/antoyo/libgccjit-patches

They are outdated but I can update them if you want.

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

Done.

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

Done.

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

Done.

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

Done.

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

I switched to using `gcc_jit_context_get_int_type (ctxt, 4, 1)` for the
integer type.

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