public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
From: David Malcolm <dmalcolm@redhat.com>
To: Petter Tomner <tomner@kth.se>,
	"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
	"jit@gcc.gnu.org" <jit@gcc.gnu.org>,
	Antoni Boucher <bouanto@zoho.com>
Subject: Re: [PATCH v2] jit: Add support for global rvalue initialization and ctors
Date: Thu, 09 Dec 2021 14:39:41 -0500	[thread overview]
Message-ID: <da58dd43c555592254c382fd17f8f8887e7c9792.camel@redhat.com> (raw)
In-Reply-To: <47b54ae597a44706aba180a05f1e5fe7@kth.se>

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



  reply	other threads:[~2021-12-09 19:39 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-06 10:47 Petter Tomner
2021-12-09 19:39 ` David Malcolm [this message]
2021-12-11 15:35   ` SV: " Petter Tomner
2021-12-13  1:39     ` Antoni Boucher
2021-12-13 19:22       ` David Malcolm
2021-12-14  0:30         ` SV: " Petter Tomner
2021-12-13  1:43     ` Antoni Boucher
2021-12-13  1:48     ` Antoni Boucher

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=da58dd43c555592254c382fd17f8f8887e7c9792.camel@redhat.com \
    --to=dmalcolm@redhat.com \
    --cc=bouanto@zoho.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jit@gcc.gnu.org \
    --cc=tomner@kth.se \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).