From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by sourceware.org (Postfix) with ESMTP id 4035B3983C55 for ; Thu, 10 Sep 2020 22:22:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4035B3983C55 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-252-dE5V2EfiPK2eipr-rPZtyQ-1; Thu, 10 Sep 2020 18:22:47 -0400 X-MC-Unique: dE5V2EfiPK2eipr-rPZtyQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 27FCA1DDFD; Thu, 10 Sep 2020 22:22:46 +0000 (UTC) Received: from ovpn-114-91.phx2.redhat.com (ovpn-114-91.phx2.redhat.com [10.3.114.91]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6F7EE7FB93; Thu, 10 Sep 2020 22:22:45 +0000 (UTC) Message-ID: Subject: Re: [PATCH V2] libgccjit: Add new gcc_jit_context_new_blob entry point From: David Malcolm To: Andrea Corallo Cc: jit@gcc.gnu.org, nd@arm.com, gcc-patches@gcc.gnu.org Date: Thu, 10 Sep 2020 18:22:44 -0400 In-Reply-To: References: <59a8d345c642d49281a601278946e087a4bbe3e2.camel@redhat.com> <87zh9kumwi.fsf@arm.com> <93e3d65a0b04b13a5d5c9970a2058d167357ed6c.camel@redhat.com> <22479e6a6a1e27df07a3d2c2cfb8c6c8420a7d3d.camel@redhat.com> User-Agent: Evolution 3.32.5 (3.32.5-1.fc30) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0.003 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 10 Sep 2020 22:22:55 -0000 On Wed, 2020-08-19 at 09:17 +0200, Andrea Corallo wrote: > David Malcolm writes: > > > Thanks for the updated patch. Comments inline below. > > Hi Dave, > > sorry for the late reply. Likewise, sorry. [...] > > Why the non-void return type? Looking at libgccjit.c I see it returns > > "global" if it succeeds, or NULL if it fails. Wouldn't it be better to > > simply have void return type, and rely on the normaly error-handling > > mechanisms? > > Or is this inspired by the inline asm patch? (for PR 87291) > > The idea is that this way the user could also create the global, > initialize it and directly use it like: > > foo (gcc_jit_global_set_initializer (gcc_jit_context_new_global (...), ...)) > > I left it that way, let me know if you prefer otherwise. I don't have a strong preference here, so let's go with what you've written. [...] > >> +/* Construct an initialized playback::lvalue instance (wrapping a > >> + tree). */ > >> + > >> +playback::lvalue * > >> +playback::context:: > >> +new_global_initialized (location *loc, > >> + enum gcc_jit_global_kind kind, > >> + type *type, > >> + size_t element_size, > >> + size_t initializer_num_elem, > >> + const void *initializer, > >> + const char *name) > >> +{ > >> + tree inner = global_new_decl (loc, kind, type, name); > >> + > >> + static vec *constructor_elements; > > > > Why the use of a function-level static variable here, and why va_gc? > > Wouldn't an auto_vec be cleaner? > > Ah: is it because of the call to build_constructor? > > If so, can the "static" be removed and replaced with an "= NULL;" > > initializer? > > > > (I'm very wary of function-level static variables, as they're a place > > when state can "hide" between multiple in-process invocations of the > > compiler) > > Sorry that's due to my ignorance on how GCC GC works, I thought GC roots > had to be static, fixed. FWIW I don't think this is a GC root; the only GTY-marked roots in gcc/jit are those in dummy-frontend.c Looking at gcc/jit/notes.txt, this playback code is called in the "No GC in here" region within jit_langhook_parse_file. [...] > diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst > index d783ceea51a..28f81be0060 100644 > --- a/gcc/jit/docs/topics/expressions.rst > +++ b/gcc/jit/docs/topics/expressions.rst > @@ -582,6 +582,27 @@ Global variables > referring to it. Analogous to using an "extern" global from a > header file. > > +.. function:: gcc_jit_lvalue *\ > + gcc_jit_global_set_initializer (gcc_jit_lvalue *global,\ > + const void *blob,\ > + size_t num_bytes) > + > + Set an initializer for an object using the memory content pointed > + by ``blob`` for ``num_bytes``. ``global`` must be an array of an > + integral type. > + > + The parameter ``blob`` must be non-NULL. The call copies the memory > + pointed by ``blob`` for ``num_bytes`` bytes, so it is valid to pass > + in a pointer to an on-stack buffer. The content will be stored in > + the compilation unit and used as initialization value of the array. Please document here that the return value is "global". [...] > +template > +static void > +load_blob_in_ctor (vec *&constructor_elements, > + size_t num_elem, > + const void *initializer) > +{ > + /* Loosely based on 'output_init_element' c-typeck.c:9691. */ > + const T *p = (const T *)initializer; > + tree node = make_unsigned_type (BITS_PER_UNIT * sizeof (T)); > + for (size_t i = 0; i < num_elem; i++) > + { > + constructor_elt celt = > + { build_int_cst (long_unsigned_type_node, i), > + build_int_cst (node, p[i]) }; > + vec_safe_push (constructor_elements, celt); In theory it would be slightly quicker to grow the vector once, and use vec_quick_push inside the loop, but given how much allocation we're doing it may be lost in the noise. > + } > +} > + > +/* Construct an initialized playback::lvalue instance (wrapping a > + tree). */ > + > +playback::lvalue * > +playback::context:: > +new_global_initialized (location *loc, > + enum gcc_jit_global_kind kind, > + type *type, > + size_t element_size, > + size_t initializer_num_elem, > + const void *initializer, > + const char *name) > +{ > + tree inner = global_new_decl (loc, kind, type, name); > + > + vec *constructor_elements = NULL; > + > + switch (element_size) > + { > + case 1: > + load_blob_in_ctor (constructor_elements, initializer_num_elem, > + initializer); > + break; > + case 2: > + load_blob_in_ctor (constructor_elements, initializer_num_elem, > + initializer); > + break; > + case 4: > + load_blob_in_ctor (constructor_elements, initializer_num_elem, > + initializer); > + break; > + case 8: > + load_blob_in_ctor (constructor_elements, initializer_num_elem, > + initializer); > + break; > + default: > + gcc_unreachable (); Is there a way to hit this gcc_unreachable? If I'm reading it right, presumably this is only going to be called on the result of get_size, and this can only return the sizes covered by the cases, right? If so, please add a comment to this effect. [...] > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c > index b73cd76a0a0..39510097c6f 100644 > --- a/gcc/jit/jit-recording.c > +++ b/gcc/jit/jit-recording.c > @@ -2175,6 +2175,57 @@ recording::type::access_as_type (reproducer &r) > return r.get_identifier (this); > } > > +/* Override of default implementation of > + recording::type::get_size. > + > + Return the size in bytes. This is in use for global > + initialization. */ > + > +size_t > +recording::memento_of_get_type::get_size () > +{ > + int size; > + switch (m_kind) > + { > + case GCC_JIT_TYPE_VOID: > + return 0; > + case GCC_JIT_TYPE_BOOL: > + case GCC_JIT_TYPE_CHAR: > + case GCC_JIT_TYPE_SIGNED_CHAR: > + case GCC_JIT_TYPE_UNSIGNED_CHAR: > + return 1; > + case GCC_JIT_TYPE_SHORT: > + case GCC_JIT_TYPE_UNSIGNED_SHORT: > + size = SHORT_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_INT: > + case GCC_JIT_TYPE_UNSIGNED_INT: > + size = INT_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_LONG: > + case GCC_JIT_TYPE_UNSIGNED_LONG: > + size = LONG_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_LONG_LONG: > + case GCC_JIT_TYPE_UNSIGNED_LONG_LONG: > + size = LONG_LONG_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_FLOAT: > + size = FLOAT_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_DOUBLE: > + size = DOUBLE_TYPE_SIZE; > + break; > + case GCC_JIT_TYPE_LONG_DOUBLE: > + size = LONG_DOUBLE_TYPE_SIZE; > + break; > + default: > + gcc_unreachable (); > + } > + > + return size / BITS_PER_UNIT; > +} > + > /* Implementation of pure virtual hook recording::type::dereference for > recording::memento_of_get_type. */ > [...] > @@ -4472,6 +4580,25 @@ recording::global::write_reproducer (reproducer &r) > global_kind_reproducer_strings[m_kind], > r.get_identifier_as_type (get_type ()), > m_name->get_debug_string ()); > + > + if (m_initializer) > + switch (m_type->dereference ()->get_size ()) > + { > + case 1: > + write_initializer_reproducer (id, r); > + break; > + case 2: > + write_initializer_reproducer (id, r); > + break; > + case 4: > + write_initializer_reproducer (id, r); > + break; > + case 8: > + write_initializer_reproducer (id, r); > + break; > + default: > + gcc_unreachable (); Similar comments as above. > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > index 726b9c4b837..aa38e606a8d 100644 > --- a/gcc/jit/jit-recording.h > +++ b/gcc/jit/jit-recording.h > @@ -502,6 +502,12 @@ public: > This will return NULL if it's not valid to dereference this type. > The caller is responsible for setting an error. */ > virtual type *dereference () = 0; > + /* Get the type size in bytes. > + > + This is implemented only for memento_of_get_type and > + memento_of_get_pointer as is use for initializing globals of "as is use" -> "as it is used" [...] > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > index 3d04f6db3af..bc45bd80b2c 100644 > --- a/gcc/jit/libgccjit.c > +++ b/gcc/jit/libgccjit.c [...] > +extern gcc_jit_lvalue * > +gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > + const void *blob, > + size_t num_bytes) > +{ [...] > + RETURN_NULL_IF_FAIL_PRINTF3 ( > + lvalue_size == num_bytes, NULL, NULL, > + "global \"%s\" of size %zu don't match initializer of size %zu", > + global->get_debug_string (), lvalue_size, num_bytes); Please reword (following the pattern of the other "mismatching" errors in libgccjit.c) to: "mismatching sizes:" " global \"%s\" has size %zu whereas initializer has size %zu", global->get_debug_string (), lvalue_size, num_bytes); [...] > + reinterpret_cast (global) > + ->set_initializer (blob, num_bytes); Was there a reason for using reinterpret_cast here, rather than static_cast? The file uses static_cast for this kind of thing throughout, so I'd prefer that here. [...] > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h > index 1c5a12e9c01..3cbcbef2693 100644 > --- a/gcc/jit/libgccjit.h > +++ b/gcc/jit/libgccjit.h > @@ -788,6 +788,20 @@ gcc_jit_context_new_global (gcc_jit_context *ctxt, > gcc_jit_type *type, > const char *name); > > +#define LIBGCCJIT_HAVE_gcc_jit_global_set_initializer > + > +/* Set a static initializer for a global return the global itself. Please reword to: /* Set an initial value for a global, which must be an array of integral type. Return the global itself. > + > + This API entrypoint was added in LIBGCCJIT_ABI_14; you can test for its > + presence using > + #ifdef LIBGCCJIT_HAVE_gcc_jit_global_set_initializer > +*/ > + > +extern gcc_jit_lvalue * > +gcc_jit_global_set_initializer (gcc_jit_lvalue *global, > + const void *blob, > + size_t num_bytes); > + [...] OK for master with these nits fixed, assuming your usual testing. Thanks! Dave