libgccjit: Add support for `restrict` attribute on function parameters

Hi,

This patch adds the possibility to specify the __restrict__ attribute
for function parameters. It is used by the Rust GCC backend.

Thanks in advance for the review.

My apologies, forgot to run the commit checkers. Here's the commit
with the errors fixed.

Le mer. 16 août 2023 à 18:32, Guillaume Gomez
<guillaume1.gomez@gmail.com> a écrit :
>
> Hi,
>
> This patch adds the possibility to specify the __restrict__ attribute
> for function parameters. It is used by the Rust GCC backend.
>
> Thanks in advance for the review.

On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit wrote:
> My apologies, forgot to run the commit checkers. Here's the commit
> with the errors fixed.
> 
> Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> <guillaume1.gomez@gmail.com> a écrit :
> > 
> > Hi,

Hi Guillaume, thanks for the patch.

> > 
> > This patch adds the possibility to specify the __restrict__
> > attribute
> > for function parameters. It is used by the Rust GCC backend.

What kind of testing has the patch had? (e.g. did you run "make check-
jit" ?  Has this been in use on real Rust code?)

Overall, this patch looks close to being ready, but some nits below...

[...]

> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> index 60eaf39bff6..2e0d08a06d8 100644
> --- a/gcc/jit/libgccjit.h
> +++ b/gcc/jit/libgccjit.h
> @@ -635,6 +635,10 @@ 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", get type "restrict T".  */
> +extern gcc_jit_type *
> +gcc_jit_type_get_restrict (gcc_jit_type *type);
> +
>  #define LIBGCCJIT_HAVE_SIZED_INTEGERS
>  
>  /* Given types LTYPE and RTYPE, return non-zero if they are
compatible.

Please add a feature macro:
#define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
(see the similar ones in the header).

> diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> index e52de0057a5..b7289b13845 100644
> --- a/gcc/jit/libgccjit.map
> +++ b/gcc/jit/libgccjit.map
> @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
>      gcc_jit_type_as_object;
>      gcc_jit_type_get_const;
>      gcc_jit_type_get_pointer;
> +    gcc_jit_type_get_restrict;
>      gcc_jit_type_get_volatile;

Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
to ABI_0.

> diff --git a/gcc/testsuite/jit.dg/test-restrict.c
b/gcc/testsuite/jit.dg/test-restrict.c
> new file mode 100644
> index 00000000000..4c8c4407f91
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-restrict.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 cold
> +	 attribute affects the optimizations. */

This refers to a "cold attribute"; is this a vestige of a copy-and-
paste from a different test case?

I see that the test scans the generated assembler.  Does the test
actually verify that restrict has an effect, or was that another
vestige from a different test case?

> +#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);
> +}
> +
> +#define TEST_COMPILING_TO_FILE
> +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> +#define OUTPUT_FILENAME  "output-of-test-restrict.c.s"
> +#include "harness.h"
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> +	/* Let's try to inject the equivalent of:
> +void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__
c) {
> +	*a += *c;
> +	*b += *c;
> +}
> +	*/
> +	gcc_jit_type *int_type =
> +		gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> +	gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
> +	gcc_jit_type *pint_restrict_type =
gcc_jit_type_get_restrict(pint_type);
> +
> +	gcc_jit_type *void_type =
> +		gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
> +
> +	gcc_jit_param *a =
> +		gcc_jit_context_new_param (ctxt, NULL,
pint_restrict_type, "a");
> +	gcc_jit_param *b =
> +		gcc_jit_context_new_param (ctxt, NULL,
pint_restrict_type, "b");
> +	gcc_jit_param *c =
> +		gcc_jit_context_new_param (ctxt, NULL,
pint_restrict_type, "c");
> +	gcc_jit_param *params[3] = {a, b, c};
> +
> +	gcc_jit_function *func_t =
> +		gcc_jit_context_new_function (ctxt, NULL,
> +					GCC_JIT_FUNCTION_EXPORTED,
> +					void_type,
> +					"t",
> +					3, params,
> +					0);
> +
> +	gcc_jit_block *block = gcc_jit_function_new_block (func_t,
NULL);
> +
> +	/* *a += *c; */
> +	gcc_jit_block_add_assignment_op (
> +		block, NULL,
> +		gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
(a), NULL),
> +		GCC_JIT_BINARY_OP_PLUS,
> +		gcc_jit_lvalue_as_rvalue (
> +			gcc_jit_rvalue_dereference
(gcc_jit_param_as_rvalue (c), NULL)));
> +	/* *b += *c; */
> +	gcc_jit_block_add_assignment_op (
> +		block, NULL,
> +		gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
(b), NULL),
> +		GCC_JIT_BINARY_OP_PLUS,
> +		gcc_jit_lvalue_as_rvalue (
> +			gcc_jit_rvalue_dereference
(gcc_jit_param_as_rvalue (c), NULL)));
> +
> +	gcc_jit_block_end_with_void_return (block, NULL);
> +}
> +
> +/* { dg-final { jit-verify-output-file-was-created "" } } */
> +/* { dg-final { jit-verify-assembler-output "addl	%eax, (%rdi)
> +	addl	%eax, (%rsi)" } } */
> -- 
> 2.34.1
> 

If this test is meant to run at -O3 and thus can't be part of test-
combination.c, please add a comment about it to
gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
place).

The patch also needs to add documentation for the new entrypoint (in 
topics/types.rst), and for the new ABI tag (in
topics/compatibility.rst).


Thanks again for the patch; hope the above is constructive
Dave

Hi Dave,

