Re: SV: [PATCH v2] jit: Add support for global rvalue initialization and ctors

[Antoni Boucher <bouanto@zoho.com>]

One thing I'm not sure if it is a code style issue, but worth
mentionning:

> @@ -1405,8 +1436,10 @@ private:
> 
>  private:
>    enum gcc_jit_global_kind m_kind;
> +  enum global_var_flags flags = GLOBAL_VAR_FLAGS_NONE;

                           ^^^^^
Should it be named m_flags instead of flags?

>    string *m_name;
>    void *m_initializer;
> +  rvalue *m_rvalue_init = nullptr; /* Only needed for write_dump. */
>    size_t m_initializer_num_bytes;
>  };



Le samedi 11 décembre 2021 à 15:35 +0000, Petter Tomner a écrit :
> Hi!
> 
> > s/an union/a union/
> > s/a rvalue/an rvalue/
> 
> Heh no way ... and I though I knew English grammar :)
> 
> Had to look that up to see what the deal was but it makes sense. 
> 
> yunion, arevalue.
> 
> > s/wrong-field-name/wrong-field-obj/
> > 
> > to match the struct example (given that the issue being tested for
> > is
> > that it's the wrong object, rather than the wrong name).
> 
> Initially, before submitting to the list, I made the code such that
> the field 
> objects did not have to be the ones that were used when creating the 
> struct or union, and forgot changing the test names. 
> 
> I figured it required too much string compares for the field names
> and 
> pointer compares for the field object were more appropriate. To
> create
> dummy field objects were also kinda heavy.
> 
> I'll address the points.
> 
> Regards, Petter
> 
> 
> Från: David Malcolm <dmalcolm@redhat.com>
> Skickat: den 9 december 2021 20:39
> Till: Petter Tomner; gcc-patches@gcc.gnu.org; jit@gcc.gnu.org; Antoni
> Boucher
> Ämne: Re: [PATCH v2] jit: Add support for global rvalue
> initialization and ctors
>     
> On Mon, 2021-12-06 at 10:47 +0000, Petter Tomner via Gcc-patches
> wrote:
> > Hi!
> > 
> > Attached is a patch with changes in line with the review of the
> > prior
> > patch.
> > The patch adds support for initialization of global variables with
> > rvalues as well
> > as rvalue constructors for structs, arrays and unions.
> 
> Thanks for the updated patch.
> 
> Antoni: does this patch work for you for your rustc plugin?
> 
> > 
> > Review: https://gcc.gnu.org/pipermail/jit/2021q4/001400.html
> > 
> > The points have been addressed, except:
> > 
> > > Can the type be made const?
> > 
> > I started to make types_kinda_same_internal () taking const args,
> > but I
> > felt the
> > patch was ballooning because some spread out methods needed a const
> > signature too. I could submit that in a separate patch.
> 
> Fair enough; fixing that isn't a blocker; it's already a big patch.
> 
> > 
> > I also addressed a problem Antoni found: 
> > https://gcc.gnu.org/pipermail/jit/2021q4/001399.html
> > 
> > , where you could not initialize global pointer variables to point
> > to
> > uninitialized variables. I did that by 
> > removing a redundant check with validate_var_has_init (), since
> > that
> > walking function would
> > have to be quite complex to allow pointers to uninitialized
> > variables.
> > 
> > Any:
> > const int foo;
> > int bar = foo;
> > 
> > will instead be reported as "not compile time constant" instead of
> > a
> > nice error message with names.
> > 
> > make check-jit runs fine on gnu-linux-x64 Debian.
> 
> Various review comments inline below, which are mostly just nits:
> 
> > From a4fef1308eaa72ce4ec51dbe5efcfbbf032e9870 Mon Sep 17 00:00:00
> > 2001
> > From: Petter Tomner <tomner@kth.se>
> > Date: Mon, 29 Nov 2021 20:44:07 +0100
> > Subject: [PATCH] Add support for global rvalue initialization and
> > constructors
> > 
> > This patch adds support for initialization of global variables
> > with rvalues and creating constructors for array, struct and
> > union types which can be used as rvalues.
> > 
> > Signed-off-by:
> > 2021-12-06    Petter Tomner   <tomner@kth.se>
> 
> [...snip...]
> 
> > diff --git a/gcc/jit/docs/topics/expressions.rst
> > b/gcc/jit/docs/topics/expressions.rst
> > index 396259ef07e..5f64ca68595 100644
> > --- a/gcc/jit/docs/topics/expressions.rst
> > +++ b/gcc/jit/docs/topics/expressions.rst
> > @@ -126,6 +126,147 @@ Simple expressions
> >      underlying string, so it is valid to pass in a pointer to an
> > on-stack
> >      buffer.
> >   
> > +Constructor expressions
> > +***********************
> > +
> > +   The following functions make constructors for array, struct and
> > union
> > +   types.
> > +
> > +   The constructor rvalue can be used for assignment to locals.
> > +   It can be used to initialize global variables with
> > +   :func:`gcc_jit_global_set_initializer_rvalue`. It can also be
> > used as a
> > +   temporary value for function calls and return values, but its
> > address
> > +   can't be taken.
> > +
> > +   Note that arrays in libgccjit does not collapse to pointers
> > like in
> 
> s/does not/do not/
> 
> > +   C. I.e. if an array constructor is used as e.g. a return value,
> > the whole
> > +   array would be returned by value - array constructors can be
> > assigned to
> > +   array variables.
> > +
> > +   The constructor can contain nested constructors.
> > +
> > +   Note that a string literal rvalue can't be used to construct a
> > char array.
> > +   It need one rvalue for each char.
> 
> s/char array.  It need one rvalue/char array; the latter needs one
> rvalue/
> 
> > +
> > +   These entrypoints were added in :ref:`LIBGCCJIT_ABI_16`; you
> > can test for its
> 
> s/16/17/  I believe.
> 
> s/its/their/
> 
> 
> > +   presense using:
> 
> s/presense/presence/
> 
> (and in various other places below)
> 
> 
> > +   .. code-block:: c
> > +     #ifdef LIBGCCJIT_HAVE_CTORS
> > +
> > +.. function:: gcc_jit_rvalue *\
> > +           gcc_jit_context_new_array_constructor (gcc_jit_context
> > *ctxt,\
> > +                                                  gcc_jit_location
> > *loc,\
> > +                                                  gcc_jit_type
> > *type,\
> > +                                                  size_t
> > arr_length,\
> > +                                                  gcc_jit_rvalue
> > **values)
> > +
> > +   Create a constructor for an array as a rvalue.
> > +
> > +   Returns NULL on error. ``values`` are copied and
> > +   do not have to outlive the context.
> > +
> > +   ``type`` specifies what the constructor will build and has to
> > be
> > +   an array.
> > +
> > +   ``arr_length`` specifies the number of elements in ``values``
> > and
> > +   it can't have more elements than the array type.
> 
> Let's rename this to ``num_values`` (both in the docs, and in
> libgccjit.c and .h, in all of the various places), since this will
> make
> it clearer that we're talking about the size of "values", rather than
> that of the type.
> 
> > +
> > +   Each value in ``values`` sets the corresponding value in the
> > array.
> > +   If the array type itself has more elements than ``values``, the
> > +   left-over elements will be zeroed.
> > +
> > +   Each value in ``values`` need to be the same unqualified type
> > as the
> > +   array type's element type.
> > +
> > +   If ``arr_length`` is 0, the ``values`` parameter will be
> > +   ignored and zero initialization will be used.
> > +
> > +   This entrypoint was added in :ref:`LIBGCCJIT_ABI_17`; you can
> > test for its
> > +   presense using:
> > +
> > +   .. code-block:: c
> > +     #ifdef LIBGCCJIT_HAVE_CTORS
> > +
> > +.. function:: gcc_jit_rvalue *\
> > +           gcc_jit_context_new_struct_constructor (gcc_jit_context
> > *ctxt,\
> > +                                                  
> > gcc_jit_location *loc,\
> > +                                                   gcc_jit_type
> > *type,\
> > +                                                   size_t
> > arr_length,\
> > +                                                   gcc_jit_field
> > **fields,\
> > +                                                   gcc_jit_rvalue
> > **value)
> > +
> > +
> > +   Create a constructor for an struct as a rvalue.
> > +
> > +   Returns NULL on error. The two parameter arrays are copied and
> > +   do not have to outlive the context.
> > +
> > +   ``type`` specifies what the constructor will build and has to
> > be
> > +   a struct.
> > +
> > +   ``arr_length`` specifies the number of elements in ``values``.
> 
> Again, let's make this "num_values" here and in the .c/.h, for
> clarity.
> 
> 
> [...snip...]
> 
> > @@ -603,6 +744,38 @@ Global variables
> >   
> >         #ifdef LIBGCCJIT_HAVE_gcc_jit_global_set_initializer
> >   
> > +.. function:: gcc_jit_lvalue *\
> > +           gcc_jit_global_set_initializer_rvalue (gcc_jit_lvalue
> > *global,
> > +                                                  gcc_jit_rvalue
> > *init_value)
> > +
> > +   Set the initial value of a global with an rvalue.
> > +
> > +   The rvalue need to be a constant expression, e.g. no function
> > calls.
> 
> s/need/needs/
> 
> [...snip...]
> 
> > +void
> > +playback::context::
> > +global_set_init_rvalue (lvalue* variable,
> > +                     rvalue* init)
> > +{
> > +  tree inner = variable->as_tree ();
> > +
> > +  /* We need to fold all expressions as much as possible.  The
> > code
> > +     for a DECL_INITIAL only handles some operations,
> > +     etc addition, minus, 'address of'.  See
> > output_addressed_constants ()
> > +     in varasm.c.  */
> > +  tree init_tree = init->as_tree ();
> > +  tree folded = fold_const_var (init_tree);
> > +
> > +  if (!TREE_CONSTANT (folded))
> > +    {
> > +      tree name = DECL_NAME (inner);
> > +
> > +      add_error (NULL,
> > +              "unable to convert initial value for the global
> > variable %s"
> > +              " to a compile-time constant",
> > +              name != NULL_TREE ? IDENTIFIER_POINTER (name) :
> > NULL);
> 
> It's not safe in general to use NULL with %s, so this needs to be
> split
> into something like:
> 
>       if (name)
>         add_error (NULL,
>                    "unable to convert initial value for the global
> variable %s"
>                    " to a compile-time constant",
>                    IDENTIFIER_POINTER);
>       else
>         add_error (NULL,
>                    "unable to convert initial value for global
> variable"
>                    " to a compile-time constant");
> 
> [...snip...]
> 
> > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > index a1c9436c545..967d8843a98 100644
> > --- a/gcc/jit/libgccjit.h
> > +++ b/gcc/jit/libgccjit.h
> > @@ -812,6 +812,159 @@ gcc_jit_context_new_global (gcc_jit_context
> > *ctxt,
> >                             gcc_jit_type *type,
> >                             const char *name);
> >   
> > +#define LIBGCCJIT_HAVE_CTORS
> > +
> > +/* Create a constructor for an struct as a rvalue.
> 
> s/an struct/a struct/
> 
> s/a rvalue/an rvalue/
> 
> [...snip...]
> 
> 
> > +extern gcc_jit_rvalue *
> > +gcc_jit_context_new_struct_constructor (gcc_jit_context *ctxt,
> > +                                     gcc_jit_location *loc,
> > +                                     gcc_jit_type *type,
> > +                                     size_t arr_length,
> > +                                     gcc_jit_field **fields,
> > +                                     gcc_jit_rvalue **values);
> > +
> > +/* Create a constructor for an union as a rvalue.
> 
> s/an union/a union/
> s/a rvalue/an rvalue/
> 
> [...snip...]
> 
> > +/* Create a constructor for an array as a rvalue.
> 
> s/a rvalue/an rvalue/
> 
> [...snip...]
> 
> > diff --git a/gcc/testsuite/jit.dg/test-error-ctor-array-wrong-
> > type.c b/gcc/testsuite/jit.dg/test-error-ctor-array-wrong-type.c
> > new file mode 100644
> > index 00000000000..1ce83b2ed6c
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-error-ctor-array-wrong-type.c
> > @@ -0,0 +1,54 @@
> > +/*
> > +
> > +  Test that the proper error is triggered when we build a ctor
> > +  for an array type, but has the type wrong on an element.
> > +
> > +*/
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include "libgccjit.h"
> > +#include "harness.h"
> > +
> > +void
> > +create_code (gcc_jit_context *ctxt, void *user_data)
> > +{
> > +  gcc_jit_type *int_type = gcc_jit_context_get_type (ctxt,
> > +    GCC_JIT_TYPE_INT);
> > +  gcc_jit_type *float_type = gcc_jit_context_get_type (ctxt,
> > +    GCC_JIT_TYPE_FLOAT);
> > +
> > +  gcc_jit_type *arr_type =
> > +    gcc_jit_context_new_array_type (ctxt, 0, int_type, 10);
> > +
> > +  gcc_jit_rvalue *frv = gcc_jit_context_new_rvalue_from_double
> > (ctxt,
> > +                                                            
> > float_type,
> > +                                                             12);
> > +
> > +  gcc_jit_rvalue *ctor = gcc_jit_context_new_array_constructor
> > +    (ctxt, 0,
> > +     arr_type,
> > +     1,
> > +     &frv);
> > +
> > +  CHECK_VALUE (ctor, NULL);
> > +}
> > +
> > +void
> > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> > +{
> > +  /* Ensure that the bad API usage prevents the API giving a bogus
> > +     result back.  */
> > +  CHECK_VALUE (result, NULL);
> > +
> > +  /* Verify that the correct error message was emitted. */
> > +  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
> > +                   "gcc_jit_context_new_array_constructor: array
> > element "
> > +                   "value types differ from types in 'values'
> > (element "
> > +                   "type: int)('values' type: float)");
> > +  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
> 
> Looks like a copy&paste error: the second CHECK_STRING_VALUE is
> presumably meant to check get_last_error here, rather than checking
> get_first_error again, right?
> 
> Might be good to introduce a macro, say, EXPECTED_ERROR_MESSAGE, to
> avoid repeating the string literal, which would make such copy&paste
> errors more obvious (and avoid repetition), for all of these various
> error cases.
> 
> > +                   "gcc_jit_context_new_array_constructor: array
> > element "
> > +                   "value types differ from types in 'values'
> > (element "
> > +                   "type: int)('values' type: float)");
> > +}
> 
> [...snip...]
> 
> > diff --git a/gcc/testsuite/jit.dg/test-error-ctor-union-wrong-
> > field-name.c b/gcc/testsuite/jit.dg/test-error-ctor-union-wrong-
> > field-name.c
> > new file mode 100644
> > index 00000000000..2bf8ee4023e
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-error-ctor-union-wrong-field-name.c
> 
> s/wrong-field-name/wrong-field-obj/
> 
> to match the struct example (given that the issue being tested for is
> that it's the wrong object, rather than the wrong name).
> 
> 
> [...snip...]
> 
> > diff --git a/gcc/testsuite/jit.dg/test-error-global-init-too-small-
> > array.c b/gcc/testsuite/jit.dg/test-error-global-init-too-small-
> > array.c
> > new file mode 100644
> > index 00000000000..996d9583860
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-error-global-init-too-small-array.c
> > @@ -0,0 +1,65 @@
> > +/*
> > +
> > +  Test that the proper error is triggered when we initialize
> > +  a global with another non-const global's rvalue.
> 
> I think this comment is wrong; the filename and code suggest this is
> testing something else.  Looks like a copy&paste error from the
> comment
> in test-error-global-lvalue-init.c
> 
> > +
> > +  Using gcc_jit_global_set_initializer_rvalue()
> > +
> > +*/
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include "libgccjit.h"
> > +#include "harness.h"
> > +
> > +void
> > +create_code (gcc_jit_context *ctxt, void *user_data)
> > +{ /* float foo[1] = {1,2}; */
> > +
> > +  gcc_jit_type *float_type = gcc_jit_context_get_type (ctxt,
> > +    GCC_JIT_TYPE_FLOAT);
> > +  gcc_jit_type *arr_type = gcc_jit_context_new_array_type (ctxt,
> > +                                                        0,
> > +                                                       
> > float_type,
> > +                                                        1);
> > +  gcc_jit_rvalue *rval_1 = gcc_jit_context_new_rvalue_from_int (
> > +    ctxt, float_type, 1);
> > +  gcc_jit_rvalue *rval_2 = gcc_jit_context_new_rvalue_from_int (
> > +    ctxt, float_type, 2);
> > +
> > +  gcc_jit_rvalue *values[] = {rval_1, rval_2};
> > +
> > +  gcc_jit_rvalue *ctor = gcc_jit_context_new_array_constructor
> > (ctxt,
> > +                                                             0,
> > +                                                            
> > arr_type,
> > +                                                             2,
> > +                                                            
> > values);
> > +  if (!ctor)
> > +    return;
> > +
> > +  gcc_jit_lvalue *foo =  gcc_jit_context_new_global (
> > +    ctxt, NULL,
> > +    GCC_JIT_GLOBAL_EXPORTED,
> > +    arr_type,
> > +    "global_floatarr_12");
> > +  gcc_jit_global_set_initializer_rvalue (foo, ctor);
> > +}
> > +
> > +void
> > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> > +{
> > +  /* Ensure that the bad API usage prevents the API giving a bogus
> > +     result back.  */
> > +  CHECK_VALUE (result, NULL);
> > +
> > +  /* Verify that the correct error message was emitted. */
> > +  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
> > +                   "gcc_jit_context_new_array_constructor: array "
> > +                   "constructor has more values than the array
> > type's "
> > +                   "length (array type length: 1, constructor
> > length: 2)");
> > +  CHECK_STRING_VALUE (gcc_jit_context_get_last_error (ctxt),
> > +                   "gcc_jit_context_new_array_constructor: array "
> > +                   "constructor has more values than the array
> > type's "
> > +                   "length (array type length: 1, constructor
> > length: 2)");
> > +}
> 
> [...snip...]
> 
> > diff --git a/gcc/testsuite/jit.dg/test-global-init-rvalue.c
> > b/gcc/testsuite/jit.dg/test-global-init-rvalue.c
> > new file mode 100644
> > index 00000000000..21675ac9acf
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-global-init-rvalue.c
> > @@ -0,0 +1,1563 @@
> > +/* This testcase checks that
> > gcc_jit_global_set_initializer_rvalue() works
> > +   with rvalues, especially with
> > gcc_jit_context_new_*_constructor() for
> > +   struct, unions and arrays. */
> 
> [...snip...]
> 
> The order in which you create things in create_code isn't quite the
> same as the order in which you check things in verify_code.  Is it
> possible to reorder things so that these are consistent?  That would
> make it easier to compare the libgccjit calls with the expected
> behavior.
> 
> Thanks for all the test coverage, BTW - both of valid usage and
> error-
> handling - it makes me much happier about the patch.
> 
> [...snip...]
> 
> I would say "OK for trunk, with those nits fixed", but I want to hear
> Antoni's opinion on whether this works for him for the rustc plugin,
> or
> if he needs further changes.  Antoni - does this patch work for you?
> 
> Thanks again for the patch; this is looking close to ready; hope the
> above makes sense.
> Dave
> 
> 
>