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.go...@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