> What kind of testing has the patch had? (e.g. did you run "make check-
> jit" ?  Has this been in use on real Rust code?)

I tested it as Rust backend directly on this code:

```
pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
    *a += *c;
    *b += *c;
}
```

I ran it with `rustc` (and the GCC backend) with the following flags:
`-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the diff
you can see in the attached file. Explanations: the diff on the right
has the `__restrict__` attribute used whereas on the left it is the
current version where we don't handle it.

As for C testing, I used this code:

```
void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
    *a += *c;
    *b += *c;
}
```

(without the `__restrict__` of course when I need to have a witness
ASM). I attached the diff as well, this time the file with the use of
`__restrict__` in on the left. I compiled with the following flags:
`-S -O3`.

> Please add a feature macro:
> #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> (see the similar ones in the header).

I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
documentation as well to mention the ABI change.

> Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> to ABI_0.

I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last one.

> This refers to a "cold attribute"; is this a vestige of a copy-and-
> paste from a different test case?

It is a vestige indeed... Missed this one.

> I see that the test scans the generated assembler.  Does the test
> actually verify that restrict has an effect, or was that another
> vestige from a different test case?

No, this time it's what I wanted. Please see the C diff I provided
above to see that the ASM has a small diff that allowed me to confirm
that the `__restrict__` attribute was correctly set.

> If this test is meant to run at -O3 and thus can't be part of test-
> combination.c, please add a comment about it to
> gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> place).

Below `-O3`, this ASM difference doesn't appear unfortunately.

> The patch also needs to add documentation for the new entrypoint (in
> topics/types.rst), and for the new ABI tag (in
> topics/compatibility.rst).

Added!

> Thanks again for the patch; hope the above is constructive

It was incredibly useful! Thanks for taking time to writing down the
explanations.

The new patch is attached to this email.

Cordially.

Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com> a écrit :
>
> On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit wrote:
> > My apologies, forgot to run the commit checkers. Here's the commit
> > with the errors fixed.
> >
> > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > <guillaume1.gomez@gmail.com> a écrit :
> > >
> > > Hi,
>
> Hi Guillaume, thanks for the patch.
>
> > >
> > > This patch adds the possibility to specify the __restrict__
> > > attribute
> > > for function parameters. It is used by the Rust GCC backend.
>
> What kind of testing has the patch had? (e.g. did you run "make check-
> jit" ?  Has this been in use on real Rust code?)
>
> Overall, this patch looks close to being ready, but some nits below...
>
> [...]
>
> > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > index 60eaf39bff6..2e0d08a06d8 100644
> > --- a/gcc/jit/libgccjit.h
> > +++ b/gcc/jit/libgccjit.h
> > @@ -635,6 +635,10 @@ 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", get type "restrict T".  */
> > +extern gcc_jit_type *
> > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > +
> >  #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> >
> >  /* Given types LTYPE and RTYPE, return non-zero if they are
> compatible.
>
> Please add a feature macro:
> #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> (see the similar ones in the header).
>
> > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > index e52de0057a5..b7289b13845 100644
> > --- a/gcc/jit/libgccjit.map
> > +++ b/gcc/jit/libgccjit.map
> > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> >      gcc_jit_type_as_object;
> >      gcc_jit_type_get_const;
> >      gcc_jit_type_get_pointer;
> > +    gcc_jit_type_get_restrict;
> >      gcc_jit_type_get_volatile;
>
> Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> to ABI_0.
>
> > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> b/gcc/testsuite/jit.dg/test-restrict.c
> > new file mode 100644
> > index 00000000000..4c8c4407f91
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-restrict.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 cold
> > +      attribute affects the optimizations. */
>
> This refers to a "cold attribute"; is this a vestige of a copy-and-
> paste from a different test case?
>
> I see that the test scans the generated assembler.  Does the test
> actually verify that restrict has an effect, or was that another
> vestige from a different test case?
>
> > +#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);
> > +}
> > +
> > +#define TEST_COMPILING_TO_FILE
> > +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > +#define OUTPUT_FILENAME  "output-of-test-restrict.c.s"
> > +#include "harness.h"
> > +
> > +void
> > +create_code (gcc_jit_context *ctxt, void *user_data)
> > +{
> > +     /* Let's try to inject the equivalent of:
> > +void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__
> c) {
> > +     *a += *c;
> > +     *b += *c;
> > +}
> > +     */
> > +     gcc_jit_type *int_type =
> > +             gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> > +     gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
> > +     gcc_jit_type *pint_restrict_type =
> gcc_jit_type_get_restrict(pint_type);
> > +
> > +     gcc_jit_type *void_type =
> > +             gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
> > +
> > +     gcc_jit_param *a =
> > +             gcc_jit_context_new_param (ctxt, NULL,
> pint_restrict_type, "a");
> > +     gcc_jit_param *b =
> > +             gcc_jit_context_new_param (ctxt, NULL,
> pint_restrict_type, "b");
> > +     gcc_jit_param *c =
> > +             gcc_jit_context_new_param (ctxt, NULL,
> pint_restrict_type, "c");
> > +     gcc_jit_param *params[3] = {a, b, c};
> > +
> > +     gcc_jit_function *func_t =
> > +             gcc_jit_context_new_function (ctxt, NULL,
> > +                                     GCC_JIT_FUNCTION_EXPORTED,
> > +                                     void_type,
> > +                                     "t",
> > +                                     3, params,
> > +                                     0);
> > +
> > +     gcc_jit_block *block = gcc_jit_function_new_block (func_t,
> NULL);
> > +
> > +     /* *a += *c; */
> > +     gcc_jit_block_add_assignment_op (
> > +             block, NULL,
> > +             gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> (a), NULL),
> > +             GCC_JIT_BINARY_OP_PLUS,
> > +             gcc_jit_lvalue_as_rvalue (
> > +                     gcc_jit_rvalue_dereference
> (gcc_jit_param_as_rvalue (c), NULL)));
> > +     /* *b += *c; */
> > +     gcc_jit_block_add_assignment_op (
> > +             block, NULL,
> > +             gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> (b), NULL),
> > +             GCC_JIT_BINARY_OP_PLUS,
> > +             gcc_jit_lvalue_as_rvalue (
> > +                     gcc_jit_rvalue_dereference
> (gcc_jit_param_as_rvalue (c), NULL)));
> > +
> > +     gcc_jit_block_end_with_void_return (block, NULL);
> > +}
> > +
> > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > +/* { dg-final { jit-verify-assembler-output "addl    %eax, (%rdi)
> > +     addl    %eax, (%rsi)" } } */
> > --
> > 2.34.1
> >
>
> If this test is meant to run at -O3 and thus can't be part of test-
> combination.c, please add a comment about it to
> gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> place).
>
> The patch also needs to add documentation for the new entrypoint (in
> topics/types.rst), and for the new ABI tag (in
> topics/compatibility.rst).
>
>
> Thanks again for the patch; hope the above is constructive
> Dave
>

Antoni spot a typo I made:

I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
`LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this patch, sorry
for the noise.

Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
<guillaume1.gomez@gmail.com> a écrit :
>
> Hi Dave,
>
> > What kind of testing has the patch had? (e.g. did you run "make check-
> > jit" ?  Has this been in use on real Rust code?)
>
> I tested it as Rust backend directly on this code:
>
> ```
> pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
>     *a += *c;
>     *b += *c;
> }
> ```
>
> I ran it with `rustc` (and the GCC backend) with the following flags:
> `-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the diff
> you can see in the attached file. Explanations: the diff on the right
> has the `__restrict__` attribute used whereas on the left it is the
> current version where we don't handle it.
>
> As for C testing, I used this code:
>
> ```
> void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
>     *a += *c;
>     *b += *c;
> }
> ```
>
> (without the `__restrict__` of course when I need to have a witness
> ASM). I attached the diff as well, this time the file with the use of
> `__restrict__` in on the left. I compiled with the following flags:
> `-S -O3`.
>
> > Please add a feature macro:
> > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > (see the similar ones in the header).
>
> I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
> documentation as well to mention the ABI change.
>
> > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> > to ABI_0.
>
> I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last one.
>
> > This refers to a "cold attribute"; is this a vestige of a copy-and-
> > paste from a different test case?
>
> It is a vestige indeed... Missed this one.
>
> > I see that the test scans the generated assembler.  Does the test
> > actually verify that restrict has an effect, or was that another
> > vestige from a different test case?
>
> No, this time it's what I wanted. Please see the C diff I provided
> above to see that the ASM has a small diff that allowed me to confirm
> that the `__restrict__` attribute was correctly set.
>
> > If this test is meant to run at -O3 and thus can't be part of test-
> > combination.c, please add a comment about it to
> > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> > place).
>
> Below `-O3`, this ASM difference doesn't appear unfortunately.
>
> > The patch also needs to add documentation for the new entrypoint (in
> > topics/types.rst), and for the new ABI tag (in
> > topics/compatibility.rst).
>
> Added!
>
> > Thanks again for the patch; hope the above is constructive
>
> It was incredibly useful! Thanks for taking time to writing down the
> explanations.
>
> The new patch is attached to this email.
>
> Cordially.
>
> Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com> a écrit :
> >
> > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit wrote:
> > > My apologies, forgot to run the commit checkers. Here's the commit
> > > with the errors fixed.
> > >
> > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > <guillaume1.gomez@gmail.com> a écrit :
> > > >
> > > > Hi,
> >
> > Hi Guillaume, thanks for the patch.
> >
> > > >
> > > > This patch adds the possibility to specify the __restrict__
> > > > attribute
> > > > for function parameters. It is used by the Rust GCC backend.
> >
> > What kind of testing has the patch had? (e.g. did you run "make check-
> > jit" ?  Has this been in use on real Rust code?)
> >
> > Overall, this patch looks close to being ready, but some nits below...
> >
> > [...]
> >
> > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > index 60eaf39bff6..2e0d08a06d8 100644
> > > --- a/gcc/jit/libgccjit.h
> > > +++ b/gcc/jit/libgccjit.h
> > > @@ -635,6 +635,10 @@ 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", get type "restrict T".  */
> > > +extern gcc_jit_type *
> > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > +
> > >  #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > >
> > >  /* Given types LTYPE and RTYPE, return non-zero if they are
> > compatible.
> >
> > Please add a feature macro:
> > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > (see the similar ones in the header).
> >
> > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > index e52de0057a5..b7289b13845 100644
> > > --- a/gcc/jit/libgccjit.map
> > > +++ b/gcc/jit/libgccjit.map
> > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > >      gcc_jit_type_as_object;
> > >      gcc_jit_type_get_const;
> > >      gcc_jit_type_get_pointer;
> > > +    gcc_jit_type_get_restrict;
> > >      gcc_jit_type_get_volatile;
> >
> > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> > to ABI_0.
> >
> > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > b/gcc/testsuite/jit.dg/test-restrict.c
> > > new file mode 100644
> > > index 00000000000..4c8c4407f91
> > > --- /dev/null
> > > +++ b/gcc/testsuite/jit.dg/test-restrict.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 cold
> > > +      attribute affects the optimizations. */
> >
> > This refers to a "cold attribute"; is this a vestige of a copy-and-
> > paste from a different test case?
> >
> > I see that the test scans the generated assembler.  Does the test
> > actually verify that restrict has an effect, or was that another
> > vestige from a different test case?
> >
> > > +#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);
> > > +}
> > > +
> > > +#define TEST_COMPILING_TO_FILE
> > > +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > +#define OUTPUT_FILENAME  "output-of-test-restrict.c.s"
> > > +#include "harness.h"
> > > +
> > > +void
> > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > +{
> > > +     /* Let's try to inject the equivalent of:
> > > +void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__
> > c) {
> > > +     *a += *c;
> > > +     *b += *c;
> > > +}
> > > +     */
> > > +     gcc_jit_type *int_type =
> > > +             gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> > > +     gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
> > > +     gcc_jit_type *pint_restrict_type =
> > gcc_jit_type_get_restrict(pint_type);
> > > +
> > > +     gcc_jit_type *void_type =
> > > +             gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
> > > +
> > > +     gcc_jit_param *a =
> > > +             gcc_jit_context_new_param (ctxt, NULL,
> > pint_restrict_type, "a");
> > > +     gcc_jit_param *b =
> > > +             gcc_jit_context_new_param (ctxt, NULL,
> > pint_restrict_type, "b");
> > > +     gcc_jit_param *c =
> > > +             gcc_jit_context_new_param (ctxt, NULL,
> > pint_restrict_type, "c");
> > > +     gcc_jit_param *params[3] = {a, b, c};
> > > +
> > > +     gcc_jit_function *func_t =
> > > +             gcc_jit_context_new_function (ctxt, NULL,
> > > +                                     GCC_JIT_FUNCTION_EXPORTED,
> > > +                                     void_type,
> > > +                                     "t",
> > > +                                     3, params,
> > > +                                     0);
> > > +
> > > +     gcc_jit_block *block = gcc_jit_function_new_block (func_t,
> > NULL);
> > > +
> > > +     /* *a += *c; */
> > > +     gcc_jit_block_add_assignment_op (
> > > +             block, NULL,
> > > +             gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> > (a), NULL),
> > > +             GCC_JIT_BINARY_OP_PLUS,
> > > +             gcc_jit_lvalue_as_rvalue (
> > > +                     gcc_jit_rvalue_dereference
> > (gcc_jit_param_as_rvalue (c), NULL)));
> > > +     /* *b += *c; */
> > > +     gcc_jit_block_add_assignment_op (
> > > +             block, NULL,
> > > +             gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> > (b), NULL),
> > > +             GCC_JIT_BINARY_OP_PLUS,
> > > +             gcc_jit_lvalue_as_rvalue (
> > > +                     gcc_jit_rvalue_dereference
> > (gcc_jit_param_as_rvalue (c), NULL)));
> > > +
> > > +     gcc_jit_block_end_with_void_return (block, NULL);
> > > +}
> > > +
> > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > +/* { dg-final { jit-verify-assembler-output "addl    %eax, (%rdi)
> > > +     addl    %eax, (%rsi)" } } */
> > > --
> > > 2.34.1
> > >
> >
> > If this test is meant to run at -O3 and thus can't be part of test-
> > combination.c, please add a comment about it to
> > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> > place).
> >
> > The patch also needs to add documentation for the new entrypoint (in
> > topics/types.rst), and for the new ABI tag (in
> > topics/compatibility.rst).
> >
> >
> > Thanks again for the patch; hope the above is constructive
> > Dave
> >

And now I just discovered that a lot of commits from Antoni's fork
haven't been sent upstream which is why the ABI count is so high in
his repository. Fixed that as well.

Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
<guillaume1.gomez@gmail.com> a écrit :
>
> Antoni spot a typo I made:
>
> I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this patch, sorry
> for the noise.
>
> Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> <guillaume1.gomez@gmail.com> a écrit :
> >
> > Hi Dave,
> >
> > > What kind of testing has the patch had? (e.g. did you run "make check-
> > > jit" ?  Has this been in use on real Rust code?)
> >
> > I tested it as Rust backend directly on this code:
> >
> > ```
> > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> >     *a += *c;
> >     *b += *c;
> > }
> > ```
> >
> > I ran it with `rustc` (and the GCC backend) with the following flags:
> > `-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the diff
> > you can see in the attached file. Explanations: the diff on the right
> > has the `__restrict__` attribute used whereas on the left it is the
> > current version where we don't handle it.
> >
> > As for C testing, I used this code:
> >
> > ```
> > void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
> >     *a += *c;
> >     *b += *c;
> > }
> > ```
> >
> > (without the `__restrict__` of course when I need to have a witness
> > ASM). I attached the diff as well, this time the file with the use of
> > `__restrict__` in on the left. I compiled with the following flags:
> > `-S -O3`.
> >
> > > Please add a feature macro:
> > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > (see the similar ones in the header).
> >
> > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
> > documentation as well to mention the ABI change.
> >
> > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> > > to ABI_0.
> >
> > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last one.
> >
> > > This refers to a "cold attribute"; is this a vestige of a copy-and-
> > > paste from a different test case?
> >
> > It is a vestige indeed... Missed this one.
> >
> > > I see that the test scans the generated assembler.  Does the test
> > > actually verify that restrict has an effect, or was that another
> > > vestige from a different test case?
> >
> > No, this time it's what I wanted. Please see the C diff I provided
> > above to see that the ASM has a small diff that allowed me to confirm
> > that the `__restrict__` attribute was correctly set.
> >
> > > If this test is meant to run at -O3 and thus can't be part of test-
> > > combination.c, please add a comment about it to
> > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> > > place).
> >
> > Below `-O3`, this ASM difference doesn't appear unfortunately.
> >
> > > The patch also needs to add documentation for the new entrypoint (in
> > > topics/types.rst), and for the new ABI tag (in
> > > topics/compatibility.rst).
> >
> > Added!
> >
> > > Thanks again for the patch; hope the above is constructive
> >
> > It was incredibly useful! Thanks for taking time to writing down the
> > explanations.
> >
> > The new patch is attached to this email.
> >
> > Cordially.
> >
> > Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com> a écrit :
> > >
> > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit wrote:
> > > > My apologies, forgot to run the commit checkers. Here's the commit
> > > > with the errors fixed.
> > > >
> > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > >
> > > > > Hi,
> > >
> > > Hi Guillaume, thanks for the patch.
> > >
> > > > >
> > > > > This patch adds the possibility to specify the __restrict__
> > > > > attribute
> > > > > for function parameters. It is used by the Rust GCC backend.
> > >
> > > What kind of testing has the patch had? (e.g. did you run "make check-
> > > jit" ?  Has this been in use on real Rust code?)
> > >
> > > Overall, this patch looks close to being ready, but some nits below...
> > >
> > > [...]
> > >
> > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > --- a/gcc/jit/libgccjit.h
> > > > +++ b/gcc/jit/libgccjit.h
> > > > @@ -635,6 +635,10 @@ 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", get type "restrict T".  */
> > > > +extern gcc_jit_type *
> > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > +
> > > >  #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > >
> > > >  /* Given types LTYPE and RTYPE, return non-zero if they are
> > > compatible.
> > >
> > > Please add a feature macro:
> > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > (see the similar ones in the header).
> > >
> > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > > index e52de0057a5..b7289b13845 100644
> > > > --- a/gcc/jit/libgccjit.map
> > > > +++ b/gcc/jit/libgccjit.map
> > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > >      gcc_jit_type_as_object;
> > > >      gcc_jit_type_get_const;
> > > >      gcc_jit_type_get_pointer;
> > > > +    gcc_jit_type_get_restrict;
> > > >      gcc_jit_type_get_volatile;
> > >
> > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> > > to ABI_0.
> > >
> > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > new file mode 100644
> > > > index 00000000000..4c8c4407f91
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/jit.dg/test-restrict.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 cold
> > > > +      attribute affects the optimizations. */
> > >
> > > This refers to a "cold attribute"; is this a vestige of a copy-and-
> > > paste from a different test case?
> > >
> > > I see that the test scans the generated assembler.  Does the test
> > > actually verify that restrict has an effect, or was that another
> > > vestige from a different test case?
> > >
> > > > +#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);
> > > > +}
> > > > +
> > > > +#define TEST_COMPILING_TO_FILE
> > > > +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > +#define OUTPUT_FILENAME  "output-of-test-restrict.c.s"
> > > > +#include "harness.h"
> > > > +
> > > > +void
> > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > +{
> > > > +     /* Let's try to inject the equivalent of:
> > > > +void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__
> > > c) {
> > > > +     *a += *c;
> > > > +     *b += *c;
> > > > +}
> > > > +     */
> > > > +     gcc_jit_type *int_type =
> > > > +             gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> > > > +     gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
> > > > +     gcc_jit_type *pint_restrict_type =
> > > gcc_jit_type_get_restrict(pint_type);
> > > > +
> > > > +     gcc_jit_type *void_type =
> > > > +             gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
> > > > +
> > > > +     gcc_jit_param *a =
> > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > pint_restrict_type, "a");
> > > > +     gcc_jit_param *b =
> > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > pint_restrict_type, "b");
> > > > +     gcc_jit_param *c =
> > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > pint_restrict_type, "c");
> > > > +     gcc_jit_param *params[3] = {a, b, c};
> > > > +
> > > > +     gcc_jit_function *func_t =
> > > > +             gcc_jit_context_new_function (ctxt, NULL,
> > > > +                                     GCC_JIT_FUNCTION_EXPORTED,
> > > > +                                     void_type,
> > > > +                                     "t",
> > > > +                                     3, params,
> > > > +                                     0);
> > > > +
> > > > +     gcc_jit_block *block = gcc_jit_function_new_block (func_t,
> > > NULL);
> > > > +
> > > > +     /* *a += *c; */
> > > > +     gcc_jit_block_add_assignment_op (
> > > > +             block, NULL,
> > > > +             gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> > > (a), NULL),
> > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > +             gcc_jit_lvalue_as_rvalue (
> > > > +                     gcc_jit_rvalue_dereference
> > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > +     /* *b += *c; */
> > > > +     gcc_jit_block_add_assignment_op (
> > > > +             block, NULL,
> > > > +             gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> > > (b), NULL),
> > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > +             gcc_jit_lvalue_as_rvalue (
> > > > +                     gcc_jit_rvalue_dereference
> > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > +
> > > > +     gcc_jit_block_end_with_void_return (block, NULL);
> > > > +}
> > > > +
> > > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > > +/* { dg-final { jit-verify-assembler-output "addl    %eax, (%rdi)
> > > > +     addl    %eax, (%rsi)" } } */
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > If this test is meant to run at -O3 and thus can't be part of test-
> > > combination.c, please add a comment about it to
> > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> > > place).
> > >
> > > The patch also needs to add documentation for the new entrypoint (in
> > > topics/types.rst), and for the new ABI tag (in
> > > topics/compatibility.rst).
> > >
> > >
> > > Thanks again for the patch; hope the above is constructive
> > > Dave
> > >

