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 <dmalc...@redhat.com>
> Skickat: den 9 december 2021 20:39
> Till: Petter Tomner; gcc-patches@gcc.gnu.org; j...@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 <tom...@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 <tom...@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
>
>
>
