From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 93CB63858402 for ; Thu, 9 Dec 2021 19:39:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 93CB63858402 Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-447-F0zisIrbPqSXrc4rCUrSBg-1; Thu, 09 Dec 2021 14:39:46 -0500 X-MC-Unique: F0zisIrbPqSXrc4rCUrSBg-1 Received: by mail-qv1-f70.google.com with SMTP id jz2-20020a0562140e6200b003bd6c62be47so10801961qvb.11 for ; Thu, 09 Dec 2021 11:39:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=GaBPbRNXL7FH0UYH0t/4VCnqtgi9HrzkWQWYHDy3GfE=; b=vGFVoiQzIOr56VGxLOjjkPnP3acatUu3jbeZHh8WvI+HaKPpIKcZGOMguQU5ovLTRL OgjsvKx0APywSIa5Ka/X/uAhAO7M9/hQ3ekMj1B4Cuo8h2vSKfXXQ+O6RYJqvsJXW442 Y4hLaYtbp7+HWRkb7IS2ol2TWs3Ca7rVy0cEaAbv4C03B4MgNZOgZjzE27pVpzk6VEgP R17bPvTUDUcmycnNE9CsyygYnc/MGWACrdBszqk61Yp8aLHrkoaQ0SfsXQl+foNguDmF VMNimBkDMke5q9fZwdbEL6279W7nTaN/MRdHtcJNpN8FvAOHbXy5iaopwxUX+xmWtzIK kVuw== X-Gm-Message-State: AOAM533P+8xylpibfzV54r6K7iu2DZjGW4Sx7FWahcQXbjQhEpVP1u9B qPawidN5EycJFj6/LknVlwa9GG+AaPlFiTTeJr3CZ3F2mfMMmqgwXIzooUcQrqyWuf19I5ctJmO kA9iJHuo= X-Received: by 2002:a05:6214:104b:: with SMTP id l11mr19085103qvr.111.1639078785582; Thu, 09 Dec 2021 11:39:45 -0800 (PST) X-Google-Smtp-Source: ABdhPJxqA37cppPor2jDu2N67QDQijCf6KjFUYAiPIkJFGSA80zxtqfnhmK/zXoVyDLhrgBYq0f1qg== X-Received: by 2002:a05:6214:104b:: with SMTP id l11mr19085063qvr.111.1639078785235; Thu, 09 Dec 2021 11:39:45 -0800 (PST) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id d5sm518916qte.27.2021.12.09.11.39.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Dec 2021 11:39:44 -0800 (PST) Message-ID: Subject: Re: [PATCH v2] jit: Add support for global rvalue initialization and ctors From: David Malcolm To: Petter Tomner , "gcc-patches@gcc.gnu.org" , "jit@gcc.gnu.org" , Antoni Boucher Date: Thu, 09 Dec 2021 14:39:41 -0500 In-Reply-To: <47b54ae597a44706aba180a05f1e5fe7@kth.se> References: <47b54ae597a44706aba180a05f1e5fe7@kth.se> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=unavailable autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Dec 2021 19:39:51 -0000 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 > 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 [...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 > +#include > + > +#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 > +#include > + > +#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