On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> And now I just discovered that a lot of commits from Antoni's fork
> haven't been sent upstream which is why the ABI count is so high in
> his repository. Fixed that as well.

Thanks for the updated patch; I was about to comment on that.

This version is good for gcc trunk.

Dave

> 
> Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> <guillaume1.gomez@gmail.com> a écrit :
> > 
> > Antoni spot a typo I made:
> > 
> > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this patch,
> > sorry
> > for the noise.
> > 
> > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > <guillaume1.gomez@gmail.com> a écrit :
> > > 
> > > Hi Dave,
> > > 
> > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > check-
> > > > jit" ?  Has this been in use on real Rust code?)
> > > 
> > > I tested it as Rust backend directly on this code:
> > > 
> > > ```
> > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > >     *a += *c;
> > >     *b += *c;
> > > }
> > > ```
> > > 
> > > I ran it with `rustc` (and the GCC backend) with the following
> > > flags:
> > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the
> > > diff
> > > you can see in the attached file. Explanations: the diff on the
> > > right
> > > has the `__restrict__` attribute used whereas on the left it is
> > > the
> > > current version where we don't handle it.
> > > 
> > > As for C testing, I used this code:
> > > 
> > > ```
> > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > *__restrict__ c) {
> > >     *a += *c;
> > >     *b += *c;
> > > }
> > > ```
> > > 
> > > (without the `__restrict__` of course when I need to have a
> > > witness
> > > ASM). I attached the diff as well, this time the file with the
> > > use of
> > > `__restrict__` in on the left. I compiled with the following
> > > flags:
> > > `-S -O3`.
> > > 
> > > > Please add a feature macro:
> > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > (see the similar ones in the header).
> > > 
> > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
> > > documentation as well to mention the ABI change.
> > > 
> > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > adding this
> > > > to ABI_0.
> > > 
> > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last
> > > one.
> > > 
> > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > and-
> > > > paste from a different test case?
> > > 
> > > It is a vestige indeed... Missed this one.
> > > 
> > > > I see that the test scans the generated assembler.  Does the
> > > > test
> > > > actually verify that restrict has an effect, or was that
> > > > another
> > > > vestige from a different test case?
> > > 
> > > No, this time it's what I wanted. Please see the C diff I
> > > provided
> > > above to see that the ASM has a small diff that allowed me to
> > > confirm
> > > that the `__restrict__` attribute was correctly set.
> > > 
> > > > If this test is meant to run at -O3 and thus can't be part of
> > > > test-
> > > > combination.c, please add a comment about it to
> > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > alphabetical
> > > > place).
> > > 
> > > Below `-O3`, this ASM difference doesn't appear unfortunately.
> > > 
> > > > The patch also needs to add documentation for the new
> > > > entrypoint (in
> > > > topics/types.rst), and for the new ABI tag (in
> > > > topics/compatibility.rst).
> > > 
> > > Added!
> > > 
> > > > Thanks again for the patch; hope the above is constructive
> > > 
> > > It was incredibly useful! Thanks for taking time to writing down
> > > the
> > > explanations.
> > > 
> > > The new patch is attached to this email.
> > > 
> > > Cordially.
> > > 
> > > Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com>
> > > a écrit :
> > > > 
> > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit
> > > > wrote:
> > > > > My apologies, forgot to run the commit checkers. Here's the
> > > > > commit
> > > > > with the errors fixed.
> > > > > 
> > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > 
> > > > > > Hi,
> > > > 
> > > > Hi Guillaume, thanks for the patch.
> > > > 
> > > > > > 
> > > > > > This patch adds the possibility to specify the __restrict__
> > > > > > attribute
> > > > > > for function parameters. It is used by the Rust GCC
> > > > > > backend.
> > > > 
> > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > check-
> > > > jit" ?  Has this been in use on real Rust code?)
> > > > 
> > > > Overall, this patch looks close to being ready, but some nits
> > > > below...
> > > > 
> > > > [...]
> > > > 
> > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > --- a/gcc/jit/libgccjit.h
> > > > > +++ b/gcc/jit/libgccjit.h
> > > > > @@ -635,6 +635,10 @@ 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", get type "restrict T".  */
> > > > > +extern gcc_jit_type *
> > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > +
> > > > >  #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > 
> > > > >  /* Given types LTYPE and RTYPE, return non-zero if they are
> > > > compatible.
> > > > 
> > > > Please add a feature macro:
> > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > (see the similar ones in the header).
> > > > 
> > > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > > > index e52de0057a5..b7289b13845 100644
> > > > > --- a/gcc/jit/libgccjit.map
> > > > > +++ b/gcc/jit/libgccjit.map
> > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > >      gcc_jit_type_as_object;
> > > > >      gcc_jit_type_get_const;
> > > > >      gcc_jit_type_get_pointer;
> > > > > +    gcc_jit_type_get_restrict;
> > > > >      gcc_jit_type_get_volatile;
> > > > 
> > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > adding this
> > > > to ABI_0.
> > > > 
> > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > new file mode 100644
> > > > > index 00000000000..4c8c4407f91
> > > > > --- /dev/null
> > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.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 cold
> > > > > +      attribute affects the optimizations. */
> > > > 
> > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > and-
> > > > paste from a different test case?
> > > > 
> > > > I see that the test scans the generated assembler.  Does the
> > > > test
> > > > actually verify that restrict has an effect, or was that
> > > > another
> > > > vestige from a different test case?
> > > > 
> > > > > +#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);
> > > > > +}
> > > > > +
> > > > > +#define TEST_COMPILING_TO_FILE
> > > > > +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > +#define OUTPUT_FILENAME  "output-of-test-restrict.c.s"
> > > > > +#include "harness.h"
> > > > > +
> > > > > +void
> > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > +{
> > > > > +     /* Let's try to inject the equivalent of:
> > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > *__restrict__
> > > > c) {
> > > > > +     *a += *c;
> > > > > +     *b += *c;
> > > > > +}
> > > > > +     */
> > > > > +     gcc_jit_type *int_type =
> > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > GCC_JIT_TYPE_INT);
> > > > > +     gcc_jit_type *pint_type =
> > > > > gcc_jit_type_get_pointer(int_type);
> > > > > +     gcc_jit_type *pint_restrict_type =
> > > > gcc_jit_type_get_restrict(pint_type);
> > > > > +
> > > > > +     gcc_jit_type *void_type =
> > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > GCC_JIT_TYPE_VOID);
> > > > > +
> > > > > +     gcc_jit_param *a =
> > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > pint_restrict_type, "a");
> > > > > +     gcc_jit_param *b =
> > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > pint_restrict_type, "b");
> > > > > +     gcc_jit_param *c =
> > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > pint_restrict_type, "c");
> > > > > +     gcc_jit_param *params[3] = {a, b, c};
> > > > > +
> > > > > +     gcc_jit_function *func_t =
> > > > > +             gcc_jit_context_new_function (ctxt, NULL,
> > > > > +                                    
> > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > +                                     void_type,
> > > > > +                                     "t",
> > > > > +                                     3, params,
> > > > > +                                     0);
> > > > > +
> > > > > +     gcc_jit_block *block = gcc_jit_function_new_block
> > > > > (func_t,
> > > > NULL);
> > > > > +
> > > > > +     /* *a += *c; */
> > > > > +     gcc_jit_block_add_assignment_op (
> > > > > +             block, NULL,
> > > > > +             gcc_jit_rvalue_dereference
> > > > > (gcc_jit_param_as_rvalue
> > > > (a), NULL),
> > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > +                     gcc_jit_rvalue_dereference
> > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > +     /* *b += *c; */
> > > > > +     gcc_jit_block_add_assignment_op (
> > > > > +             block, NULL,
> > > > > +             gcc_jit_rvalue_dereference
> > > > > (gcc_jit_param_as_rvalue
> > > > (b), NULL),
> > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > +                     gcc_jit_rvalue_dereference
> > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > +
> > > > > +     gcc_jit_block_end_with_void_return (block, NULL);
> > > > > +}
> > > > > +
> > > > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > > > +/* { dg-final { jit-verify-assembler-output "addl    %eax,
> > > > > (%rdi)
> > > > > +     addl    %eax, (%rsi)" } } */
> > > > > --
> > > > > 2.34.1
> > > > > 
> > > > 
> > > > If this test is meant to run at -O3 and thus can't be part of
> > > > test-
> > > > combination.c, please add a comment about it to
> > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > alphabetical
> > > > place).
> > > > 
> > > > The patch also needs to add documentation for the new
> > > > entrypoint (in
> > > > topics/types.rst), and for the new ABI tag (in
> > > > topics/compatibility.rst).
> > > > 
> > > > 
> > > > Thanks again for the patch; hope the above is constructive
> > > > Dave
> > > >

