public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
From: Antoni Boucher <bouanto@zoho.com>
To: David Malcolm <dmalcolm@redhat.com>
Cc: gcc-patches@gcc.gnu.org, jit@gcc.gnu.org
Subject: Re: [PATCH] libgccjit: add some reflection functions in the jit C api [WIP]
Date: Sat, 3 Oct 2020 14:14:10 -0400	[thread overview]
Message-ID: <20201003181410.iggpsfrwlmu7gq5k@bouanto-desktop.localdomain> (raw)
In-Reply-To: <9cd00fe5cb5c03184e3a5bd939447b30450a8ca7.camel@redhat.com>

By the way, that seemed off to return NULL for the function returning a 
size_t to indicate an error. So I changed it to return -1 (and return 
type to ssize_t).  Is that the proper way to indicate an error?

I also wanted to mention that this patch is still a work-in-progress as 
I'm adding a few other functions.

On Fri, Oct 02, 2020 at 04:24:26PM -0400, David Malcolm wrote:
>On Fri, 2020-10-02 at 16:17 -0400, David Malcolm wrote:
>> On Tue, 2020-09-01 at 21:01 -0400, Antoni Boucher via Jit wrote:
>> > Hello.
>> > This WIP patch implements new reflection functions in the C API as
>> > mentioned in bug 96889.
>> > I'm looking forward for feedbacks on this patch.
>> > It's WIP because I'll probably add a few more reflection functions.
>> > Thanks.
>>
>> Sorry about the belated review, looks like I missed this one.
>>
>> At a high level, it seems reasonable.
>>
>> Do you have a copyright assignment in place for GCC contributions?
>> See https://gcc.gnu.org/contribute.html
>>
>> [...]
>
>diff --git a/gcc/jit/docs/topics/compatibility.rst
>> > b/gcc/jit/docs/topics/compatibility.rst
>> > index bb3387fa583..7e786194ded 100644
>> > --- a/gcc/jit/docs/topics/compatibility.rst
>> > +++ b/gcc/jit/docs/topics/compatibility.rst
>> > @@ -219,3 +219,14 @@ entrypoints:
>> >    * :func:`gcc_jit_version_minor`
>> >
>> >    * :func:`gcc_jit_version_patchlevel`
>> > +
>> > +.. _LIBGCCJIT_ABI_14:
>> > +
>> > +``LIBGCCJIT_ABI_14``
>> > +--------------------
>> > +``LIBGCCJIT_ABI_14`` covers the addition of reflection functions
>> > via API
>> > +entrypoints:
>> > +
>> > +  * :func:`gcc_jit_function_get_return_type`
>> > +
>> > +  * :func:`gcc_jit_function_get_param_count`
>>
>> This will now need bumping to 15; 14 covers the addition of
>> gcc_jit_global_set_initializer.
>>
>> [...]
>>
>> > +/* Public entrypoint.  See description in libgccjit.h.
>> > +
>> > +   After error-checking, the real work is done by the
>> > +   gcc::jit::recording::function::get_return_type method, in
>> > +   jit-recording.h.  */
>> > +
>> > +gcc_jit_type *
>> > +gcc_jit_function_get_return_type (gcc_jit_function *func)
>> > +{
>>
>> This one is missing a:
>>   RETURN_NULL_IF_FAIL (func, NULL, NULL, "NULL function");
>>
>>
>> > +    return (gcc_jit_type *)func->get_return_type ();
>> > +}
>>
>> [...]
>>
>> > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
>> > index 1c5a12e9c01..6999ce25ca2 100644
>> > --- a/gcc/jit/libgccjit.h
>> > +++ b/gcc/jit/libgccjit.h
>>
>> [...]
>>
>> > @@ -1503,6 +1511,22 @@ 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 and return
>> > types of
>> > +   a function from the C API.
>>
>> "return type", is better grammar, I think, given that "function" is
>> singular.
>>
>> > +
>> > +   "vec_type" should be a vector type, created using
>> > gcc_jit_type_get_vector.
>>
>> This line about "vec_type" seems to be a leftover from a copy&paste.
>>
>>
>> > +   This API entrypoint was added in LIBGCCJIT_ABI_14; you can test
>> > for its
>> > +   presence using
>> > +     #ifdef LIBGCCJIT_HAVE_gcc_jit_function_reflection
>>
>> Version number will need bumping, as mentioned above.
>>
>> [...]
>>
>> > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
>> > index 6137dd4b4b0..b28f81a7a32 100644
>> > --- a/gcc/jit/libgccjit.map
>> > +++ b/gcc/jit/libgccjit.map
>> > @@ -186,4 +186,10 @@ LIBGCCJIT_ABI_13 {
>> >      gcc_jit_version_major;
>> >      gcc_jit_version_minor;
>> >      gcc_jit_version_patchlevel;
>> > -} LIBGCCJIT_ABI_12;
>> > \ No newline at end of file
>> > +} LIBGCCJIT_ABI_12;
>> > +
>> > +LIBGCCJIT_ABI_14 {
>> > +  global:
>> > +    gcc_jit_function_get_return_type;
>> > +    gcc_jit_function_get_param_count;
>> > +} LIBGCCJIT_ABI_13;
>>
>> Likewise.
>>
>> [...]
>>
>> Otherwise looks good to me.
>>
>> Bonus points for adding C++ bindings (and docs for them), but I don't
>> know of anyone using the C++ bindings.
>
>
>Also, please add "PR jit/96889" to the ChangeLog entries, and [PR96889]
>to the subject line.
>
>Dave
>

  parent reply	other threads:[~2020-10-03 18:14 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-02  1:01 [PATCH] libgccjit: add some reflection functions in the jit C api Antoni Boucher
2020-10-02 20:17 ` David Malcolm
2020-10-02 20:24   ` David Malcolm
2020-10-02 22:39     ` [PATCH] libgccjit: add some reflection functions in the jit C api [PR96889] Antoni Boucher
2020-10-03 18:14     ` Antoni Boucher [this message]
2020-10-15 16:02     ` [PATCH] libgccjit: add some reflection functions in the jit C api Antoni Boucher
2020-10-15 16:23       ` Andrea Corallo
2020-10-15 17:39         ` Antoni Boucher
2020-10-15 18:04           ` Andrea Corallo
2020-10-15 21:52           ` David Malcolm
2020-10-17  0:41             ` Antoni Boucher
2020-11-03 22:13             ` Antoni Boucher
2021-05-13  8:33               ` Martin Liška
2021-05-13 21:30               ` David Malcolm
2021-05-14  2:11                 ` Antoni Boucher
2021-05-26  0:19                 ` Antoni Boucher
2021-05-27 22:19                   ` David Malcolm
2021-05-28  1:51                     ` Antoni Boucher
2021-06-10 22:41                       ` David Malcolm
     [not found]                         ` <bc9e81dc3f0a68d6389c9765b5901a5dbd1dcd71.camel@zoho.com>
     [not found]                           ` <e962387aff72bce1ea29b0fc2cb04b84c26f9855.camel@redhat.com>
2021-06-18 15:55                             ` Antoni Boucher
2021-06-18 16:09                               ` David Malcolm
2021-06-18 19:41                                 ` Antoni Boucher
2021-06-18 20:37                                   ` David Malcolm
2021-07-19 16:10                                     ` Antoni Boucher
2021-07-29 12:59                                       ` Antoni Boucher
2021-08-31 12:34                                         ` Antoni Boucher
2021-09-28  0:53                                     ` Antoni Boucher
2021-10-13  2:09                                       ` Antoni Boucher
2021-10-23 21:20                                         ` SV: " Petter Tomner
2021-11-14 21:30                                         ` Antoni Boucher
2021-11-20  0:53                                       ` David Malcolm
2021-11-26 18:03                                         ` Gerald Pfeifer
2021-11-26 19:51                                           ` Gerald Pfeifer
2021-11-27 16:09                                           ` SV: " Petter Tomner
2021-12-02 18:04                                             ` Gerald Pfeifer

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=20201003181410.iggpsfrwlmu7gq5k@bouanto-desktop.localdomain \
    --to=bouanto@zoho.com \
    --cc=dmalcolm@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jit@gcc.gnu.org \
    /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).