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 [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id B5F283857808 for ; Thu, 15 Oct 2020 21:52:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B5F283857808 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-568-cD6OTK9cN5exiUnICmnzXQ-1; Thu, 15 Oct 2020 17:52:36 -0400 X-MC-Unique: cD6OTK9cN5exiUnICmnzXQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 13C0983DC24; Thu, 15 Oct 2020 21:52:35 +0000 (UTC) Received: from ovpn-112-135.phx2.redhat.com (ovpn-112-135.phx2.redhat.com [10.3.112.135]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3C0CF5D9CA; Thu, 15 Oct 2020 21:52:34 +0000 (UTC) Message-ID: <54ba5c58dbd2b8c7b1f3276c2b87cddf55e258bd.camel@redhat.com> Subject: Re: [PATCH] libgccjit: add some reflection functions in the jit C api From: David Malcolm To: Antoni Boucher , Andrea Corallo Cc: Antoni Boucher via Jit , gcc-patches@gcc.gnu.org Date: Thu, 15 Oct 2020 17:52:33 -0400 In-Reply-To: <20201015173952.ft7mu5ajbaxb3gke@bouanto-desktop.localdomain> References: <20200902010120.crnx55ev635ceiey@bouanto-desktop.localdomain> <6fe2ea355403eb177c9632e076c11c792f23c791.camel@redhat.com> <9cd00fe5cb5c03184e3a5bd939447b30450a8ca7.camel@redhat.com> <20201015160226.cka4iq3w22jztopm@bouanto-desktop.localdomain> <20201015173952.ft7mu5ajbaxb3gke@bouanto-desktop.localdomain> User-Agent: Evolution 3.36.5 (3.36.5-1.fc32) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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=-13.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_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable 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, 15 Oct 2020 21:52:43 -0000 On Thu, 2020-10-15 at 13:39 -0400, Antoni Boucher wrote: > Thanks. I updated the patch with these changes. Thanks for patch; review below. Sorry if it seems excessively nitpicky in places. > 2020-09-1 Antoni Boucher > > gcc/jit/ > PR target/96889 > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_14): New ABI tag. 15 now. > * docs/topics/functions.rst: Add documentation for the > functions gcc_jit_function_get_return_type and > gcc_jit_function_get_param_count > * libgccjit.c: New functions: > * gcc_jit_function_get_return_type; > * gcc_jit_function_get_param_count; > * gcc_jit_function_type_get_return_type; > * gcc_jit_function_type_get_param_count; > * gcc_jit_function_type_get_param_type; > * gcc_jit_type_unqualified; > * gcc_jit_type_is_array; > * gcc_jit_type_is_bool; > * gcc_jit_type_is_function_ptr_type; > * gcc_jit_type_is_int; > * gcc_jit_type_is_pointer; > * gcc_jit_type_is_vector; > * gcc_jit_vector_type_get_element_type; > * gcc_jit_vector_type_get_num_units; > * gcc_jit_struct_get_field; > * gcc_jit_type_is_struct; > * gcc_jit_struct_get_field_count; This isn't valid ChangeLog format; it will fail the git hooks. > * libgccjit.h Likewise. > * jit-recording.h: New functions (is_struct and is_vector) > * libgccjit.map (LIBGCCJIT_ABI_14): New ABI tag. 15 now. > > gcc/testsuite/ > PR target/96889 > * jit.dg/all-non-failing-tests.h: Add test-reflection.c. > * jit.dg/test-reflection.c: New test. [...] > diff --git a/gcc/jit/docs/topics/functions.rst b/gcc/jit/docs/topics/functions.rst > index eb40d64010e..9819c28cda2 100644 > --- a/gcc/jit/docs/topics/functions.rst > +++ b/gcc/jit/docs/topics/functions.rst > @@ -171,6 +171,16 @@ Functions > underlying string, so it is valid to pass in a pointer to an on-stack > buffer. > > +.. function:: size_t \ > + gcc_jit_function_get_param_count (gcc_jit_function *func) > + > + Get the number of parameters of the function. > + > +.. function:: gcc_jit_type \* > + gcc_jit_function_get_return_type (gcc_jit_function *func) > + > + Get the return type of the function. > + The documentation part of the patch is incomplete: it hasn't been updated to add all the new entrypoints. Also, the return type of gcc_jit_function_get_param_count is inconsistent (size_t above, but ssize_t below). > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > index 30e37aff387..525b8bc921d 100644 > --- a/gcc/jit/jit-recording.h > +++ b/gcc/jit/jit-recording.h > @@ -538,7 +538,9 @@ public: > virtual bool is_bool () const = 0; > virtual type *is_pointer () = 0; > virtual type *is_array () = 0; > + virtual struct_ *is_struct () { return NULL; } Can't you use dyn_cast_struct for this? Or is this about looking through decorated_type? e.g. for const and volatile variants? I guess my question is, what is the purpose of gcc_jit_type_is_struct? > virtual bool is_void () const { return false; } > + virtual vector_type *is_vector () { return NULL; } Likewise, can't you use dyn_cast_vector_type for this? > virtual bool has_known_size () const { return true; } > > bool is_numeric () const > @@ -595,6 +597,8 @@ public: > bool is_bool () const FINAL OVERRIDE; > type *is_pointer () FINAL OVERRIDE { return dereference (); } > type *is_array () FINAL OVERRIDE { return NULL; } > + vector_type *is_vector () FINAL OVERRIDE { return NULL; } > + struct_ *is_struct () FINAL OVERRIDE { return NULL; } Likewise, and this is redundant, as it's merely copying the base class implementation. > bool is_void () const FINAL OVERRIDE { return m_kind == GCC_JIT_TYPE_VOID; } > > public: > @@ -629,6 +633,8 @@ public: > bool is_bool () const FINAL OVERRIDE { return false; } > type *is_pointer () FINAL OVERRIDE { return m_other_type; } > type *is_array () FINAL OVERRIDE { return NULL; } > + vector_type *is_vector () FINAL OVERRIDE { return NULL; } > + struct_ *is_struct () FINAL OVERRIDE { return NULL; } Likewise. > @@ -655,6 +661,7 @@ public: > bool is_bool () const FINAL OVERRIDE { return m_other_type->is_bool (); } > type *is_pointer () FINAL OVERRIDE { return m_other_type->is_pointer (); } > type *is_array () FINAL OVERRIDE { return m_other_type->is_array (); } > + struct_ *is_struct () FINAL OVERRIDE { return m_other_type->is_struct (); } Aha: with a decorated type you look through the decoration. > protected: > type *m_other_type; > @@ -737,6 +744,8 @@ public: > > void replay_into (replayer *) FINAL OVERRIDE; > > + vector_type *is_vector () FINAL OVERRIDE { return this; } > + > private: > string * make_debug_string () FINAL OVERRIDE; > void write_reproducer (reproducer &r) FINAL OVERRIDE; > @@ -765,6 +774,8 @@ class array_type : public type > bool is_bool () const FINAL OVERRIDE { return false; } > type *is_pointer () FINAL OVERRIDE { return NULL; } > type *is_array () FINAL OVERRIDE { return m_element_type; } > + vector_type *is_vector () FINAL OVERRIDE { return NULL; } > + struct_ *is_struct () FINAL OVERRIDE { return NULL; } Are these redundant? > int num_elements () { return m_num_elements; } > > void replay_into (replayer *) FINAL OVERRIDE; > @@ -799,6 +810,8 @@ public: > bool is_bool () const FINAL OVERRIDE { return false; } > type *is_pointer () FINAL OVERRIDE { return NULL; } > type *is_array () FINAL OVERRIDE { return NULL; } > + vector_type *is_vector () FINAL OVERRIDE { return NULL; } > + struct_ *is_struct () FINAL OVERRIDE { return NULL; } Likewise. > void replay_into (replayer *) FINAL OVERRIDE; > > @@ -912,6 +925,7 @@ public: > bool is_bool () const FINAL OVERRIDE { return false; } > type *is_pointer () FINAL OVERRIDE { return NULL; } > type *is_array () FINAL OVERRIDE { return NULL; } > + vector_type *is_vector () FINAL OVERRIDE { return NULL; } Likewise. > bool has_known_size () const FINAL OVERRIDE { return m_fields != NULL; } > > @@ -943,6 +957,8 @@ public: > > const char *access_as_type (reproducer &r) FINAL OVERRIDE; > > + struct_ *is_struct () FINAL OVERRIDE { return this; } > + > private: > string * make_debug_string () FINAL OVERRIDE; > void write_reproducer (reproducer &r) FINAL OVERRIDE; > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > index a00aefc7108..8e6a9e85ff0 100644 > --- a/gcc/jit/libgccjit.c > +++ b/gcc/jit/libgccjit.c > @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see > #include "config.h" > #include "system.h" > #include "coretypes.h" > +#include "tm.h" Why is this included? It's not mentioned in the ChangeLog. > #include "timevar.h" > #include "typed-splay-tree.h" > #include "cppbuiltin.h" > @@ -60,6 +61,14 @@ struct gcc_jit_struct : public gcc::jit::recording::struct_ > { > }; > > +struct gcc_jit_function_type : public gcc::jit::recording::function_type > +{ > +}; > + > +struct gcc_jit_vector_type : public gcc::jit::recording::vector_type > +{ > +}; > + FWIW these aren't mentioned in the ChangeLog either. > struct gcc_jit_field : public gcc::jit::recording::field > { > }; > @@ -510,6 +519,197 @@ gcc_jit_type_get_volatile (gcc_jit_type *type) [...] > +/* Public entrypoint. See description in libgccjit.h. > + > + After error-checking, the real work is done by the > + gcc::jit::recording::function_type::get_param_types method, in > + jit-recording.c. */ > + > +gcc_jit_type * > +gcc_jit_function_type_get_param_type (gcc_jit_function_type *function_type, > + int index) > +{ > + RETURN_NULL_IF_FAIL (function_type, NULL, NULL, "NULL struct_type"); > + int num_params = function_type->get_param_types ().length (); > + gcc::jit::recording::context *ctxt = function_type->m_ctxt; Should check that index >= 0 as well (or make it unsigned). Given that gcc_jit_function_get_param_count returns ssize_t, should that be the type of "index"? > + RETURN_NULL_IF_FAIL_PRINTF3 (index < num_params, > + ctxt, NULL, > + "index of %d is too large (%s has %d params)", > + index, > + function_type->get_debug_string (), > + num_params); > + return (gcc_jit_type *)function_type->get_param_types ()[index]; > +} [...] > +extern gcc_jit_field * > +gcc_jit_struct_get_field (gcc_jit_struct *struct_type, > + int index) > +{ > + RETURN_NULL_IF_FAIL (struct_type, NULL, NULL, "NULL struct type"); Similar comments as per gcc_jit_function_type_get_param_type above. Should check here that index is in range (>=0 and < num fields). > + return (gcc_jit_field *)struct_type->get_fields ()->get_field (index); > +} > + > +/* Public entrypoint. See description in libgccjit.h. > + > + After error-checking, this calls the trivial > + gcc::jit::recording::struct_::get_fields method in > + jit-recording.h. */ > + > +ssize_t > +gcc_jit_struct_get_field_count (gcc_jit_struct *struct_type) > +{ > + RETURN_VAL_IF_FAIL (struct_type, -1, NULL, NULL, "NULL struct type"); > + return struct_type->get_fields ()->length (); > +} > + [...] > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h > index 7134841bb07..6056d00a3ea 100644 > --- a/gcc/jit/libgccjit.h > +++ b/gcc/jit/libgccjit.h > @@ -96,6 +96,12 @@ typedef struct gcc_jit_field gcc_jit_field; > the layout for, or an opaque type. */ > typedef struct gcc_jit_struct gcc_jit_struct; > > +/* A gcc_jit_function_type encapsulates a function type. */ > +typedef struct gcc_jit_function_type gcc_jit_function_type; > + > +/* A gcc_jit_vector_type encapsulates a vector type. */ > +typedef struct gcc_jit_vector_type gcc_jit_vector_type; > + > /* A gcc_jit_function encapsulates a function: either one that you're > creating yourself, or a reference to one that you're dynamically > linking to within the rest of the process. */ > @@ -586,6 +592,61 @@ gcc_jit_type_get_const (gcc_jit_type *type); > extern gcc_jit_type * > gcc_jit_type_get_volatile (gcc_jit_type *type); > > +/* Given type "T", return TRUE if the type is an array. */ > +extern gcc_jit_type * > +gcc_jit_type_is_array (gcc_jit_type *type); The comment is unclear; does this actually return the type vs NULL? As noted above, I think we need to think about what happens on qualified types; is stripping them the responsibility of the caller? Should it be? (I'm thinking "yes", but presumably all this is motivated by your rust hacking). Also, there isn't a type "T" here; the param name is "type", and there isn't a supporting example using "T" as a placeholder. Maybe just lose the 'Given type "T", ' prefix here and below (unless giving a example). > +/* Given type "T", return TRUE if the type is a bool. */ > +extern int > +gcc_jit_type_is_bool (gcc_jit_type *type); I'd prefer "return non-zero" over "return TRUE" (here, and in various places below). Does this check for the "bool" type i.e.: /* C++'s bool type; also C99's "_Bool" type, aka "bool" if using stdbool.h. */ GCC_JIT_TYPE_BOOL, ? > +/* Given type "T", return the function type if it is one. */ ..."or NULL". > +extern gcc_jit_function_type * > +gcc_jit_type_is_function_ptr_type (gcc_jit_type *type); > + > +/* Given a function type, return its return type. */ > +extern gcc_jit_type * > +gcc_jit_function_type_get_return_type (gcc_jit_function_type *function_type); > + > +/* Given a function type, return its number of parameters. */ > +extern ssize_t > +gcc_jit_function_type_get_param_count (gcc_jit_function_type *function_type); > + > +/* Given a function type, return the type of the specified parameter. */ > +extern gcc_jit_type * > +gcc_jit_function_type_get_param_type (gcc_jit_function_type *function_type, > + int index); > + > +/* Given type "T", return the type pointed by the pointer type > + * or NULL if it's not a pointer. */ > +extern gcc_jit_type * > +gcc_jit_type_is_pointer (gcc_jit_type *type); > + > +/* Given type "T", return TRUE if the type is an int. */ > +extern int > +gcc_jit_type_is_int (gcc_jit_type *type); I'm not sure I understand what this function does, which suggests it's misnamed. The current name suggests that it checks for the C "int" type, whereas I believe it actually checks for an integral type. Should it be: extern int gcc_jit_type_is_integral (gcc_jit_type *type); ? What's the output? Do you merely need nonzero/zero for boolean, or information on the width of the type/signedness, or whatnot? (perhaps have some out params for extracting width/signedness for when it returns nonzero?) Looking at the testcase, it looks like it currently returns false on the _Bool type. Is that correct? > +/* Given type "T", return the unqualified type, i.e. for a > + * decorated type, return the type it wraps. */ > +extern gcc_jit_type * > +gcc_jit_type_unqualified (gcc_jit_type *type); "decorated" is a term used inside the implementation, but not in the user-facing docs. How about: /* Given a type, return the unqualified type, removing "const", "volatile" and alignment qualifiers. */ or somesuch. > +/* Given type "T", return TRUE if the type is a vector type. */ > +extern gcc_jit_vector_type * > +gcc_jit_type_is_vector (gcc_jit_type *type); As above, this doesn't return TRUE, it's more of a dynamic cast. Likewise about responsibility for qualifiers. > +/* Given type "T", return the struct type or NULL. */ > +extern gcc_jit_struct * > +gcc_jit_type_is_struct (gcc_jit_type *type); Wording could be improved as noted above. > +/* Given a vector type, return the number of units it contains. */ > +extern ssize_t > +gcc_jit_vector_type_get_num_units (gcc_jit_vector_type *vector_type); > + > +/* Given a vector type, return the type of its elements. */ > +extern gcc_jit_type * > +gcc_jit_vector_type_get_element_type (gcc_jit_vector_type *vector_type); > + [...] > @@ -647,6 +708,15 @@ gcc_jit_struct_set_fields (gcc_jit_struct *struct_type, > int num_fields, > gcc_jit_field **fields); > > +/* Get a field by index. */ > +extern gcc_jit_field * > +gcc_jit_struct_get_field (gcc_jit_struct *struct_type, > + int index); > + > +/* Get the number of fields. */ > +extern ssize_t > +gcc_jit_struct_get_field_count (gcc_jit_struct *struct_type); Do we need these for unions? Or skip them for now? > @@ -740,6 +810,14 @@ gcc_jit_function_as_object (gcc_jit_function *func); > extern gcc_jit_param * > gcc_jit_function_get_param (gcc_jit_function *func, int index); > > +/* Get the number of params of a function. */ > +extern ssize_t > +gcc_jit_function_get_param_count (gcc_jit_function *func); > + > +/* Get the return type of a function. */ > +extern gcc_jit_type * > +gcc_jit_function_get_return_type (gcc_jit_function *func); Are these redundant if there's a way to ask a gcc_jit_function for its type, and then to use the gcc_jit_function_type_ functions? (I don't mind if they are) > @@ -1518,6 +1596,47 @@ gcc_jit_version_minor (void); > extern int > gcc_jit_version_patchlevel (void); > > +#define LIBGCCJIT_HAVE_gcc_jit_function_reflection > + > +/* Reflection functions to get the number of parameters, return type of > + a function and whether a type is a bool from the C API. > + > + This API entrypoint was added in LIBGCCJIT_ABI_15; you can test for its > + presence using > + #ifdef LIBGCCJIT_HAVE_gcc_jit_function_reflection > +*/ This has become a big mixture of things beyond just function reflection, so I think I'd prefer it if each entrypoint "funcname" had its own "LIBGCCJIT_HAVE_funcname" define. Or just "REFLECTION" rather than "function_reflection", that's probably simpler. Note that the existing "HAVE_foo" defines are lowercase "foo" when it's an actual entrypoint named "foo", and are uppercase otherwise e.g. LIBGCCJIT_HAVE_SWITCH_STATEMENTS and LIBGCCJIT_HAVE_TIMING_API. > +extern gcc_jit_type * > +gcc_jit_function_get_return_type (gcc_jit_function *func); > +extern ssize_t > +gcc_jit_function_get_param_count (gcc_jit_function *func); > +extern gcc_jit_type * > +gcc_jit_type_is_array (gcc_jit_type *type); > +extern int > +gcc_jit_type_is_bool (gcc_jit_type *type); > +extern gcc_jit_function_type * > +gcc_jit_type_is_function_ptr_type (gcc_jit_type *type); > +extern gcc_jit_type * > +gcc_jit_function_type_get_return_type (gcc_jit_function_type *function_type); > +extern ssize_t > +gcc_jit_function_type_get_param_count (gcc_jit_function_type *function_type); > +extern gcc_jit_type * > +gcc_jit_function_type_get_param_type (gcc_jit_function_type *function_type, > + int index); > +extern int > +gcc_jit_type_is_int (gcc_jit_type *type); > +extern gcc_jit_type * > +gcc_jit_type_is_pointer (gcc_jit_type *type); > +extern gcc_jit_vector_type * > +gcc_jit_type_is_vector (gcc_jit_type *type); > +extern gcc_jit_struct * > +gcc_jit_type_is_struct (gcc_jit_type *type); > +extern ssize_t > +gcc_jit_vector_type_get_num_units (gcc_jit_vector_type *vector_type); > +extern gcc_jit_type * > +gcc_jit_vector_type_get_element_type (gcc_jit_vector_type *vector_type); > +extern gcc_jit_type * > +gcc_jit_type_unqualified (gcc_jit_type *type); Is this part of the patch a duplicate copy of the various things added above? Perhaps copy-and-paste error, or am I misreading this? [...] Hope this is constructive Dave