Thanks for the review!

Le jeu. 17 août 2023 à 17:50, David Malcolm <dmalcolm@redhat.com> a écrit :
>
> On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> > And now I just discovered that a lot of commits from Antoni's fork
> > haven't been sent upstream which is why the ABI count is so high in
> > his repository. Fixed that as well.
>
> Thanks for the updated patch; I was about to comment on that.
>
> This version is good for gcc trunk.
>
> Dave
>
> >
> > Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> > <guillaume1.gomez@gmail.com> a écrit :
> > >
> > > Antoni spot a typo I made:
> > >
> > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this patch,
> > > sorry
> > > for the noise.
> > >
> > > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > > <guillaume1.gomez@gmail.com> a écrit :
> > > >
> > > > Hi Dave,
> > > >
> > > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > > check-
> > > > > jit" ?  Has this been in use on real Rust code?)
> > > >
> > > > I tested it as Rust backend directly on this code:
> > > >
> > > > ```
> > > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > >     *a += *c;
> > > >     *b += *c;
> > > > }
> > > > ```
> > > >
> > > > I ran it with `rustc` (and the GCC backend) with the following
> > > > flags:
> > > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the
> > > > diff
> > > > you can see in the attached file. Explanations: the diff on the
> > > > right
> > > > has the `__restrict__` attribute used whereas on the left it is
> > > > the
> > > > current version where we don't handle it.
> > > >
> > > > As for C testing, I used this code:
> > > >
> > > > ```
> > > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > > *__restrict__ c) {
> > > >     *a += *c;
> > > >     *b += *c;
> > > > }
> > > > ```
> > > >
> > > > (without the `__restrict__` of course when I need to have a
> > > > witness
> > > > ASM). I attached the diff as well, this time the file with the
> > > > use of
> > > > `__restrict__` in on the left. I compiled with the following
> > > > flags:
> > > > `-S -O3`.
> > > >
> > > > > Please add a feature macro:
> > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > (see the similar ones in the header).
> > > >
> > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
> > > > documentation as well to mention the ABI change.
> > > >
> > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > > adding this
> > > > > to ABI_0.
> > > >
> > > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last
> > > > one.
> > > >
> > > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > > and-
> > > > > paste from a different test case?
> > > >
> > > > It is a vestige indeed... Missed this one.
> > > >
> > > > > I see that the test scans the generated assembler.  Does the
> > > > > test
> > > > > actually verify that restrict has an effect, or was that
> > > > > another
> > > > > vestige from a different test case?
> > > >
> > > > No, this time it's what I wanted. Please see the C diff I
> > > > provided
> > > > above to see that the ASM has a small diff that allowed me to
> > > > confirm
> > > > that the `__restrict__` attribute was correctly set.
> > > >
> > > > > If this test is meant to run at -O3 and thus can't be part of
> > > > > test-
> > > > > combination.c, please add a comment about it to
> > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > alphabetical
> > > > > place).
> > > >
> > > > Below `-O3`, this ASM difference doesn't appear unfortunately.
> > > >
> > > > > The patch also needs to add documentation for the new
> > > > > entrypoint (in
> > > > > topics/types.rst), and for the new ABI tag (in
> > > > > topics/compatibility.rst).
> > > >
> > > > Added!
> > > >
> > > > > Thanks again for the patch; hope the above is constructive
> > > >
> > > > It was incredibly useful! Thanks for taking time to writing down
> > > > the
> > > > explanations.
> > > >
> > > > The new patch is attached to this email.
> > > >
> > > > Cordially.
> > > >
> > > > Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com>
> > > > a écrit :
> > > > >
> > > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit
> > > > > wrote:
> > > > > > My apologies, forgot to run the commit checkers. Here's the
> > > > > > commit
> > > > > > with the errors fixed.
> > > > > >
> > > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > >
> > > > > > > Hi,
> > > > >
> > > > > Hi Guillaume, thanks for the patch.
> > > > >
> > > > > > >
> > > > > > > This patch adds the possibility to specify the __restrict__
> > > > > > > attribute
> > > > > > > for function parameters. It is used by the Rust GCC
> > > > > > > backend.
> > > > >
> > > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > > check-
> > > > > jit" ?  Has this been in use on real Rust code?)
> > > > >
> > > > > Overall, this patch looks close to being ready, but some nits
> > > > > below...
> > > > >
> > > > > [...]
> > > > >
> > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > > --- a/gcc/jit/libgccjit.h
> > > > > > +++ b/gcc/jit/libgccjit.h
> > > > > > @@ -635,6 +635,10 @@ 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", get type "restrict T".  */
> > > > > > +extern gcc_jit_type *
> > > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > > +
> > > > > >  #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > >
> > > > > >  /* Given types LTYPE and RTYPE, return non-zero if they are
> > > > > compatible.
> > > > >
> > > > > Please add a feature macro:
> > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > (see the similar ones in the header).
> > > > >
> > > > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > > > > index e52de0057a5..b7289b13845 100644
> > > > > > --- a/gcc/jit/libgccjit.map
> > > > > > +++ b/gcc/jit/libgccjit.map
> > > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > >      gcc_jit_type_as_object;
> > > > > >      gcc_jit_type_get_const;
> > > > > >      gcc_jit_type_get_pointer;
> > > > > > +    gcc_jit_type_get_restrict;
> > > > > >      gcc_jit_type_get_volatile;
> > > > >
> > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > > adding this
> > > > > to ABI_0.
> > > > >
> > > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > new file mode 100644
> > > > > > index 00000000000..4c8c4407f91
> > > > > > --- /dev/null
> > > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.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 cold
> > > > > > +      attribute affects the optimizations. */
> > > > >
> > > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > > and-
> > > > > paste from a different test case?
> > > > >
> > > > > I see that the test scans the generated assembler.  Does the
> > > > > test
> > > > > actually verify that restrict has an effect, or was that
> > > > > another
> > > > > vestige from a different test case?
> > > > >
> > > > > > +#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);
> > > > > > +}
> > > > > > +
> > > > > > +#define TEST_COMPILING_TO_FILE
> > > > > > +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > > +#define OUTPUT_FILENAME  "output-of-test-restrict.c.s"
> > > > > > +#include "harness.h"
> > > > > > +
> > > > > > +void
> > > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > > +{
> > > > > > +     /* Let's try to inject the equivalent of:
> > > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > *__restrict__
> > > > > c) {
> > > > > > +     *a += *c;
> > > > > > +     *b += *c;
> > > > > > +}
> > > > > > +     */
> > > > > > +     gcc_jit_type *int_type =
> > > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > > GCC_JIT_TYPE_INT);
> > > > > > +     gcc_jit_type *pint_type =
> > > > > > gcc_jit_type_get_pointer(int_type);
> > > > > > +     gcc_jit_type *pint_restrict_type =
> > > > > gcc_jit_type_get_restrict(pint_type);
> > > > > > +
> > > > > > +     gcc_jit_type *void_type =
> > > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > > GCC_JIT_TYPE_VOID);
> > > > > > +
> > > > > > +     gcc_jit_param *a =
> > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > pint_restrict_type, "a");
> > > > > > +     gcc_jit_param *b =
> > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > pint_restrict_type, "b");
> > > > > > +     gcc_jit_param *c =
> > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > pint_restrict_type, "c");
> > > > > > +     gcc_jit_param *params[3] = {a, b, c};
> > > > > > +
> > > > > > +     gcc_jit_function *func_t =
> > > > > > +             gcc_jit_context_new_function (ctxt, NULL,
> > > > > > +
> > > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > > +                                     void_type,
> > > > > > +                                     "t",
> > > > > > +                                     3, params,
> > > > > > +                                     0);
> > > > > > +
> > > > > > +     gcc_jit_block *block = gcc_jit_function_new_block
> > > > > > (func_t,
> > > > > NULL);
> > > > > > +
> > > > > > +     /* *a += *c; */
> > > > > > +     gcc_jit_block_add_assignment_op (
> > > > > > +             block, NULL,
> > > > > > +             gcc_jit_rvalue_dereference
> > > > > > (gcc_jit_param_as_rvalue
> > > > > (a), NULL),
> > > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > > +                     gcc_jit_rvalue_dereference
> > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > +     /* *b += *c; */
> > > > > > +     gcc_jit_block_add_assignment_op (
> > > > > > +             block, NULL,
> > > > > > +             gcc_jit_rvalue_dereference
> > > > > > (gcc_jit_param_as_rvalue
> > > > > (b), NULL),
> > > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > > +                     gcc_jit_rvalue_dereference
> > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > +
> > > > > > +     gcc_jit_block_end_with_void_return (block, NULL);
> > > > > > +}
> > > > > > +
> > > > > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > > > > +/* { dg-final { jit-verify-assembler-output "addl    %eax,
> > > > > > (%rdi)
> > > > > > +     addl    %eax, (%rsi)" } } */
> > > > > > --
> > > > > > 2.34.1
> > > > > >
> > > > >
> > > > > If this test is meant to run at -O3 and thus can't be part of
> > > > > test-
> > > > > combination.c, please add a comment about it to
> > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > alphabetical
> > > > > place).
> > > > >
> > > > > The patch also needs to add documentation for the new
> > > > > entrypoint (in
> > > > > topics/types.rst), and for the new ABI tag (in
> > > > > topics/compatibility.rst).
> > > > >
> > > > >
> > > > > Thanks again for the patch; hope the above is constructive
> > > > > Dave
> > > > >
>

Quick question: do you plan to make the merge or should I ask Antoni?

Le jeu. 17 août 2023 à 17:59, Guillaume Gomez <guillaume1.gomez@gmail.com>
a écrit :

