From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 780C1385483E; Thu, 13 May 2021 08:33:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 780C1385483E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mliska@suse.cz X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 8C886AD9F; Thu, 13 May 2021 08:33:05 +0000 (UTC) Subject: Re: [PATCH] libgccjit: add some reflection functions in the jit C api To: Antoni Boucher , David Malcolm Cc: Antoni Boucher via Jit , gcc-patches@gcc.gnu.org References: <20200902010120.crnx55ev635ceiey@bouanto-desktop.localdomain> <6fe2ea355403eb177c9632e076c11c792f23c791.camel@redhat.com> <9cd00fe5cb5c03184e3a5bd939447b30450a8ca7.camel@redhat.com> <20201015160226.cka4iq3w22jztopm@bouanto-desktop.localdomain> <20201015173952.ft7mu5ajbaxb3gke@bouanto-desktop.localdomain> <54ba5c58dbd2b8c7b1f3276c2b87cddf55e258bd.camel@redhat.com> <20201103221324.xw3jbvrw2uwdc4rz@bouanto-desktop.localdomain> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: Date: Thu, 13 May 2021 10:33:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20201103221324.xw3jbvrw2uwdc4rz@bouanto-desktop.localdomain> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00, BODY_8BITS, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, 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, 13 May 2021 08:33:09 -0000 @David: PING On 11/3/20 11:13 PM, Antoni Boucher via Gcc-patches wrote: > I was missing a check in gcc_jit_struct_get_field, I added it in this new patch. > > On Thu, Oct 15, 2020 at 05:52:33PM -0400, David Malcolm wrote: >> 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 >>