From: Guillaume Gomez <guillaume1.gomez@gmail.com>
To: David Malcolm <dmalcolm@redhat.com>
Cc: jit@gcc.gnu.org, Antoni <bouanto@zoho.com>, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] Add support for function attributes and variable attributes
Date: Thu, 11 Jan 2024 22:40:35 +0100 [thread overview]
Message-ID: <CAAOQCfTz+1dhfwaLnNPgRfLcTFsb0yT4dyCnvkURhRghmTGrYw@mail.gmail.com> (raw)
In-Reply-To: <df47450d788eacae7ce60aa6c288a8e4ec62b9b2.camel@redhat.com>
Hi David,
> The above looks correct, but the patch adds the entrypoint descriptions
> to topics/types.rst, which seems like the wrong place. The function-
> related ones should be in topics/functions.rst in the "Functions"
> section and the lvalue/variable one in topics/expression.rst after the
> "Global variables" section.
Ah indeed. Mix-up on my end. Fixed it.
> test-restrict.c is a pre-existing testcase, so please don't delete its
> entry.
Ah indeed, I went too quickly and thought it was a test I renamed...
> BTW, the ChangeLog entry mentions adding test-restrict.c, but the patch
> doesn't add it, so that part of the proposed ChangeLog is wrong.
>
> Does the patch pass ./contrib/gcc-changelog/git_check_commit.py ?
I messed up a bit, fixed it thanks to you. I didn't run the script in my last
update but just did:
```
$ contrib/gcc-changelog/git_check_commit.py $(git log -1 --format=%h)
Checking 3849ee2eadf0eeec2b0080a5142ced00be96a60d: OK
```
> Otherwise, looks good, assuming that the patch has been tested with the
> full jit testsuite.
When rebasing on upstream yesterday I discovered that two tests
were not working anymore. For the first one, it was simply because of
the changes in `dummy-frontend.cc`. For the second one
(test-noinline-attribute.c), it was because the rules for inlining changed
since we wrote this patch apparently (our fork is very late). Antoni discovered
that we could just add a call to `asm` to prevent this from happening so I
added it.
So yes, all jit tests are passing as expected. :)
Le jeu. 11 janv. 2024 à 19:46, David Malcolm <dmalcolm@redhat.com> a écrit :
>
> On Thu, 2024-01-11 at 01:00 +0100, Guillaume Gomez wrote:
> > Hi David.
> >
> > Thanks for the review!
> >
> > > > +.. function:: void\
> > > > + gcc_jit_lvalue_add_string_attribute
> > > > (gcc_jit_lvalue *variable,
> > > > + enum
> > > > gcc_jit_fn_attribute attribute,
> > >
> > > ^^
> > >
> > > This got out of sync with the declaration in the header file; it
> > > should
> > > be enum gcc_jit_variable_attribute attribute
> >
> > Indeed, good catch!
> >
> > > I took a brief look through the handler functions and with the
> > > above
> > > caveat I didn't see anything obviously wrong. I'm going to assume
> > > this
> > > code is OK given that presumably you've been testing it within
> > > rustc,
> > > right?
> >
> > Both in rustc and in the JIT tests we added.
> >
> > [..snip...]
> >
> > I added all the missing `RETURN_IF_FAIL` you mentioned. None of the
> > arguments should be `NULL` so it was a mistake not to check it.
> >
> > [..snip...]
> >
> > I removed the tests comments as you mentioned.
> >
> > > Please update jit.dg/all-non-failing-tests.h for the new tests;
> > > it's
> > > meant to list all of the (non failing) tests alphabetically.
> >
> > It's not always correctly sorted. Might be worth sending a patch
> > after this
> > one gets merged to fix that.
> >
> > > I *think* all of the new tests aren't suitable to be run as part of
> > > a
> > > shared context (e.g. due to touching the optimization level or
> > > examining generated asm), so they should be listed in that header
> > > with
> > > comments explaining why.
> >
> > I added them with a comment on top of each of them.
> >
> > I joined the new patch version.
> >
> > Thanks again for the review!
>
> Thanks for the updated patch. I noticed a few minor issues:
>
> > diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.rst
> > index bb51f037b7e..b1aedc03787 100644
> > --- a/gcc/jit/docs/topics/types.rst
> > +++ b/gcc/jit/docs/topics/types.rst
> > @@ -553,3 +553,80 @@ Reflection API
> > .. code-block:: c
> >
> > #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > +
> > +.. function:: void\
> > + gcc_jit_function_add_attribute (gcc_jit_function *func,
> > + enum gcc_jit_fn_attribute attribute)
> > +
> > + Add an attribute ``attribute`` to a function ``func``.
> > +
> > + This is equivalent to the following code:
> > +
> > + .. code-block:: c
> > +
> > + __attribute__((always_inline))
> > +
> > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test for
> > + its presence using
> > +
> > + .. code-block:: c
> > +
> > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES
> > +
> > +.. function:: void\
> > + gcc_jit_function_add_string_attribute (gcc_jit_function *func,
> > + enum gcc_jit_fn_attribute attribute,
> > + const char *value)
> > +
> > + Add a string attribute ``attribute`` with value ``value`` to a function
> > + ``func``.
> > +
> > + This is equivalent to the following code:
> > +
> > + .. code-block:: c
> > +
> > + __attribute__ ((alias ("xxx")))
> > +
> > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test for
> > + its presence using
> > +
> > + .. code-block:: c
> > +
> > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES
> > +
> > +.. function:: void\
> > + gcc_jit_function_add_integer_array_attribute (gcc_jit_function *func,
> > + enum gcc_jit_fn_attribute attribute,
> > + const int *value,
> > + size_t length)
> > +
> > + Add an attribute ``attribute`` with ``length`` integer values ``value`` to a
> > + function ``func``. The integer values must be the same as you would write
> > + them in a C code.
> > +
> > + This is equivalent to the following code:
> > +
> > + .. code-block:: c
> > +
> > + __attribute__ ((nonnull (1, 2)))
> > +
> > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test for
> > + its presence using
> > +
> > + .. code-block:: c
> > +
> > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES
> > +
> > +.. function:: void\
> > + gcc_jit_lvalue_add_string_attribute (gcc_jit_lvalue *variable,
> > + enum gcc_jit_variable_attribute attribute,
> > + const char *value)
> > +
> > + Add an attribute ``attribute`` with value ``value`` to a variable ``variable``.
> > +
> > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test for
> > + its presence using
> > +
> > + .. code-block:: c
> > +
> > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES
>
> The above looks correct, but the patch adds the entrypoint descriptions
> to topics/types.rst, which seems like the wrong place. The function-
> related ones should be in topics/functions.rst in the "Functions"
> section and the lvalue/variable one in topics/expression.rst after the
> "Global variables" section.
>
> > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > index e762563f9bd..84001203352 100644
> > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
>
> [...snip...]
>
> > @@ -313,7 +334,7 @@
> > #undef create_code
> > #undef verify_code
> >
> > -/* test-restrict.c: This can't be in the testcases array as it needs
> > +/* test-restrict-attribute.c: This can't be in the testcases array as it needs
> > the `-O3` flag. */
>
> test-restrict.c is a pre-existing testcase, so please don't delete its
> entry.
> BTW, the ChangeLog entry mentions adding test-restrict.c, but the patch
> doesn't add it, so that part of the proposed ChangeLog is wrong.
>
> Does the patch pass ./contrib/gcc-changelog/git_check_commit.py ?
>
> [...snip...]
>
> > diff --git a/gcc/testsuite/jit.dg/test-cold-attribute.c b/gcc/testsuite/jit.dg/test-cold-attribute.c
> > new file mode 100644
> > index 00000000000..8dc7ec5a34b
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-cold-attribute.c
> > @@ -0,0 +1,54 @@
> > +/* { dg-do compile { target x86_64-*-* } } */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include "libgccjit.h"
> > +
> > +/* We don't want set_options() in harness.h to set -O2 to see that the cold
> > + attribute affects the optimizations. */
>
> Please delete the above comment.
>
> > +#define TEST_ESCHEWS_SET_OPTIONS
> > +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> > +{
> > + // Set "-O2".
> > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 2);
> > +}
>
> [...snip...]
>
> > diff --git a/gcc/testsuite/jit.dg/test-const-attribute.c b/gcc/testsuite/jit.dg/test-const-attribute.c
> > new file mode 100644
> > index 00000000000..c06742d163f
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-const-attribute.c
> > @@ -0,0 +1,134 @@
> > +/* { dg-do compile { target x86_64-*-* } } */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include "libgccjit.h"
> > +
> > +/* We don't want set_options() in harness.h to set -O3 to see that the const
> > + attribute affects the optimizations. */
>
> Please delete the above comment.
>
> > +#define TEST_ESCHEWS_SET_OPTIONS
> > +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> > +{
> > + // Set "-O3".
> > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > +}
>
> [...snip...]
>
> > diff --git a/gcc/testsuite/jit.dg/test-noinline-attribute.c b/gcc/testsuite/jit.dg/test-noinline-attribute.c
> > new file mode 100644
> > index 00000000000..a455b4493fd
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-noinline-attribute.c
> > @@ -0,0 +1,121 @@
> > +/* { dg-do compile { target x86_64-*-* } } */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include "libgccjit.h"
> > +
> > +/* We don't want set_options() in harness.h to set -O2 to see that the `noinline`
> > + attribute affects the optimizations. */
>
> Please delete the above comment.
>
> > +#define TEST_ESCHEWS_SET_OPTIONS
> > +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> > +{
> > + // Set "-O2".
> > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 2);
> > +}
>
> [...snip...]
>
> > diff --git a/gcc/testsuite/jit.dg/test-nonnull-attribute.c b/gcc/testsuite/jit.dg/test-nonnull-attribute.c
> > new file mode 100644
> > index 00000000000..3306f890657
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-nonnull-attribute.c
> > @@ -0,0 +1,94 @@
> > +/* { dg-do compile { target x86_64-*-* } } */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include "libgccjit.h"
> > +
> > +/* We don't want set_options() in harness.h to set -O2 to see that the nonnull
> > + attribute affects the optimizations. */
>
> Please delete the above comment.
>
>
> > +#define TEST_ESCHEWS_SET_OPTIONS
> > +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> > +{
> > + // Set "-O2".
> > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 2);
> > +}
>
> [...snip...]
>
> > diff --git a/gcc/testsuite/jit.dg/test-pure-attribute.c b/gcc/testsuite/jit.dg/test-pure-attribute.c
> > new file mode 100644
> > index 00000000000..0c9ba1366e0
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-pure-attribute.c
> > @@ -0,0 +1,134 @@
> > +/* { dg-do compile { target x86_64-*-* } } */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include "libgccjit.h"
> > +
> > +/* We don't want set_options() in harness.h to set -O3 to see that the pure
> > + attribute affects the optimizations. */
>
> Please delete the above comment.
>
> > +#define TEST_ESCHEWS_SET_OPTIONS
> > +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> > +{
> > + // Set "-O3".
> > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > +}
> > +
>
> [...snip...]
>
> > diff --git a/gcc/testsuite/jit.dg/test-restrict-attribute.c b/gcc/testsuite/jit.dg/test-restrict-attribute.c
> > new file mode 100644
> > index 00000000000..7d7444b624f
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-restrict-attribute.c
> > @@ -0,0 +1,77 @@
> > +/* { dg-do compile { target x86_64-*-* } } */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include "libgccjit.h"
> > +
> > +/* We don't want set_options() in harness.h to set -O3 to see that the restrict
> > + attribute affects the optimizations. */
>
> Please delete this comment.
>
> > +#define TEST_ESCHEWS_SET_OPTIONS
> > +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> > +{
> > + // Set "-O3".
> > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > +}
> > +
>
> [...snip...]
>
> Otherwise, looks good, assuming that the patch has been tested with the
> full jit testsuite.
>
> Thanks again
> Dave
>
next prev parent reply other threads:[~2024-01-11 21:40 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-15 16:53 Guillaume Gomez
2023-11-15 16:56 ` Antoni Boucher
2023-11-23 21:52 ` Guillaume Gomez
2023-11-23 21:59 ` Antoni Boucher
2023-11-30 9:55 ` Guillaume Gomez
2023-12-07 17:13 ` Antoni Boucher
2023-12-09 11:12 ` Guillaume Gomez
2023-12-18 22:27 ` Guillaume Gomez
2024-01-03 13:37 ` Guillaume Gomez
2024-01-09 19:59 ` David Malcolm
2024-01-11 0:00 ` Guillaume Gomez
2024-01-11 18:46 ` David Malcolm
2024-01-11 21:40 ` Guillaume Gomez [this message]
2024-01-11 22:38 ` David Malcolm
2024-01-12 10:09 ` Guillaume Gomez
2024-01-12 13:47 ` Guillaume Gomez
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=CAAOQCfTz+1dhfwaLnNPgRfLcTFsb0yT4dyCnvkURhRghmTGrYw@mail.gmail.com \
--to=guillaume1.gomez@gmail.com \
--cc=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).