> Thanks for the review!
>
> Le jeu. 17 août 2023 à 17:50, David Malcolm <dmalcolm@redhat.com> a écrit
> :
> >
> > On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> > > And now I just discovered that a lot of commits from Antoni's fork
> > > haven't been sent upstream which is why the ABI count is so high in
> > > his repository. Fixed that as well.
> >
> > Thanks for the updated patch; I was about to comment on that.
> >
> > This version is good for gcc trunk.
> >
> > Dave
> >
> > >
> > > Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> > > <guillaume1.gomez@gmail.com> a écrit :
> > > >
> > > > Antoni spot a typo I made:
> > > >
> > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > > > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this patch,
> > > > sorry
> > > > for the noise.
> > > >
> > > > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > >
> > > > > Hi Dave,
> > > > >
> > > > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > > > check-
> > > > > > jit" ?  Has this been in use on real Rust code?)
> > > > >
> > > > > I tested it as Rust backend directly on this code:
> > > > >
> > > > > ```
> > > > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > > >     *a += *c;
> > > > >     *b += *c;
> > > > > }
> > > > > ```
> > > > >
> > > > > I ran it with `rustc` (and the GCC backend) with the following
> > > > > flags:
> > > > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the
> > > > > diff
> > > > > you can see in the attached file. Explanations: the diff on the
> > > > > right
> > > > > has the `__restrict__` attribute used whereas on the left it is
> > > > > the
> > > > > current version where we don't handle it.
> > > > >
> > > > > As for C testing, I used this code:
> > > > >
> > > > > ```
> > > > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > *__restrict__ c) {
> > > > >     *a += *c;
> > > > >     *b += *c;
> > > > > }
> > > > > ```
> > > > >
> > > > > (without the `__restrict__` of course when I need to have a
> > > > > witness
> > > > > ASM). I attached the diff as well, this time the file with the
> > > > > use of
> > > > > `__restrict__` in on the left. I compiled with the following
> > > > > flags:
> > > > > `-S -O3`.
> > > > >
> > > > > > Please add a feature macro:
> > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > (see the similar ones in the header).
> > > > >
> > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
> > > > > documentation as well to mention the ABI change.
> > > > >
> > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > > > adding this
> > > > > > to ABI_0.
> > > > >
> > > > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last
> > > > > one.
> > > > >
> > > > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > > > and-
> > > > > > paste from a different test case?
> > > > >
> > > > > It is a vestige indeed... Missed this one.
> > > > >
> > > > > > I see that the test scans the generated assembler.  Does the
> > > > > > test
> > > > > > actually verify that restrict has an effect, or was that
> > > > > > another
> > > > > > vestige from a different test case?
> > > > >
> > > > > No, this time it's what I wanted. Please see the C diff I
> > > > > provided
> > > > > above to see that the ASM has a small diff that allowed me to
> > > > > confirm
> > > > > that the `__restrict__` attribute was correctly set.
> > > > >
> > > > > > If this test is meant to run at -O3 and thus can't be part of
> > > > > > test-
> > > > > > combination.c, please add a comment about it to
> > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > alphabetical
> > > > > > place).
> > > > >
> > > > > Below `-O3`, this ASM difference doesn't appear unfortunately.
> > > > >
> > > > > > The patch also needs to add documentation for the new
> > > > > > entrypoint (in
> > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > topics/compatibility.rst).
> > > > >
> > > > > Added!
> > > > >
> > > > > > Thanks again for the patch; hope the above is constructive
> > > > >
> > > > > It was incredibly useful! Thanks for taking time to writing down
> > > > > the
> > > > > explanations.
> > > > >
> > > > > The new patch is attached to this email.
> > > > >
> > > > > Cordially.
> > > > >
> > > > > Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com>
> > > > > a écrit :
> > > > > >
> > > > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit
> > > > > > wrote:
> > > > > > > My apologies, forgot to run the commit checkers. Here's the
> > > > > > > commit
> > > > > > > with the errors fixed.
> > > > > > >
> > > > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > > >
> > > > > > > > Hi,
> > > > > >
> > > > > > Hi Guillaume, thanks for the patch.
> > > > > >
> > > > > > > >
> > > > > > > > This patch adds the possibility to specify the __restrict__
> > > > > > > > attribute
> > > > > > > > for function parameters. It is used by the Rust GCC
> > > > > > > > backend.
> > > > > >
> > > > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > > > check-
> > > > > > jit" ?  Has this been in use on real Rust code?)
> > > > > >
> > > > > > Overall, this patch looks close to being ready, but some nits
> > > > > > below...
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > > > --- a/gcc/jit/libgccjit.h
> > > > > > > +++ b/gcc/jit/libgccjit.h
> > > > > > > @@ -635,6 +635,10 @@ 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", get type "restrict T".  */
> > > > > > > +extern gcc_jit_type *
> > > > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > > > +
> > > > > > >  #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > > >
> > > > > > >  /* Given types LTYPE and RTYPE, return non-zero if they are
> > > > > > compatible.
> > > > > >
> > > > > > Please add a feature macro:
> > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > (see the similar ones in the header).
> > > > > >
> > > > > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > > > > > index e52de0057a5..b7289b13845 100644
> > > > > > > --- a/gcc/jit/libgccjit.map
> > > > > > > +++ b/gcc/jit/libgccjit.map
> > > > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > > >      gcc_jit_type_as_object;
> > > > > > >      gcc_jit_type_get_const;
> > > > > > >      gcc_jit_type_get_pointer;
> > > > > > > +    gcc_jit_type_get_restrict;
> > > > > > >      gcc_jit_type_get_volatile;
> > > > > >
> > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > > > adding this
> > > > > > to ABI_0.
> > > > > >
> > > > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > new file mode 100644
> > > > > > > index 00000000000..4c8c4407f91
> > > > > > > --- /dev/null
> > > > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.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 cold
> > > > > > > +      attribute affects the optimizations. */
> > > > > >
> > > > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > > > and-
> > > > > > paste from a different test case?
> > > > > >
> > > > > > I see that the test scans the generated assembler.  Does the
> > > > > > test
> > > > > > actually verify that restrict has an effect, or was that
> > > > > > another
> > > > > > vestige from a different test case?
> > > > > >
> > > > > > > +#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);
> > > > > > > +}
> > > > > > > +
> > > > > > > +#define TEST_COMPILING_TO_FILE
> > > > > > > +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > > > +#define OUTPUT_FILENAME  "output-of-test-restrict.c.s"
> > > > > > > +#include "harness.h"
> > > > > > > +
> > > > > > > +void
> > > > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > > > +{
> > > > > > > +     /* Let's try to inject the equivalent of:
> > > > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > *__restrict__
> > > > > > c) {
> > > > > > > +     *a += *c;
> > > > > > > +     *b += *c;
> > > > > > > +}
> > > > > > > +     */
> > > > > > > +     gcc_jit_type *int_type =
> > > > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > > > GCC_JIT_TYPE_INT);
> > > > > > > +     gcc_jit_type *pint_type =
> > > > > > > gcc_jit_type_get_pointer(int_type);
> > > > > > > +     gcc_jit_type *pint_restrict_type =
> > > > > > gcc_jit_type_get_restrict(pint_type);
> > > > > > > +
> > > > > > > +     gcc_jit_type *void_type =
> > > > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > > > GCC_JIT_TYPE_VOID);
> > > > > > > +
> > > > > > > +     gcc_jit_param *a =
> > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > pint_restrict_type, "a");
> > > > > > > +     gcc_jit_param *b =
> > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > pint_restrict_type, "b");
> > > > > > > +     gcc_jit_param *c =
> > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > pint_restrict_type, "c");
> > > > > > > +     gcc_jit_param *params[3] = {a, b, c};
> > > > > > > +
> > > > > > > +     gcc_jit_function *func_t =
> > > > > > > +             gcc_jit_context_new_function (ctxt, NULL,
> > > > > > > +
> > > > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > > > +                                     void_type,
> > > > > > > +                                     "t",
> > > > > > > +                                     3, params,
> > > > > > > +                                     0);
> > > > > > > +
> > > > > > > +     gcc_jit_block *block = gcc_jit_function_new_block
> > > > > > > (func_t,
> > > > > > NULL);
> > > > > > > +
> > > > > > > +     /* *a += *c; */
> > > > > > > +     gcc_jit_block_add_assignment_op (
> > > > > > > +             block, NULL,
> > > > > > > +             gcc_jit_rvalue_dereference
> > > > > > > (gcc_jit_param_as_rvalue
> > > > > > (a), NULL),
> > > > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > > > +                     gcc_jit_rvalue_dereference
> > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > +     /* *b += *c; */
> > > > > > > +     gcc_jit_block_add_assignment_op (
> > > > > > > +             block, NULL,
> > > > > > > +             gcc_jit_rvalue_dereference
> > > > > > > (gcc_jit_param_as_rvalue
> > > > > > (b), NULL),
> > > > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > > > +                     gcc_jit_rvalue_dereference
> > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > +
> > > > > > > +     gcc_jit_block_end_with_void_return (block, NULL);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > > > > > +/* { dg-final { jit-verify-assembler-output "addl    %eax,
> > > > > > > (%rdi)
> > > > > > > +     addl    %eax, (%rsi)" } } */
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > >
> > > > > >
> > > > > > If this test is meant to run at -O3 and thus can't be part of
> > > > > > test-
> > > > > > combination.c, please add a comment about it to
> > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > alphabetical
> > > > > > place).
> > > > > >
> > > > > > The patch also needs to add documentation for the new
> > > > > > entrypoint (in
> > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > topics/compatibility.rst).
> > > > > >
> > > > > >
> > > > > > Thanks again for the patch; hope the above is constructive
> > > > > > Dave
> > > > > >
> >
>

Since the tests in the PR for rustc_codegen_gcc
(https://github.com/rust-lang/rustc_codegen_gcc/pull/312) currently
fails, let's wait a bit before merging the patch, in case it would need
some fixes.

On Thu, 2023-08-17 at 20:09 +0200, Guillaume Gomez via Jit wrote:
> Quick question: do you plan to make the merge or should I ask Antoni?
> 
> Le jeu. 17 août 2023 à 17:59, Guillaume Gomez
> <guillaume1.gomez@gmail.com>
> a écrit :
> 
> > Thanks for the review!
> > 
> > Le jeu. 17 août 2023 à 17:50, David Malcolm <dmalcolm@redhat.com> a
> > écrit
> > :
> > > 
> > > On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> > > > And now I just discovered that a lot of commits from Antoni's
> > > > fork
> > > > haven't been sent upstream which is why the ABI count is so
> > > > high in
> > > > his repository. Fixed that as well.
> > > 
> > > Thanks for the updated patch; I was about to comment on that.
> > > 
> > > This version is good for gcc trunk.
> > > 
> > > Dave
> > > 
> > > > 
> > > > Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > 
> > > > > Antoni spot a typo I made:
> > > > > 
> > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > > > > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this
> > > > > patch,
> > > > > sorry
> > > > > for the noise.
> > > > > 
> > > > > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > 
> > > > > > Hi Dave,
> > > > > > 
> > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > "make
> > > > > > > check-
> > > > > > > jit" ?  Has this been in use on real Rust code?)
> > > > > > 
> > > > > > I tested it as Rust backend directly on this code:
> > > > > > 
> > > > > > ```
> > > > > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > > > >     *a += *c;
> > > > > >     *b += *c;
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > I ran it with `rustc` (and the GCC backend) with the
> > > > > > following
> > > > > > flags:
> > > > > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which
> > > > > > gave the
> > > > > > diff
> > > > > > you can see in the attached file. Explanations: the diff on
> > > > > > the
> > > > > > right
> > > > > > has the `__restrict__` attribute used whereas on the left
> > > > > > it is
> > > > > > the
> > > > > > current version where we don't handle it.
> > > > > > 
> > > > > > As for C testing, I used this code:
> > > > > > 
> > > > > > ```
> > > > > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > *__restrict__ c) {
> > > > > >     *a += *c;
> > > > > >     *b += *c;
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > (without the `__restrict__` of course when I need to have a
> > > > > > witness
> > > > > > ASM). I attached the diff as well, this time the file with
> > > > > > the
> > > > > > use of
> > > > > > `__restrict__` in on the left. I compiled with the
> > > > > > following
> > > > > > flags:
> > > > > > `-S -O3`.
> > > > > > 
> > > > > > > Please add a feature macro:
> > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > (see the similar ones in the header).
> > > > > > 
> > > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended
> > > > > > the
> > > > > > documentation as well to mention the ABI change.
> > > > > > 
> > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > than
> > > > > > > adding this
> > > > > > > to ABI_0.
> > > > > > 
> > > > > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the
> > > > > > last
> > > > > > one.
> > > > > > 
> > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > copy-
> > > > > > > and-
> > > > > > > paste from a different test case?
> > > > > > 
> > > > > > It is a vestige indeed... Missed this one.
> > > > > > 
> > > > > > > I see that the test scans the generated assembler.  Does
> > > > > > > the
> > > > > > > test
> > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > another
> > > > > > > vestige from a different test case?
> > > > > > 
> > > > > > No, this time it's what I wanted. Please see the C diff I
> > > > > > provided
> > > > > > above to see that the ASM has a small diff that allowed me
> > > > > > to
> > > > > > confirm
> > > > > > that the `__restrict__` attribute was correctly set.
> > > > > > 
> > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > part of
> > > > > > > test-
> > > > > > > combination.c, please add a comment about it to
> > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > alphabetical
> > > > > > > place).
> > > > > > 
> > > > > > Below `-O3`, this ASM difference doesn't appear
> > > > > > unfortunately.
> > > > > > 
> > > > > > > The patch also needs to add documentation for the new
> > > > > > > entrypoint (in
> > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > topics/compatibility.rst).
> > > > > > 
> > > > > > Added!
> > > > > > 
> > > > > > > Thanks again for the patch; hope the above is
> > > > > > > constructive
> > > > > > 
> > > > > > It was incredibly useful! Thanks for taking time to writing
> > > > > > down
> > > > > > the
> > > > > > explanations.
> > > > > > 
> > > > > > The new patch is attached to this email.
> > > > > > 
> > > > > > Cordially.
> > > > > > 
> > > > > > Le jeu. 17 août 2023 à 01:06, David Malcolm
> > > > > > <dmalcolm@redhat.com>
> > > > > > a écrit :
> > > > > > > 
> > > > > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via
> > > > > > > Jit
> > > > > > > wrote:
> > > > > > > > My apologies, forgot to run the commit checkers. Here's
> > > > > > > > the
> > > > > > > > commit
> > > > > > > > with the errors fixed.
> > > > > > > > 
> > > > > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > > > > 
> > > > > > > > > Hi,
> > > > > > > 
> > > > > > > Hi Guillaume, thanks for the patch.
> > > > > > > 
> > > > > > > > > 
> > > > > > > > > This patch adds the possibility to specify the
> > > > > > > > > __restrict__
> > > > > > > > > attribute
> > > > > > > > > for function parameters. It is used by the Rust GCC
> > > > > > > > > backend.
> > > > > > > 
> > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > "make
> > > > > > > check-
> > > > > > > jit" ?  Has this been in use on real Rust code?)
> > > > > > > 
> > > > > > > Overall, this patch looks close to being ready, but some
> > > > > > > nits
> > > > > > > below...
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > > > > --- a/gcc/jit/libgccjit.h
> > > > > > > > +++ b/gcc/jit/libgccjit.h
> > > > > > > > @@ -635,6 +635,10 @@ 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", get type "restrict T".  */
> > > > > > > > +extern gcc_jit_type *
> > > > > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > > > > +
> > > > > > > >  #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > > > > 
> > > > > > > >  /* Given types LTYPE and RTYPE, return non-zero if
> > > > > > > > they are
> > > > > > > compatible.
> > > > > > > 
> > > > > > > Please add a feature macro:
> > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > (see the similar ones in the header).
> > > > > > > 
> > > > > > > > diff --git a/gcc/jit/libgccjit.map
> > > > > > > > b/gcc/jit/libgccjit.map
> > > > > > > > index e52de0057a5..b7289b13845 100644
> > > > > > > > --- a/gcc/jit/libgccjit.map
> > > > > > > > +++ b/gcc/jit/libgccjit.map
> > > > > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > > > >      gcc_jit_type_as_object;
> > > > > > > >      gcc_jit_type_get_const;
> > > > > > > >      gcc_jit_type_get_pointer;
> > > > > > > > +    gcc_jit_type_get_restrict;
> > > > > > > >      gcc_jit_type_get_volatile;
> > > > > > > 
> > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > than
> > > > > > > adding this
> > > > > > > to ABI_0.
> > > > > > > 
> > > > > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > new file mode 100644
> > > > > > > > index 00000000000..4c8c4407f91
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.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 cold
> > > > > > > > +      attribute affects the optimizations. */
> > > > > > > 
> > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > copy-
> > > > > > > and-
> > > > > > > paste from a different test case?
> > > > > > > 
> > > > > > > I see that the test scans the generated assembler.  Does
> > > > > > > the
> > > > > > > test
> > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > another
> > > > > > > vestige from a different test case?
> > > > > > > 
> > > > > > > > +#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);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +#define TEST_COMPILING_TO_FILE
> > > > > > > > +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > > > > +#define OUTPUT_FILENAME  "output-of-test-restrict.c.s"
> > > > > > > > +#include "harness.h"
> > > > > > > > +
> > > > > > > > +void
> > > > > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > > > > +{
> > > > > > > > +     /* Let's try to inject the equivalent of:
> > > > > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > > *__restrict__
> > > > > > > c) {
> > > > > > > > +     *a += *c;
> > > > > > > > +     *b += *c;
> > > > > > > > +}
> > > > > > > > +     */
> > > > > > > > +     gcc_jit_type *int_type =
> > > > > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > > > > GCC_JIT_TYPE_INT);
> > > > > > > > +     gcc_jit_type *pint_type =
> > > > > > > > gcc_jit_type_get_pointer(int_type);
> > > > > > > > +     gcc_jit_type *pint_restrict_type =
> > > > > > > gcc_jit_type_get_restrict(pint_type);
> > > > > > > > +
> > > > > > > > +     gcc_jit_type *void_type =
> > > > > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > > > > GCC_JIT_TYPE_VOID);
> > > > > > > > +
> > > > > > > > +     gcc_jit_param *a =
> > > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > pint_restrict_type, "a");
> > > > > > > > +     gcc_jit_param *b =
> > > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > pint_restrict_type, "b");
> > > > > > > > +     gcc_jit_param *c =
> > > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > pint_restrict_type, "c");
> > > > > > > > +     gcc_jit_param *params[3] = {a, b, c};
> > > > > > > > +
> > > > > > > > +     gcc_jit_function *func_t =
> > > > > > > > +             gcc_jit_context_new_function (ctxt, NULL,
> > > > > > > > +
> > > > > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > > > > +                                     void_type,
> > > > > > > > +                                     "t",
> > > > > > > > +                                     3, params,
> > > > > > > > +                                     0);
> > > > > > > > +
> > > > > > > > +     gcc_jit_block *block = gcc_jit_function_new_block
> > > > > > > > (func_t,
> > > > > > > NULL);
> > > > > > > > +
> > > > > > > > +     /* *a += *c; */
> > > > > > > > +     gcc_jit_block_add_assignment_op (
> > > > > > > > +             block, NULL,
> > > > > > > > +             gcc_jit_rvalue_dereference
> > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > (a), NULL),
> > > > > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > > > > +                     gcc_jit_rvalue_dereference
> > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > +     /* *b += *c; */
> > > > > > > > +     gcc_jit_block_add_assignment_op (
> > > > > > > > +             block, NULL,
> > > > > > > > +             gcc_jit_rvalue_dereference
> > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > (b), NULL),
> > > > > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > > > > +                     gcc_jit_rvalue_dereference
> > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > +
> > > > > > > > +     gcc_jit_block_end_with_void_return (block, NULL);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/* { dg-final { jit-verify-output-file-was-created ""
> > > > > > > > } } */
> > > > > > > > +/* { dg-final { jit-verify-assembler-output "addl   
> > > > > > > > %eax,
> > > > > > > > (%rdi)
> > > > > > > > +     addl    %eax, (%rsi)" } } */
> > > > > > > > --
> > > > > > > > 2.34.1
> > > > > > > > 
> > > > > > > 
> > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > part of
> > > > > > > test-
> > > > > > > combination.c, please add a comment about it to
> > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > alphabetical
> > > > > > > place).
> > > > > > > 
> > > > > > > The patch also needs to add documentation for the new
> > > > > > > entrypoint (in
> > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > topics/compatibility.rst).
> > > > > > > 
> > > > > > > 
> > > > > > > Thanks again for the patch; hope the above is
> > > > > > > constructive
> > > > > > > Dave
> > > > > > > 
> > > 
> >

