Re: [PATCH] Add support for function attributes and variable attributes
Just realized that you were asking for the patch I forgot to join... Here it is. Le ven. 12 janv. 2024 à 11:09, Guillaume Gomez a écrit : > > > It sounds like the patch you have locally is ready, but it has some > > nontrivial changes compared to the last version you posted to the list. > > Please post your latest version to the list. > > Sure! > > This patch adds the support for attributes on functions and variables. It does > so by adding the following functions: > > * gcc_jit_function_add_attribute > * gcc_jit_function_add_string_attribute > * gcc_jit_function_add_integer_array_attribute > * gcc_jit_lvalue_add_string_attribute > > It adds the following types: > > * gcc_jit_fn_attribute > * gcc_jit_variable_attribute > > It adds tests to ensure that the attributes are correctly applied. > > > Do you have push rights, or do you need me to push it for you? > > I have push rights so I'll merge the patch myself. But thanks for offering to > do it. > > Le jeu. 11 janv. 2024 à 23:38, David Malcolm a écrit : > > > > On Thu, 2024-01-11 at 22:40 +0100, Guillaume Gomez wrote: > > > 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. :) > > > > Good. > > > > It sounds like the patch you have locally is ready, but it has some > > nontrivial changes compared to the last version you posted to the list. > > Please post your latest version to the list. > > > > Do you have push rights, or do you need me to push it for you? > > > > Thanks > > Dave > > > > > > > > Le jeu. 11 janv. 2024 à 19:46, David Malcolm 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. > > > > > >
Re: [PATCH] Add support for function attributes and variable attributes
> It sounds like the patch you have locally is ready, but it has some > nontrivial changes compared to the last version you posted to the list. > Please post your latest version to the list. Sure! This patch adds the support for attributes on functions and variables. It does so by adding the following functions: * gcc_jit_function_add_attribute * gcc_jit_function_add_string_attribute * gcc_jit_function_add_integer_array_attribute * gcc_jit_lvalue_add_string_attribute It adds the following types: * gcc_jit_fn_attribute * gcc_jit_variable_attribute It adds tests to ensure that the attributes are correctly applied. > Do you have push rights, or do you need me to push it for you? I have push rights so I'll merge the patch myself. But thanks for offering to do it. Le jeu. 11 janv. 2024 à 23:38, David Malcolm a écrit : > > On Thu, 2024-01-11 at 22:40 +0100, Guillaume Gomez wrote: > > 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. :) > > Good. > > It sounds like the patch you have locally is ready, but it has some > nontrivial changes compared to the last version you posted to the list. > Please post your latest version to the list. > > Do you have push rights, or do you need me to push it for you? > > Thanks > Dave > > > > > Le jeu. 11 janv. 2024 à 19:46, David Malcolm 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
Re: [PATCH] Add support for function attributes and variable attributes
On Thu, 2024-01-11 at 22:40 +0100, Guillaume Gomez wrote: > 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. :) Good. It sounds like the patch you have locally is ready, but it has some nontrivial changes compared to the last version you posted to the list. Please post your latest version to the list. Do you have push rights, or do you need me to push it for you? Thanks Dave > > Le jeu. 11 janv. 2024 à 19:46, David Malcolm 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 > > > + > > > + .. cod
Re: [PATCH] Add support for function attributes and variable attributes
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 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
> > +
Re: [PATCH] Add support for function attributes and variable attributes
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
Re: [PATCH] Add support for function attributes and variable attributes
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!
Le mar. 9 janv. 2024 à 20:59, David Malcolm a écrit :
>
> On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote:
> > Hi,
> >
> > This patch adds the (incomplete) support for function and variable
> > attributes. The added attributes are the ones we're using in
> > rustc_codegen_gcc but all the groundwork is done to add more (and we
> > will very likely add more as we didn't add all the ones we use in
> > rustc_codegen_gcc yet).
> >
> > The only big question with this patch is about `inline`. We currently
> > handle it as an attribute because it is more convenient for us but is
> > it ok or should we create a separate function to mark a function as
> > inlined?
> >
> > Thanks in advance for the review.
>
> Thanks for the patch; sorry for the delay in reviewing.
>
> At a high-level I think the API is OK as-is, but I have some nitpicks
> with the implementation:
>
> [...snip...]
>
> > diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.rst
> > index d8c1d15d69d..6c72c99cbd9 100644
> > --- a/gcc/jit/docs/topics/types.rst
> > +++ b/gcc/jit/docs/topics/types.rst
>
> [...snip...]
>
> > +.. 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
>
> [...snip...]
>
> > diff --git a/gcc/jit/dummy-frontend.cc b/gcc/jit/dummy-frontend.cc
> > index a729086bafb..898b4d6e7f8 100644
> > --- a/gcc/jit/dummy-frontend.cc
> > +++ b/gcc/jit/dummy-frontend.cc
>
> It's unfortunate that jit/dummy-frontend.cc has its own copy of the
> material in c-common/c-attribs.cc. I glanced through this code, and it
> seems that there are already various differences between the two copies
> in the existing code, and the patch adds more such differences.
>
> Bother - but I think this part of the patch is inevitable (and OK)
> given the existing state of attribute handling here.
>
> [...snip...]
>
> 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 rustcc,
> right?
>
> [..snip...]
>
> > diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
> > index 0451b4df7f9..337d4ea3b95 100644
> > --- a/gcc/jit/libgccjit.cc
> > +++ b/gcc/jit/libgccjit.cc
> > @@ -3965,6 +3965,51 @@ gcc_jit_type_get_aligned (gcc_jit_type *type,
> >return (gcc_jit_type *)type->get_aligned (alignment_in_bytes);
> > }
> >
> > +void
> > +gcc_jit_function_add_attribute (gcc_jit_function *func,
> > + gcc_jit_fn_attribute attribute)
> > +{
> > + RETURN_IF_FAIL (func, NULL, NULL, "NULL func");
> > +
> > + func->add_attribute (attribute);
>
> Ideally should validate parameter "attribute" here with a
> RETURN_IF_FAIL.
>
> > +}
> > +
> > +void
> > +gcc_jit_function_add_string_attribute (gcc_jit_function *func,
> > +gcc_jit_fn_attribute attribute,
> > +const char* value)
> > +{
> > + RETURN_IF_FAIL (func, NULL, NULL, "NULL func");
>
> Likewise, ideally should validate parameter "attribute" here with a
> RETURN_IF_FAIL.
>
> Can "value" be NULL?
Re: [PATCH] Add support for function attributes and variable attributes
On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote:
> Hi,
>
> This patch adds the (incomplete) support for function and variable
> attributes. The added attributes are the ones we're using in
> rustc_codegen_gcc but all the groundwork is done to add more (and we
> will very likely add more as we didn't add all the ones we use in
> rustc_codegen_gcc yet).
>
> The only big question with this patch is about `inline`. We currently
> handle it as an attribute because it is more convenient for us but is
> it ok or should we create a separate function to mark a function as
> inlined?
>
> Thanks in advance for the review.
Thanks for the patch; sorry for the delay in reviewing.
At a high-level I think the API is OK as-is, but I have some nitpicks
with the implementation:
[...snip...]
> diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.rst
> index d8c1d15d69d..6c72c99cbd9 100644
> --- a/gcc/jit/docs/topics/types.rst
> +++ b/gcc/jit/docs/topics/types.rst
[...snip...]
> +.. 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
[...snip...]
> diff --git a/gcc/jit/dummy-frontend.cc b/gcc/jit/dummy-frontend.cc
> index a729086bafb..898b4d6e7f8 100644
> --- a/gcc/jit/dummy-frontend.cc
> +++ b/gcc/jit/dummy-frontend.cc
It's unfortunate that jit/dummy-frontend.cc has its own copy of the
material in c-common/c-attribs.cc. I glanced through this code, and it
seems that there are already various differences between the two copies
in the existing code, and the patch adds more such differences.
Bother - but I think this part of the patch is inevitable (and OK)
given the existing state of attribute handling here.
[...snip...]
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 rustcc,
right?
[..snip...]
> diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
> index 0451b4df7f9..337d4ea3b95 100644
> --- a/gcc/jit/libgccjit.cc
> +++ b/gcc/jit/libgccjit.cc
> @@ -3965,6 +3965,51 @@ gcc_jit_type_get_aligned (gcc_jit_type *type,
>return (gcc_jit_type *)type->get_aligned (alignment_in_bytes);
> }
>
> +void
> +gcc_jit_function_add_attribute (gcc_jit_function *func,
> + gcc_jit_fn_attribute attribute)
> +{
> + RETURN_IF_FAIL (func, NULL, NULL, "NULL func");
> +
> + func->add_attribute (attribute);
Ideally should validate parameter "attribute" here with a
RETURN_IF_FAIL.
> +}
> +
> +void
> +gcc_jit_function_add_string_attribute (gcc_jit_function *func,
> +gcc_jit_fn_attribute attribute,
> +const char* value)
> +{
> + RETURN_IF_FAIL (func, NULL, NULL, "NULL func");
Likewise, ideally should validate parameter "attribute" here with a
RETURN_IF_FAIL.
Can "value" be NULL? If not, then we should add a RETURN_IF_FAIL for
it here at the API boundary.
> +
> + func->add_string_attribute (attribute, value);
> +}
> +
> +/* This function adds an attribute with multiple integer values. For example
> + `nonnull(1, 2)`. The numbers in `values` are supposed to map how they
> + should be written in C code. So for `nonnull(1, 2)`, you should pass `1`
> + and `2` in `values` (and set `length` to `2`). */
> +void
> +gcc_jit_function_add_integer_array_attribute (gcc_jit_function *func,
> + gcc_jit_fn_attribute attribute,
> + const int* values,
> + size_t length)
> +{
> + RETURN_IF_FAIL (func, NULL, NULL, "NULL func");
As before, ideally should validate parameter "attribute" here with a
RETURN_IF_FAIL.
> + RETURN_IF_FAIL (values, NULL, NULL, "NULL values");
> +
> + func->add_integer_array_attribute (attribute, values, length);
> +}
> +
> +void
> +gcc_jit_lvalue_add_string_attribute (gcc_jit_lvalue *variable,
> + gcc_jit_variable_attribute attribute,
> + const char* value)
> +{
> + RETURN_IF_FAIL (variable, NULL, NULL, "NULL variable");
As before, we should validate parameters "attribute" and "value" here
with RETURN_IF_FAILs.
We should also validate here that "variable" is indeed a variable, not
some arbitrary lvalue e.g. the address of the element of an array (or
whatever).
> +
> + variable->add_string_attribute (attribute, value);
> +}
> +
[...snip...]
> diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp
> index 8bf7e51c24f..657b454a003 100644
> --- a/gcc/testsuite/jit.dg/jit.exp
> +++ b/gcc
Re: [PATCH] Add support for function attributes and variable attributes
Ping David. :) Le lun. 18 déc. 2023 à 23:27, Guillaume Gomez a écrit : > > Ping David. :) > > Le sam. 9 déc. 2023 à 12:12, Guillaume Gomez > a écrit : > > > > Added it. > > > > Le jeu. 7 déc. 2023 à 18:13, Antoni Boucher a écrit : > > > > > > It seems like you forgot to prefix the commit message with "libgccjit: > > > ". > > > > > > On Thu, 2023-11-30 at 10:55 +0100, Guillaume Gomez wrote: > > > > Ping David. :) > > > > > > > > Le jeu. 23 nov. 2023 à 22:59, Antoni Boucher a > > > > écrit : > > > > > David: I found back the comment you made. Here it is: > > > > > > > > > >I see you have patches to add function and variable attributes; > > > > > I > > > > >wonder if this would be cleaner internally if there was a > > > > >recording::attribute class, rather than the std::pair currently > > > > > in > > > > >use > > > > >(some attributes have int arguments rather than string, others > > > > > have > > > > >multiple args). > > > > > > > > > >I also wondered if a "gcc_jit_attribute" type could be exposed > > > > > to > > > > >the > > > > >user, e.g.: > > > > > > > > > > attr1 = gcc_jit_context_new_attribute (ctxt, "noreturn"); > > > > > attr2 = gcc_jit_context_new_attribute_with_string (ctxt, > > > > > "alias", > > > > >"__foo"); > > > > > gcc_jit_function_add_attribute (ctxt, attr1); > > > > > gcc_jit_function_add_attribute (ctxt, attr2); > > > > > > > > > >or somesuch? But I think the API you currently have is OK. > > > > > > > > > > On Thu, 2023-11-23 at 22:52 +0100, Guillaume Gomez wrote: > > > > > > Ping David. :) > > > > > > > > > > > > Le mer. 15 nov. 2023 à 17:56, Antoni Boucher a > > > > > > écrit : > > > > > > > > > > > > > > David: another thing I remember you mentioned when you reviewed > > > > > > > an > > > > > > > earlier version of this patch is the usage of `std::pair`. > > > > > > > I can't find where you said that, but I remember you mentioned > > > > > > > that > > > > > > > we > > > > > > > should use a struct instead. > > > > > > > Can you please elaborate again? > > > > > > > Thanks. > > > > > > > > > > > > > > On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > This patch adds the (incomplete) support for function and > > > > > > > > variable > > > > > > > > attributes. The added attributes are the ones we're using in > > > > > > > > rustc_codegen_gcc but all the groundwork is done to add more > > > > > > > > (and > > > > > > > > we > > > > > > > > will very likely add more as we didn't add all the ones we > > > > > > > > use in > > > > > > > > rustc_codegen_gcc yet). > > > > > > > > > > > > > > > > The only big question with this patch is about `inline`. We > > > > > > > > currently > > > > > > > > handle it as an attribute because it is more convenient for > > > > > > > > us > > > > > > > > but is > > > > > > > > it ok or should we create a separate function to mark a > > > > > > > > function > > > > > > > > as > > > > > > > > inlined? > > > > > > > > > > > > > > > > Thanks in advance for the review. > > > > > > > > > > > > > > >
Re: [PATCH] Add support for function attributes and variable attributes
Ping David. :) Le sam. 9 déc. 2023 à 12:12, Guillaume Gomez a écrit : > > Added it. > > Le jeu. 7 déc. 2023 à 18:13, Antoni Boucher a écrit : > > > > It seems like you forgot to prefix the commit message with "libgccjit: > > ". > > > > On Thu, 2023-11-30 at 10:55 +0100, Guillaume Gomez wrote: > > > Ping David. :) > > > > > > Le jeu. 23 nov. 2023 à 22:59, Antoni Boucher a > > > écrit : > > > > David: I found back the comment you made. Here it is: > > > > > > > >I see you have patches to add function and variable attributes; > > > > I > > > >wonder if this would be cleaner internally if there was a > > > >recording::attribute class, rather than the std::pair currently > > > > in > > > >use > > > >(some attributes have int arguments rather than string, others > > > > have > > > >multiple args). > > > > > > > >I also wondered if a "gcc_jit_attribute" type could be exposed > > > > to > > > >the > > > >user, e.g.: > > > > > > > > attr1 = gcc_jit_context_new_attribute (ctxt, "noreturn"); > > > > attr2 = gcc_jit_context_new_attribute_with_string (ctxt, > > > > "alias", > > > >"__foo"); > > > > gcc_jit_function_add_attribute (ctxt, attr1); > > > > gcc_jit_function_add_attribute (ctxt, attr2); > > > > > > > >or somesuch? But I think the API you currently have is OK. > > > > > > > > On Thu, 2023-11-23 at 22:52 +0100, Guillaume Gomez wrote: > > > > > Ping David. :) > > > > > > > > > > Le mer. 15 nov. 2023 à 17:56, Antoni Boucher a > > > > > écrit : > > > > > > > > > > > > David: another thing I remember you mentioned when you reviewed > > > > > > an > > > > > > earlier version of this patch is the usage of `std::pair`. > > > > > > I can't find where you said that, but I remember you mentioned > > > > > > that > > > > > > we > > > > > > should use a struct instead. > > > > > > Can you please elaborate again? > > > > > > Thanks. > > > > > > > > > > > > On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote: > > > > > > > Hi, > > > > > > > > > > > > > > This patch adds the (incomplete) support for function and > > > > > > > variable > > > > > > > attributes. The added attributes are the ones we're using in > > > > > > > rustc_codegen_gcc but all the groundwork is done to add more > > > > > > > (and > > > > > > > we > > > > > > > will very likely add more as we didn't add all the ones we > > > > > > > use in > > > > > > > rustc_codegen_gcc yet). > > > > > > > > > > > > > > The only big question with this patch is about `inline`. We > > > > > > > currently > > > > > > > handle it as an attribute because it is more convenient for > > > > > > > us > > > > > > > but is > > > > > > > it ok or should we create a separate function to mark a > > > > > > > function > > > > > > > as > > > > > > > inlined? > > > > > > > > > > > > > > Thanks in advance for the review. > > > > > > > > > > > >
Re: [PATCH] Add support for function attributes and variable attributes
Added it. Le jeu. 7 déc. 2023 à 18:13, Antoni Boucher a écrit : > > It seems like you forgot to prefix the commit message with "libgccjit: > ". > > On Thu, 2023-11-30 at 10:55 +0100, Guillaume Gomez wrote: > > Ping David. :) > > > > Le jeu. 23 nov. 2023 à 22:59, Antoni Boucher a > > écrit : > > > David: I found back the comment you made. Here it is: > > > > > >I see you have patches to add function and variable attributes; > > > I > > >wonder if this would be cleaner internally if there was a > > >recording::attribute class, rather than the std::pair currently > > > in > > >use > > >(some attributes have int arguments rather than string, others > > > have > > >multiple args). > > > > > >I also wondered if a "gcc_jit_attribute" type could be exposed > > > to > > >the > > >user, e.g.: > > > > > > attr1 = gcc_jit_context_new_attribute (ctxt, "noreturn"); > > > attr2 = gcc_jit_context_new_attribute_with_string (ctxt, > > > "alias", > > >"__foo"); > > > gcc_jit_function_add_attribute (ctxt, attr1); > > > gcc_jit_function_add_attribute (ctxt, attr2); > > > > > >or somesuch? But I think the API you currently have is OK. > > > > > > On Thu, 2023-11-23 at 22:52 +0100, Guillaume Gomez wrote: > > > > Ping David. :) > > > > > > > > Le mer. 15 nov. 2023 à 17:56, Antoni Boucher a > > > > écrit : > > > > > > > > > > David: another thing I remember you mentioned when you reviewed > > > > > an > > > > > earlier version of this patch is the usage of `std::pair`. > > > > > I can't find where you said that, but I remember you mentioned > > > > > that > > > > > we > > > > > should use a struct instead. > > > > > Can you please elaborate again? > > > > > Thanks. > > > > > > > > > > On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote: > > > > > > Hi, > > > > > > > > > > > > This patch adds the (incomplete) support for function and > > > > > > variable > > > > > > attributes. The added attributes are the ones we're using in > > > > > > rustc_codegen_gcc but all the groundwork is done to add more > > > > > > (and > > > > > > we > > > > > > will very likely add more as we didn't add all the ones we > > > > > > use in > > > > > > rustc_codegen_gcc yet). > > > > > > > > > > > > The only big question with this patch is about `inline`. We > > > > > > currently > > > > > > handle it as an attribute because it is more convenient for > > > > > > us > > > > > > but is > > > > > > it ok or should we create a separate function to mark a > > > > > > function > > > > > > as > > > > > > inlined? > > > > > > > > > > > > Thanks in advance for the review. > > > > > > > > > From df75f0eb8aacba249b6e791603752e35778951a4 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 20 Jun 2022 14:34:39 -0400 Subject: [PATCH] libgccjit: Add support for function attributes and variable attributes. gcc/jit/ChangeLog: * dummy-frontend.cc (handle_alias_attribute): New function. (handle_always_inline_attribute): New function. (handle_cold_attribute): New function. (handle_fnspec_attribute): New function. (handle_format_arg_attribute): New function. (handle_format_attribute): New function. (handle_noinline_attribute): New function. (handle_target_attribute): New function. (handle_used_attribute): New function. (handle_visibility_attribute): New function. (handle_weak_attribute): New function. (handle_alias_ifunc_attribute): New function. * jit-playback.cc (fn_attribute_to_string): New function. (variable_attribute_to_string): New function. (global_new_decl): Add attributes support. (set_variable_attribute): New function. (new_global): Add attributes support. (new_global_initialized): Add attributes support. (new_local): Add attributes support. * jit-playback.h (fn_attribute_to_string): New function. (set_variable_attribute): New function. * jit-recording.cc (recording::lvalue::add_attribute): New function. (recording::function::function): New function. (recording::function::write_to_dump): Add attributes support. (recording::function::add_attribute): New function. (recording::function::add_string_attribute): New function. (recording::function::add_integer_array_attribute): New function. (recording::global::replay_into): Add attributes support. (recording::local::replay_into): Add attributes support. * libgccjit.cc (gcc_jit_function_add_attribute): New function. (gcc_jit_function_add_string_attribute): New function. (gcc_jit_function_add_integer_array_attribute): New function. (gcc_jit_lvalue_add_attribute): New function. * libgccjit.h (enum gcc_jit_fn_attribute): New enum. (gcc_jit_function_add_attribute): New function. (gcc_jit_function_add_string_attribute): New function. (gcc_jit_function_add_integer_array_attribute): New function. (enum gcc_jit_variable_attribute): New function. (gcc_jit_lvalue_add_string_attribute): New function. * libgccjit.map: Declare new functions. gcc/testsuite/ChangeLog: * jit.dg/jit.exp: Add `jit-verify-assembler-
Re: [PATCH] Add support for function attributes and variable attributes
It seems like you forgot to prefix the commit message with "libgccjit: ". On Thu, 2023-11-30 at 10:55 +0100, Guillaume Gomez wrote: > Ping David. :) > > Le jeu. 23 nov. 2023 à 22:59, Antoni Boucher a > écrit : > > David: I found back the comment you made. Here it is: > > > > I see you have patches to add function and variable attributes; > > I > > wonder if this would be cleaner internally if there was a > > recording::attribute class, rather than the std::pair currently > > in > > use > > (some attributes have int arguments rather than string, others > > have > > multiple args). > > > > I also wondered if a "gcc_jit_attribute" type could be exposed > > to > > the > > user, e.g.: > > > > attr1 = gcc_jit_context_new_attribute (ctxt, "noreturn"); > > attr2 = gcc_jit_context_new_attribute_with_string (ctxt, > > "alias", > > "__foo"); > > gcc_jit_function_add_attribute (ctxt, attr1); > > gcc_jit_function_add_attribute (ctxt, attr2); > > > > or somesuch? But I think the API you currently have is OK. > > > > On Thu, 2023-11-23 at 22:52 +0100, Guillaume Gomez wrote: > > > Ping David. :) > > > > > > Le mer. 15 nov. 2023 à 17:56, Antoni Boucher a > > > écrit : > > > > > > > > David: another thing I remember you mentioned when you reviewed > > > > an > > > > earlier version of this patch is the usage of `std::pair`. > > > > I can't find where you said that, but I remember you mentioned > > > > that > > > > we > > > > should use a struct instead. > > > > Can you please elaborate again? > > > > Thanks. > > > > > > > > On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote: > > > > > Hi, > > > > > > > > > > This patch adds the (incomplete) support for function and > > > > > variable > > > > > attributes. The added attributes are the ones we're using in > > > > > rustc_codegen_gcc but all the groundwork is done to add more > > > > > (and > > > > > we > > > > > will very likely add more as we didn't add all the ones we > > > > > use in > > > > > rustc_codegen_gcc yet). > > > > > > > > > > The only big question with this patch is about `inline`. We > > > > > currently > > > > > handle it as an attribute because it is more convenient for > > > > > us > > > > > but is > > > > > it ok or should we create a separate function to mark a > > > > > function > > > > > as > > > > > inlined? > > > > > > > > > > Thanks in advance for the review. > > > > > >
Re: [PATCH] Add support for function attributes and variable attributes
Ping David. :) Le jeu. 23 nov. 2023 à 22:59, Antoni Boucher a écrit : > David: I found back the comment you made. Here it is: > >I see you have patches to add function and variable attributes; I >wonder if this would be cleaner internally if there was a >recording::attribute class, rather than the std::pair currently in >use >(some attributes have int arguments rather than string, others have >multiple args). > >I also wondered if a "gcc_jit_attribute" type could be exposed to >the >user, e.g.: > > attr1 = gcc_jit_context_new_attribute (ctxt, "noreturn"); > attr2 = gcc_jit_context_new_attribute_with_string (ctxt, "alias", >"__foo"); > gcc_jit_function_add_attribute (ctxt, attr1); > gcc_jit_function_add_attribute (ctxt, attr2); > >or somesuch? But I think the API you currently have is OK. > > On Thu, 2023-11-23 at 22:52 +0100, Guillaume Gomez wrote: > > Ping David. :) > > > > Le mer. 15 nov. 2023 à 17:56, Antoni Boucher a > > écrit : > > > > > > David: another thing I remember you mentioned when you reviewed an > > > earlier version of this patch is the usage of `std::pair`. > > > I can't find where you said that, but I remember you mentioned that > > > we > > > should use a struct instead. > > > Can you please elaborate again? > > > Thanks. > > > > > > On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote: > > > > Hi, > > > > > > > > This patch adds the (incomplete) support for function and > > > > variable > > > > attributes. The added attributes are the ones we're using in > > > > rustc_codegen_gcc but all the groundwork is done to add more (and > > > > we > > > > will very likely add more as we didn't add all the ones we use in > > > > rustc_codegen_gcc yet). > > > > > > > > The only big question with this patch is about `inline`. We > > > > currently > > > > handle it as an attribute because it is more convenient for us > > > > but is > > > > it ok or should we create a separate function to mark a function > > > > as > > > > inlined? > > > > > > > > Thanks in advance for the review. > > > > >
Re: [PATCH] Add support for function attributes and variable attributes
David: I found back the comment you made. Here it is: I see you have patches to add function and variable attributes; I wonder if this would be cleaner internally if there was a recording::attribute class, rather than the std::pair currently in use (some attributes have int arguments rather than string, others have multiple args). I also wondered if a "gcc_jit_attribute" type could be exposed to the user, e.g.: attr1 = gcc_jit_context_new_attribute (ctxt, "noreturn"); attr2 = gcc_jit_context_new_attribute_with_string (ctxt, "alias", "__foo"); gcc_jit_function_add_attribute (ctxt, attr1); gcc_jit_function_add_attribute (ctxt, attr2); or somesuch? But I think the API you currently have is OK. On Thu, 2023-11-23 at 22:52 +0100, Guillaume Gomez wrote: > Ping David. :) > > Le mer. 15 nov. 2023 à 17:56, Antoni Boucher a > écrit : > > > > David: another thing I remember you mentioned when you reviewed an > > earlier version of this patch is the usage of `std::pair`. > > I can't find where you said that, but I remember you mentioned that > > we > > should use a struct instead. > > Can you please elaborate again? > > Thanks. > > > > On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote: > > > Hi, > > > > > > This patch adds the (incomplete) support for function and > > > variable > > > attributes. The added attributes are the ones we're using in > > > rustc_codegen_gcc but all the groundwork is done to add more (and > > > we > > > will very likely add more as we didn't add all the ones we use in > > > rustc_codegen_gcc yet). > > > > > > The only big question with this patch is about `inline`. We > > > currently > > > handle it as an attribute because it is more convenient for us > > > but is > > > it ok or should we create a separate function to mark a function > > > as > > > inlined? > > > > > > Thanks in advance for the review. > >
Re: [PATCH] Add support for function attributes and variable attributes
Ping David. :) Le mer. 15 nov. 2023 à 17:56, Antoni Boucher a écrit : > > David: another thing I remember you mentioned when you reviewed an > earlier version of this patch is the usage of `std::pair`. > I can't find where you said that, but I remember you mentioned that we > should use a struct instead. > Can you please elaborate again? > Thanks. > > On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote: > > Hi, > > > > This patch adds the (incomplete) support for function and variable > > attributes. The added attributes are the ones we're using in > > rustc_codegen_gcc but all the groundwork is done to add more (and we > > will very likely add more as we didn't add all the ones we use in > > rustc_codegen_gcc yet). > > > > The only big question with this patch is about `inline`. We currently > > handle it as an attribute because it is more convenient for us but is > > it ok or should we create a separate function to mark a function as > > inlined? > > > > Thanks in advance for the review. >
Re: [PATCH] Add support for function attributes and variable attributes
David: another thing I remember you mentioned when you reviewed an earlier version of this patch is the usage of `std::pair`. I can't find where you said that, but I remember you mentioned that we should use a struct instead. Can you please elaborate again? Thanks. On Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote: > Hi, > > This patch adds the (incomplete) support for function and variable > attributes. The added attributes are the ones we're using in > rustc_codegen_gcc but all the groundwork is done to add more (and we > will very likely add more as we didn't add all the ones we use in > rustc_codegen_gcc yet). > > The only big question with this patch is about `inline`. We currently > handle it as an attribute because it is more convenient for us but is > it ok or should we create a separate function to mark a function as > inlined? > > Thanks in advance for the review.
[PATCH] Add support for function attributes and variable attributes
Hi, This patch adds the (incomplete) support for function and variable attributes. The added attributes are the ones we're using in rustc_codegen_gcc but all the groundwork is done to add more (and we will very likely add more as we didn't add all the ones we use in rustc_codegen_gcc yet). The only big question with this patch is about `inline`. We currently handle it as an attribute because it is more convenient for us but is it ok or should we create a separate function to mark a function as inlined? Thanks in advance for the review. From df75f0eb8aacba249b6e791603752e35778951a4 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 20 Jun 2022 14:34:39 -0400 Subject: [PATCH] [PATCH] Add support for function attributes and variable attributes. gcc/jit/ChangeLog: * dummy-frontend.cc (handle_alias_attribute): New function. (handle_always_inline_attribute): New function. (handle_cold_attribute): New function. (handle_fnspec_attribute): New function. (handle_format_arg_attribute): New function. (handle_format_attribute): New function. (handle_noinline_attribute): New function. (handle_target_attribute): New function. (handle_used_attribute): New function. (handle_visibility_attribute): New function. (handle_weak_attribute): New function. (handle_alias_ifunc_attribute): New function. * jit-playback.cc (fn_attribute_to_string): New function. (variable_attribute_to_string): New function. (global_new_decl): Add attributes support. (set_variable_attribute): New function. (new_global): Add attributes support. (new_global_initialized): Add attributes support. (new_local): Add attributes support. * jit-playback.h (fn_attribute_to_string): New function. (set_variable_attribute): New function. * jit-recording.cc (recording::lvalue::add_attribute): New function. (recording::function::function): New function. (recording::function::write_to_dump): Add attributes support. (recording::function::add_attribute): New function. (recording::function::add_string_attribute): New function. (recording::function::add_integer_array_attribute): New function. (recording::global::replay_into): Add attributes support. (recording::local::replay_into): Add attributes support. * libgccjit.cc (gcc_jit_function_add_attribute): New function. (gcc_jit_function_add_string_attribute): New function. (gcc_jit_function_add_integer_array_attribute): New function. (gcc_jit_lvalue_add_attribute): New function. * libgccjit.h (enum gcc_jit_fn_attribute): New enum. (gcc_jit_function_add_attribute): New function. (gcc_jit_function_add_string_attribute): New function. (gcc_jit_function_add_integer_array_attribute): New function. (enum gcc_jit_variable_attribute): New function. (gcc_jit_lvalue_add_string_attribute): New function. * libgccjit.map: Declare new functions. gcc/testsuite/ChangeLog: * jit.dg/jit.exp: Add `jit-verify-assembler-output-not` test command. * jit.dg/test-restrict.c: New test. * jit.dg/test-restrict-attribute.c: New test. * jit.dg/test-alias-attribute.c: New test. * jit.dg/test-always_inline-attribute.c: New test. * jit.dg/test-cold-attribute.c: New test. * jit.dg/test-const-attribute.c: New test. * jit.dg/test-noinline-attribute.c: New test. * jit.dg/test-nonnull-attribute.c: New test. * jit.dg/test-pure-attribute.c: New test. * jit.dg/test-used-attribute.c: New test. * jit.dg/test-variable-attribute.c: New test. * jit.dg/test-weak-attribute.c: New test. gcc/jit/ChangeLog: * docs/topics/compatibility.rst: Add documentation for LIBGCCJIT_ABI_26. * docs/topics/types.rst: Add documentation for new functions. Co-authored-by: Antoni Boucher Signed-off-by: Guillaume Gomez --- gcc/jit/docs/topics/compatibility.rst | 12 + gcc/jit/docs/topics/types.rst | 77 +++ gcc/jit/dummy-frontend.cc | 504 -- gcc/jit/jit-playback.cc | 165 +- gcc/jit/jit-playback.h| 37 +- gcc/jit/jit-recording.cc | 166 +- gcc/jit/jit-recording.h | 19 +- gcc/jit/libgccjit.cc | 45 ++ gcc/jit/libgccjit.h | 49 ++ gcc/jit/libgccjit.map | 8 + gcc/testsuite/jit.dg/jit.exp | 33 ++ gcc/testsuite/jit.dg/test-alias-attribute.c | 50 ++ .../jit.dg/test-always_inline-attribute.c | 153 ++ gcc/testsuite/jit.dg/test-cold-attribute.c| 54 ++ gcc/testsuite/jit.dg/test-const-attribute.c | 134 + .../jit.dg/test-noinline-attribute.c | 114 gcc/testsuite/jit.dg/test-nonnull-attribute.c | 94 gcc/testsuite/jit.dg/test-pure-attribute.c| 134 + ...t-restrict.c => test-restrict-attribute.c} | 4 +- gcc/testsuite/jit.dg/test-used-attribute.c| 112 .../jit.dg/test-variable-attribute.c | 46 ++ gcc/testsuite/jit.dg/test-weak-attribute.c| 41 ++ 22 files changed, 1986 inse