After more investigations, it seems like the fault was on our side as
we were calling `gcc_jit_type_get_restrict` on types that weren't
pointers.
I added a check for that in `gcc_jit_type_get_restrict` directly as well.

We will continue our investigation to be sure we didn't miss anything.

Le mar. 22 août 2023 à 17:26, Antoni Boucher <bouanto@zoho.com> a écrit :
>
> Since the tests in the PR for rustc_codegen_gcc
> (https://github.com/rust-lang/rustc_codegen_gcc/pull/312) currently
> fails, let's wait a bit before merging the patch, in case it would need
> some fixes.
>
> On Thu, 2023-08-17 at 20:09 +0200, Guillaume Gomez via Jit wrote:
> > Quick question: do you plan to make the merge or should I ask Antoni?
> >
> > Le jeu. 17 août 2023 à 17:59, Guillaume Gomez
> > <guillaume1.gomez@gmail.com>
> > a écrit :
> >
> > > Thanks for the review!
> > >
> > > Le jeu. 17 août 2023 à 17:50, David Malcolm <dmalcolm@redhat.com> a
> > > écrit
> > > :
> > > >
> > > > On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> > > > > And now I just discovered that a lot of commits from Antoni's
> > > > > fork
> > > > > haven't been sent upstream which is why the ABI count is so
> > > > > high in
> > > > > his repository. Fixed that as well.
> > > >
> > > > Thanks for the updated patch; I was about to comment on that.
> > > >
> > > > This version is good for gcc trunk.
> > > >
> > > > Dave
> > > >
> > > > >
> > > > > Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > >
> > > > > > Antoni spot a typo I made:
> > > > > >
> > > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > > > > > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this
> > > > > > patch,
> > > > > > sorry
> > > > > > for the noise.
> > > > > >
> > > > > > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > >
> > > > > > > Hi Dave,
> > > > > > >
> > > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > > "make
> > > > > > > > check-
> > > > > > > > jit" ?  Has this been in use on real Rust code?)
> > > > > > >
> > > > > > > I tested it as Rust backend directly on this code:
> > > > > > >
> > > > > > > ```
> > > > > > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > > > > >     *a += *c;
> > > > > > >     *b += *c;
> > > > > > > }
> > > > > > > ```
> > > > > > >
> > > > > > > I ran it with `rustc` (and the GCC backend) with the
> > > > > > > following
> > > > > > > flags:
> > > > > > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which
> > > > > > > gave the
> > > > > > > diff
> > > > > > > you can see in the attached file. Explanations: the diff on
> > > > > > > the
> > > > > > > right
> > > > > > > has the `__restrict__` attribute used whereas on the left
> > > > > > > it is
> > > > > > > the
> > > > > > > current version where we don't handle it.
> > > > > > >
> > > > > > > As for C testing, I used this code:
> > > > > > >
> > > > > > > ```
> > > > > > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > *__restrict__ c) {
> > > > > > >     *a += *c;
> > > > > > >     *b += *c;
> > > > > > > }
> > > > > > > ```
> > > > > > >
> > > > > > > (without the `__restrict__` of course when I need to have a
> > > > > > > witness
> > > > > > > ASM). I attached the diff as well, this time the file with
> > > > > > > the
> > > > > > > use of
> > > > > > > `__restrict__` in on the left. I compiled with the
> > > > > > > following
> > > > > > > flags:
> > > > > > > `-S -O3`.
> > > > > > >
> > > > > > > > Please add a feature macro:
> > > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > > (see the similar ones in the header).
> > > > > > >
> > > > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended
> > > > > > > the
> > > > > > > documentation as well to mention the ABI change.
> > > > > > >
> > > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > > than
> > > > > > > > adding this
> > > > > > > > to ABI_0.
> > > > > > >
> > > > > > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the
> > > > > > > last
> > > > > > > one.
> > > > > > >
> > > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > > copy-
> > > > > > > > and-
> > > > > > > > paste from a different test case?
> > > > > > >
> > > > > > > It is a vestige indeed... Missed this one.
> > > > > > >
> > > > > > > > I see that the test scans the generated assembler.  Does
> > > > > > > > the
> > > > > > > > test
> > > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > > another
> > > > > > > > vestige from a different test case?
> > > > > > >
> > > > > > > No, this time it's what I wanted. Please see the C diff I
> > > > > > > provided
> > > > > > > above to see that the ASM has a small diff that allowed me
> > > > > > > to
> > > > > > > confirm
> > > > > > > that the `__restrict__` attribute was correctly set.
> > > > > > >
> > > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > > part of
> > > > > > > > test-
> > > > > > > > combination.c, please add a comment about it to
> > > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > > alphabetical
> > > > > > > > place).
> > > > > > >
> > > > > > > Below `-O3`, this ASM difference doesn't appear
> > > > > > > unfortunately.
> > > > > > >
> > > > > > > > The patch also needs to add documentation for the new
> > > > > > > > entrypoint (in
> > > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > > topics/compatibility.rst).
> > > > > > >
> > > > > > > Added!
> > > > > > >
> > > > > > > > Thanks again for the patch; hope the above is
> > > > > > > > constructive
> > > > > > >
> > > > > > > It was incredibly useful! Thanks for taking time to writing
> > > > > > > down
> > > > > > > the
> > > > > > > explanations.
> > > > > > >
> > > > > > > The new patch is attached to this email.
> > > > > > >
> > > > > > > Cordially.
> > > > > > >
> > > > > > > Le jeu. 17 août 2023 à 01:06, David Malcolm
> > > > > > > <dmalcolm@redhat.com>
> > > > > > > a écrit :
> > > > > > > >
> > > > > > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via
> > > > > > > > Jit
> > > > > > > > wrote:
> > > > > > > > > My apologies, forgot to run the commit checkers. Here's
> > > > > > > > > the
> > > > > > > > > commit
> > > > > > > > > with the errors fixed.
> > > > > > > > >
> > > > > > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > > > > >
> > > > > > > > > > Hi,
> > > > > > > >
> > > > > > > > Hi Guillaume, thanks for the patch.
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > This patch adds the possibility to specify the
> > > > > > > > > > __restrict__
> > > > > > > > > > attribute
> > > > > > > > > > for function parameters. It is used by the Rust GCC
> > > > > > > > > > backend.
> > > > > > > >
> > > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > > "make
> > > > > > > > check-
> > > > > > > > jit" ?  Has this been in use on real Rust code?)
> > > > > > > >
> > > > > > > > Overall, this patch looks close to being ready, but some
> > > > > > > > nits
> > > > > > > > below...
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > > > > > --- a/gcc/jit/libgccjit.h
> > > > > > > > > +++ b/gcc/jit/libgccjit.h
> > > > > > > > > @@ -635,6 +635,10 @@ 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", get type "restrict T".  */
> > > > > > > > > +extern gcc_jit_type *
> > > > > > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > > > > > +
> > > > > > > > >  #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > > > > >
> > > > > > > > >  /* Given types LTYPE and RTYPE, return non-zero if
> > > > > > > > > they are
> > > > > > > > compatible.
> > > > > > > >
> > > > > > > > Please add a feature macro:
> > > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > > (see the similar ones in the header).
> > > > > > > >
> > > > > > > > > diff --git a/gcc/jit/libgccjit.map
> > > > > > > > > b/gcc/jit/libgccjit.map
> > > > > > > > > index e52de0057a5..b7289b13845 100644
> > > > > > > > > --- a/gcc/jit/libgccjit.map
> > > > > > > > > +++ b/gcc/jit/libgccjit.map
> > > > > > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > > > > >      gcc_jit_type_as_object;
> > > > > > > > >      gcc_jit_type_get_const;
> > > > > > > > >      gcc_jit_type_get_pointer;
> > > > > > > > > +    gcc_jit_type_get_restrict;
> > > > > > > > >      gcc_jit_type_get_volatile;
> > > > > > > >
> > > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > > than
> > > > > > > > adding this
> > > > > > > > to ABI_0.
> > > > > > > >
> > > > > > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > > new file mode 100644
> > > > > > > > > index 00000000000..4c8c4407f91
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.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 cold
> > > > > > > > > +      attribute affects the optimizations. */
> > > > > > > >
> > > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > > copy-
> > > > > > > > and-
> > > > > > > > paste from a different test case?
> > > > > > > >
> > > > > > > > I see that the test scans the generated assembler.  Does
> > > > > > > > the
> > > > > > > > test
> > > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > > another
> > > > > > > > vestige from a different test case?
> > > > > > > >
> > > > > > > > > +#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);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +#define TEST_COMPILING_TO_FILE
> > > > > > > > > +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > > > > > +#define OUTPUT_FILENAME  "output-of-test-restrict.c.s"
> > > > > > > > > +#include "harness.h"
> > > > > > > > > +
> > > > > > > > > +void
> > > > > > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > > > > > +{
> > > > > > > > > +     /* Let's try to inject the equivalent of:
> > > > > > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > > > *__restrict__
> > > > > > > > c) {
> > > > > > > > > +     *a += *c;
> > > > > > > > > +     *b += *c;
> > > > > > > > > +}
> > > > > > > > > +     */
> > > > > > > > > +     gcc_jit_type *int_type =
> > > > > > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > > > > > GCC_JIT_TYPE_INT);
> > > > > > > > > +     gcc_jit_type *pint_type =
> > > > > > > > > gcc_jit_type_get_pointer(int_type);
> > > > > > > > > +     gcc_jit_type *pint_restrict_type =
> > > > > > > > gcc_jit_type_get_restrict(pint_type);
> > > > > > > > > +
> > > > > > > > > +     gcc_jit_type *void_type =
> > > > > > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > > > > > GCC_JIT_TYPE_VOID);
> > > > > > > > > +
> > > > > > > > > +     gcc_jit_param *a =
> > > > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > pint_restrict_type, "a");
> > > > > > > > > +     gcc_jit_param *b =
> > > > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > pint_restrict_type, "b");
> > > > > > > > > +     gcc_jit_param *c =
> > > > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > pint_restrict_type, "c");
> > > > > > > > > +     gcc_jit_param *params[3] = {a, b, c};
> > > > > > > > > +
> > > > > > > > > +     gcc_jit_function *func_t =
> > > > > > > > > +             gcc_jit_context_new_function (ctxt, NULL,
> > > > > > > > > +
> > > > > > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > > > > > +                                     void_type,
> > > > > > > > > +                                     "t",
> > > > > > > > > +                                     3, params,
> > > > > > > > > +                                     0);
> > > > > > > > > +
> > > > > > > > > +     gcc_jit_block *block = gcc_jit_function_new_block
> > > > > > > > > (func_t,
> > > > > > > > NULL);
> > > > > > > > > +
> > > > > > > > > +     /* *a += *c; */
> > > > > > > > > +     gcc_jit_block_add_assignment_op (
> > > > > > > > > +             block, NULL,
> > > > > > > > > +             gcc_jit_rvalue_dereference
> > > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > > (a), NULL),
> > > > > > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > > > > > +                     gcc_jit_rvalue_dereference
> > > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > > +     /* *b += *c; */
> > > > > > > > > +     gcc_jit_block_add_assignment_op (
> > > > > > > > > +             block, NULL,
> > > > > > > > > +             gcc_jit_rvalue_dereference
> > > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > > (b), NULL),
> > > > > > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > > > > > +                     gcc_jit_rvalue_dereference
> > > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > > +
> > > > > > > > > +     gcc_jit_block_end_with_void_return (block, NULL);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +/* { dg-final { jit-verify-output-file-was-created ""
> > > > > > > > > } } */
> > > > > > > > > +/* { dg-final { jit-verify-assembler-output "addl
> > > > > > > > > %eax,
> > > > > > > > > (%rdi)
> > > > > > > > > +     addl    %eax, (%rsi)" } } */
> > > > > > > > > --
> > > > > > > > > 2.34.1
> > > > > > > > >
> > > > > > > >
> > > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > > part of
> > > > > > > > test-
> > > > > > > > combination.c, please add a comment about it to
> > > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > > alphabetical
> > > > > > > > place).
> > > > > > > >
> > > > > > > > The patch also needs to add documentation for the new
> > > > > > > > entrypoint (in
> > > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > > topics/compatibility.rst).
> > > > > > > >
> > > > > > > >
> > > > > > > > Thanks again for the patch; hope the above is
> > > > > > > > constructive
> > > > > > > > Dave
> > > > > > > >
> > > >
> > >
>

We finished the investigation and found out the issue: when passing
arguments by value to functions, rustc still provides "NoAlias" as
attribute to the argument whereas it should never be passed in this
case. Luckily for us, in case the argument is a function pointer
coming from a struct field, it crashes GCC, which is what allowed us
to figure out about this. A code which reproduces this bug:

```
use std::mem;

#[repr(C)]
pub struct Buffer {
    data: *mut u8,
    len: usize,
    drop: extern "C" fn(Buffer),
}

impl Default for Buffer {
    #[inline]
    fn default() -> Self {
        Self::from(vec![])
    }
}

impl Buffer {
    #[inline]
    pub fn new() -> Self {
        Self::default()
    }

    #[inline]
    pub fn push(&mut self, v: u8) {}
}

impl From<Vec<u8>> for Buffer {
    fn from(mut v: Vec<u8>) -> Self {
        let (data, len) = (v.as_mut_ptr(), v.len());
        mem::forget(v);

        extern "C" fn drop(b: Buffer) {}

        Buffer { data, len, drop }
    }
}

fn main() {
    let mut buffer = Buffer::new();
}
```

So in short: the patch in the previous mail which added this check:

```
RETURN_NULL_IF_FAIL (type->is_pointer (), NULL, NULL, "not a pointer type");
```

is correct and ready. We fixed the bug on our side in
https://github.com/rust-lang/rustc_codegen_gcc/pull/312.

For more information about the "ByValue" attributes issue, there were
some discussions about it in LLVM:
 * https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171218/512066.html
 * https://reviews.llvm.org/D40118


Le ven. 25 août 2023 à 22:47, Guillaume Gomez
<guillaume1.gomez@gmail.com> a écrit :
>
> After more investigations, it seems like the fault was on our side as
> we were calling `gcc_jit_type_get_restrict` on types that weren't
> pointers.
> I added a check for that in `gcc_jit_type_get_restrict` directly as well.
>
> We will continue our investigation to be sure we didn't miss anything.
>
> Le mar. 22 août 2023 à 17:26, Antoni Boucher <bouanto@zoho.com> a écrit :
> >
> > Since the tests in the PR for rustc_codegen_gcc
> > (https://github.com/rust-lang/rustc_codegen_gcc/pull/312) currently
> > fails, let's wait a bit before merging the patch, in case it would need
> > some fixes.
> >
> > On Thu, 2023-08-17 at 20:09 +0200, Guillaume Gomez via Jit wrote:
> > > Quick question: do you plan to make the merge or should I ask Antoni?
> > >
> > > Le jeu. 17 août 2023 à 17:59, Guillaume Gomez
> > > <guillaume1.gomez@gmail.com>
> > > a écrit :
> > >
> > > > Thanks for the review!
> > > >
> > > > Le jeu. 17 août 2023 à 17:50, David Malcolm <dmalcolm@redhat.com> a
> > > > écrit
> > > > :
> > > > >
> > > > > On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> > > > > > And now I just discovered that a lot of commits from Antoni's
> > > > > > fork
> > > > > > haven't been sent upstream which is why the ABI count is so
> > > > > > high in
> > > > > > his repository. Fixed that as well.
> > > > >
> > > > > Thanks for the updated patch; I was about to comment on that.
> > > > >
> > > > > This version is good for gcc trunk.
> > > > >
> > > > > Dave
> > > > >
> > > > > >
> > > > > > Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > >
> > > > > > > Antoni spot a typo I made:
> > > > > > >
> > > > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > > > > > > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this
> > > > > > > patch,
> > > > > > > sorry
> > > > > > > for the noise.
> > > > > > >
> > > > > > > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > > >
> > > > > > > > Hi Dave,
> > > > > > > >
> > > > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > > > "make
> > > > > > > > > check-
> > > > > > > > > jit" ?  Has this been in use on real Rust code?)
> > > > > > > >
> > > > > > > > I tested it as Rust backend directly on this code:
> > > > > > > >
> > > > > > > > ```
> > > > > > > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > > > > > >     *a += *c;
> > > > > > > >     *b += *c;
> > > > > > > > }
> > > > > > > > ```
> > > > > > > >
> > > > > > > > I ran it with `rustc` (and the GCC backend) with the
> > > > > > > > following
> > > > > > > > flags:
> > > > > > > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which
> > > > > > > > gave the
> > > > > > > > diff
> > > > > > > > you can see in the attached file. Explanations: the diff on
> > > > > > > > the
> > > > > > > > right
> > > > > > > > has the `__restrict__` attribute used whereas on the left
> > > > > > > > it is
> > > > > > > > the
> > > > > > > > current version where we don't handle it.
> > > > > > > >
> > > > > > > > As for C testing, I used this code:
> > > > > > > >
> > > > > > > > ```
> > > > > > > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > > *__restrict__ c) {
> > > > > > > >     *a += *c;
> > > > > > > >     *b += *c;
> > > > > > > > }
> > > > > > > > ```
> > > > > > > >
> > > > > > > > (without the `__restrict__` of course when I need to have a
> > > > > > > > witness
> > > > > > > > ASM). I attached the diff as well, this time the file with
> > > > > > > > the
> > > > > > > > use of
> > > > > > > > `__restrict__` in on the left. I compiled with the
> > > > > > > > following
> > > > > > > > flags:
> > > > > > > > `-S -O3`.
> > > > > > > >
> > > > > > > > > Please add a feature macro:
> > > > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > > > (see the similar ones in the header).
> > > > > > > >
> > > > > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended
> > > > > > > > the
> > > > > > > > documentation as well to mention the ABI change.
> > > > > > > >
> > > > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > > > than
> > > > > > > > > adding this
> > > > > > > > > to ABI_0.
> > > > > > > >
> > > > > > > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the
> > > > > > > > last
> > > > > > > > one.
> > > > > > > >
> > > > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > > > copy-
> > > > > > > > > and-
> > > > > > > > > paste from a different test case?
> > > > > > > >
> > > > > > > > It is a vestige indeed... Missed this one.
> > > > > > > >
> > > > > > > > > I see that the test scans the generated assembler.  Does
> > > > > > > > > the
> > > > > > > > > test
> > > > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > > > another
> > > > > > > > > vestige from a different test case?
> > > > > > > >
> > > > > > > > No, this time it's what I wanted. Please see the C diff I
> > > > > > > > provided
> > > > > > > > above to see that the ASM has a small diff that allowed me
> > > > > > > > to
> > > > > > > > confirm
> > > > > > > > that the `__restrict__` attribute was correctly set.
> > > > > > > >
> > > > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > > > part of
> > > > > > > > > test-
> > > > > > > > > combination.c, please add a comment about it to
> > > > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > > > alphabetical
> > > > > > > > > place).
> > > > > > > >
> > > > > > > > Below `-O3`, this ASM difference doesn't appear
> > > > > > > > unfortunately.
> > > > > > > >
> > > > > > > > > The patch also needs to add documentation for the new
> > > > > > > > > entrypoint (in
> > > > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > > > topics/compatibility.rst).
> > > > > > > >
> > > > > > > > Added!
> > > > > > > >
> > > > > > > > > Thanks again for the patch; hope the above is
> > > > > > > > > constructive
> > > > > > > >
> > > > > > > > It was incredibly useful! Thanks for taking time to writing
> > > > > > > > down
> > > > > > > > the
> > > > > > > > explanations.
> > > > > > > >
> > > > > > > > The new patch is attached to this email.
> > > > > > > >
> > > > > > > > Cordially.
> > > > > > > >
> > > > > > > > Le jeu. 17 août 2023 à 01:06, David Malcolm
> > > > > > > > <dmalcolm@redhat.com>
> > > > > > > > a écrit :
> > > > > > > > >
> > > > > > > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via
> > > > > > > > > Jit
> > > > > > > > > wrote:
> > > > > > > > > > My apologies, forgot to run the commit checkers. Here's
> > > > > > > > > > the
> > > > > > > > > > commit
> > > > > > > > > > with the errors fixed.
> > > > > > > > > >
> > > > > > > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > > > > > >
> > > > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > Hi Guillaume, thanks for the patch.
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > This patch adds the possibility to specify the
> > > > > > > > > > > __restrict__
> > > > > > > > > > > attribute
> > > > > > > > > > > for function parameters. It is used by the Rust GCC
> > > > > > > > > > > backend.
> > > > > > > > >
> > > > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > > > "make
> > > > > > > > > check-
> > > > > > > > > jit" ?  Has this been in use on real Rust code?)
> > > > > > > > >
> > > > > > > > > Overall, this patch looks close to being ready, but some
> > > > > > > > > nits
> > > > > > > > > below...
> > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > > > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > > > > > > --- a/gcc/jit/libgccjit.h
> > > > > > > > > > +++ b/gcc/jit/libgccjit.h
> > > > > > > > > > @@ -635,6 +635,10 @@ 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", get type "restrict T".  */
> > > > > > > > > > +extern gcc_jit_type *
> > > > > > > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > > > > > > +
> > > > > > > > > >  #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > > > > > >
> > > > > > > > > >  /* Given types LTYPE and RTYPE, return non-zero if
> > > > > > > > > > they are
> > > > > > > > > compatible.
> > > > > > > > >
> > > > > > > > > Please add a feature macro:
> > > > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > > > (see the similar ones in the header).
> > > > > > > > >
> > > > > > > > > > diff --git a/gcc/jit/libgccjit.map
> > > > > > > > > > b/gcc/jit/libgccjit.map
> > > > > > > > > > index e52de0057a5..b7289b13845 100644
> > > > > > > > > > --- a/gcc/jit/libgccjit.map
> > > > > > > > > > +++ b/gcc/jit/libgccjit.map
> > > > > > > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > > > > > >      gcc_jit_type_as_object;
> > > > > > > > > >      gcc_jit_type_get_const;
> > > > > > > > > >      gcc_jit_type_get_pointer;
> > > > > > > > > > +    gcc_jit_type_get_restrict;
> > > > > > > > > >      gcc_jit_type_get_volatile;
> > > > > > > > >
> > > > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > > > than
> > > > > > > > > adding this
> > > > > > > > > to ABI_0.
> > > > > > > > >
> > > > > > > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 00000000000..4c8c4407f91
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.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 cold
> > > > > > > > > > +      attribute affects the optimizations. */
> > > > > > > > >
> > > > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > > > copy-
> > > > > > > > > and-
> > > > > > > > > paste from a different test case?
> > > > > > > > >
> > > > > > > > > I see that the test scans the generated assembler.  Does
> > > > > > > > > the
> > > > > > > > > test
> > > > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > > > another
> > > > > > > > > vestige from a different test case?
> > > > > > > > >
> > > > > > > > > > +#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);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +#define TEST_COMPILING_TO_FILE
> > > > > > > > > > +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > > > > > > +#define OUTPUT_FILENAME  "output-of-test-restrict.c.s"
> > > > > > > > > > +#include "harness.h"
> > > > > > > > > > +
> > > > > > > > > > +void
> > > > > > > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > > > > > > +{
> > > > > > > > > > +     /* Let's try to inject the equivalent of:
> > > > > > > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > > > > *__restrict__
> > > > > > > > > c) {
> > > > > > > > > > +     *a += *c;
> > > > > > > > > > +     *b += *c;
> > > > > > > > > > +}
> > > > > > > > > > +     */
> > > > > > > > > > +     gcc_jit_type *int_type =
> > > > > > > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > > > > > > GCC_JIT_TYPE_INT);
> > > > > > > > > > +     gcc_jit_type *pint_type =
> > > > > > > > > > gcc_jit_type_get_pointer(int_type);
> > > > > > > > > > +     gcc_jit_type *pint_restrict_type =
> > > > > > > > > gcc_jit_type_get_restrict(pint_type);
> > > > > > > > > > +
> > > > > > > > > > +     gcc_jit_type *void_type =
> > > > > > > > > > +             gcc_jit_context_get_type (ctxt,
> > > > > > > > > > GCC_JIT_TYPE_VOID);
> > > > > > > > > > +
> > > > > > > > > > +     gcc_jit_param *a =
> > > > > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > > pint_restrict_type, "a");
> > > > > > > > > > +     gcc_jit_param *b =
> > > > > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > > pint_restrict_type, "b");
> > > > > > > > > > +     gcc_jit_param *c =
> > > > > > > > > > +             gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > > pint_restrict_type, "c");
> > > > > > > > > > +     gcc_jit_param *params[3] = {a, b, c};
> > > > > > > > > > +
> > > > > > > > > > +     gcc_jit_function *func_t =
> > > > > > > > > > +             gcc_jit_context_new_function (ctxt, NULL,
> > > > > > > > > > +
> > > > > > > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > > > > > > +                                     void_type,
> > > > > > > > > > +                                     "t",
> > > > > > > > > > +                                     3, params,
> > > > > > > > > > +                                     0);
> > > > > > > > > > +
> > > > > > > > > > +     gcc_jit_block *block = gcc_jit_function_new_block
> > > > > > > > > > (func_t,
> > > > > > > > > NULL);
> > > > > > > > > > +
> > > > > > > > > > +     /* *a += *c; */
> > > > > > > > > > +     gcc_jit_block_add_assignment_op (
> > > > > > > > > > +             block, NULL,
> > > > > > > > > > +             gcc_jit_rvalue_dereference
> > > > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > > > (a), NULL),
> > > > > > > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > > > > > > +                     gcc_jit_rvalue_dereference
> > > > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > > > +     /* *b += *c; */
> > > > > > > > > > +     gcc_jit_block_add_assignment_op (
> > > > > > > > > > +             block, NULL,
> > > > > > > > > > +             gcc_jit_rvalue_dereference
> > > > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > > > (b), NULL),
> > > > > > > > > > +             GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > > > +             gcc_jit_lvalue_as_rvalue (
> > > > > > > > > > +                     gcc_jit_rvalue_dereference
> > > > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > > > +
> > > > > > > > > > +     gcc_jit_block_end_with_void_return (block, NULL);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +/* { dg-final { jit-verify-output-file-was-created ""
> > > > > > > > > > } } */
> > > > > > > > > > +/* { dg-final { jit-verify-assembler-output "addl
> > > > > > > > > > %eax,
> > > > > > > > > > (%rdi)
> > > > > > > > > > +     addl    %eax, (%rsi)" } } */
> > > > > > > > > > --
> > > > > > > > > > 2.34.1
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > > > part of
> > > > > > > > > test-
> > > > > > > > > combination.c, please add a comment about it to
> > > > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > > > alphabetical
> > > > > > > > > place).
> > > > > > > > >
> > > > > > > > > The patch also needs to add documentation for the new
> > > > > > > > > entrypoint (in
> > > > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > > > topics/compatibility.rst).
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks again for the patch; hope the above is
> > > > > > > > > constructive
> > > > > > > > > Dave
> > > > > > > > >
> > > > >
> > > >
> >

On Tue, 2023-08-29 at 17:15 +0200, Guillaume Gomez wrote:
> We finished the investigation and found out the issue: when passing
> arguments by value to functions, rustc still provides "NoAlias" as
> attribute to the argument whereas it should never be passed in this
> case. Luckily for us, in case the argument is a function pointer
> coming from a struct field, it crashes GCC, which is what allowed us
> to figure out about this. A code which reproduces this bug:

[...snip...]

> So in short: the patch in the previous mail which added this check:
> 
> ```
> RETURN_NULL_IF_FAIL (type->is_pointer (), NULL, NULL, "not a pointer
> type");
> ```
> 
> is correct and ready. 

Thanks.  I've gone ahead and pushed it to gcc trunk for you as r14-
3552-g29763b002459cb.

[...snip...]

Dave

Thanks a lot!

Le mar. 29 août 2023 à 17:35, David Malcolm <dmalcolm@redhat.com> a écrit :
>
> On Tue, 2023-08-29 at 17:15 +0200, Guillaume Gomez wrote:
> > We finished the investigation and found out the issue: when passing
> > arguments by value to functions, rustc still provides "NoAlias" as
> > attribute to the argument whereas it should never be passed in this
> > case. Luckily for us, in case the argument is a function pointer
> > coming from a struct field, it crashes GCC, which is what allowed us
> > to figure out about this. A code which reproduces this bug:
>
> [...snip...]
>
> > So in short: the patch in the previous mail which added this check:
> >
> > ```
> > RETURN_NULL_IF_FAIL (type->is_pointer (), NULL, NULL, "not a pointer
> > type");
> > ```
> >
> > is correct and ready.
>
> Thanks.  I've gone ahead and pushed it to gcc trunk for you as r14-
> 3552-g29763b002459cb.
>
> [...snip...]
>
> Dave
>