From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x330.google.com (mail-wm1-x330.google.com [IPv6:2a00:1450:4864:20::330]) by sourceware.org (Postfix) with ESMTPS id C123C3858CD1; Thu, 11 Jan 2024 21:40:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C123C3858CD1 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org C123C3858CD1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::330 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705009251; cv=none; b=aFABb6QQkz8ZUqKY3gnOWYkdsVTdSzdIatIy1+peKG7HvHHEEVA2H+vayH39N7oakDaLyUxmj66q2Bfot7jAiY/ys3jZITJwA1d1rnBT7t78PLjY/pIBSj8FBiSaQ7CZT+qtWbcGh9EU2gSzWb6XBAdSlmU9nK2tCsEE3A91lzk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705009251; c=relaxed/simple; bh=5cFNck7594+9cHL5c24oiNH3EMWapte73qkyXXSWylM=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=haAx+84Zfxv2QQSukye/J9J9T4aenMAtQ/SxqpsFb+S3VvYuUnLGILFCdFi7ig/W9fkEzpOO1hqsIQxF4bs+X5CjvA60DRQm0nZYa0aKLMnbxp2H+amN2C98R4eL6hXpAHgMj0Gb7wQaaZtf5vNVSCn2zMgfwW2fObCSZgnxrCU= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-x330.google.com with SMTP id 5b1f17b1804b1-40d5336986cso74588835e9.1; Thu, 11 Jan 2024 13:40:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1705009247; x=1705614047; darn=gcc.gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=7+08USECDxJ1TEqxY0nJ8PJ+rvqUzh+fFHyPwcd/azg=; b=bHyyt9UdV55BvLTa0touZ6VN+tLkda6RsPNpOntrOJvR9olCiBZYNHNwj2el0l3tWm 6KQmUu3cAFz44KEbx5mVfadiFkBVCFUJ6aJvJu7nB1sWf8iWzUpD4Kn29e2lro2QKx4c y1isa5EvT25y1eo7jSNPs3HoldBWKdDgl+tzdYFYJLlJjIfAIuGOqim44Lx5qEsSqlw0 U+igVbC/oJ941LN1mA0MFG2xUIgOpZ98A/IXUliksQJ5PhCWByE4GZtk+jaHu9+60EVJ Ry/Cjj18bJL+2qEiNwye1/57pFnu0RtUnL4g0p6icdld4qkbb8Bnh24RJGnP0HXp0eIL 0oZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705009247; x=1705614047; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=7+08USECDxJ1TEqxY0nJ8PJ+rvqUzh+fFHyPwcd/azg=; b=Ydzv+9pPzTq0115HbTeGuS0AyFiNhMPF4X+d9gXRw/12yUiUKEw3AcRKAgchJ2PXti a/XeYGO0S5SNqftP+8i/hpqm/3SHXcFRzBu3SLZ52rRSrz5u3yEa+ZPPGi1ll0Pkzz1a wss2EUFygCGcHObZ5gMIhmtT4K/OKhsqAhWs/0GFPa1vldngJEiW3KcqcD/Ok1oueF/D 901LA7+swvQ7q5dttZL61PdwfIVWAuhtclMcwe9aE+c4iKG5YaU6m5mB/BBAtnJmX50v WE66b1a5oNQqSpLpXhnGleu6DkXCawufmlG7ZZty/j7ul56z5JPixQ9Vgi2x6VqA/9tI eD0Q== X-Gm-Message-State: AOJu0YwEsDTWoXy2qOAA6xD9dDxekS8EhSrkYbdhUtLAs4DUx8yxu/8Y NJgHIA6dTG6460/a2PcQ19Uxgfv/idQD53OI330= X-Google-Smtp-Source: AGHT+IH7jCV7DDuoDEgsW/INLkVHd0S4mXSuCeeejbx/3RKdnxQDoDeejdgsRXVoxglxTMKYqkJATI0lKUJyXmKOSBY= X-Received: by 2002:a05:600c:1c85:b0:40d:8818:8ea5 with SMTP id k5-20020a05600c1c8500b0040d88188ea5mr217926wms.122.1705009247080; Thu, 11 Jan 2024 13:40:47 -0800 (PST) MIME-Version: 1.0 References: <319a998a3c63eaf0e28d7267a901ad2c016dba49.camel@redhat.com> In-Reply-To: From: Guillaume Gomez Date: Thu, 11 Jan 2024 22:40:35 +0100 Message-ID: Subject: Re: [PATCH] Add support for function attributes and variable attributes To: David Malcolm Cc: jit@gcc.gnu.org, Antoni , gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-8.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi David, > The above looks correct, but the patch adds the entrypoint descriptions > to topics/types.rst, which seems like the wrong place. The function- > related ones should be in topics/functions.rst in the "Functions" > section and the lvalue/variable one in topics/expression.rst after the > "Global variables" section. Ah indeed. Mix-up on my end. Fixed it. > test-restrict.c is a pre-existing testcase, so please don't delete its > entry. Ah indeed, I went too quickly and thought it was a test I renamed... > BTW, the ChangeLog entry mentions adding test-restrict.c, but the patch > doesn't add it, so that part of the proposed ChangeLog is wrong. > > Does the patch pass ./contrib/gcc-changelog/git_check_commit.py ? I messed up a bit, fixed it thanks to you. I didn't run the script in my la= st update but just did: ``` $ contrib/gcc-changelog/git_check_commit.py $(git log -1 --format=3D%h) Checking 3849ee2eadf0eeec2b0080a5142ced00be96a60d: OK ``` > Otherwise, looks good, assuming that the patch has been tested with the > full jit testsuite. When rebasing on upstream yesterday I discovered that two tests were not working anymore. For the first one, it was simply because of the changes in `dummy-frontend.cc`. For the second one (test-noinline-attribute.c), it was because the rules for inlining changed since we wrote this patch apparently (our fork is very late). Antoni discov= ered that we could just add a call to `asm` to prevent this from happening so I added it. So yes, all jit tests are passing as expected. :) Le jeu. 11 janv. 2024 =C3=A0 19:46, David Malcolm a = =C3=A9crit : > > On Thu, 2024-01-11 at 01:00 +0100, Guillaume Gomez wrote: > > Hi David. > > > > Thanks for the review! > > > > > > +.. function:: void\ > > > > + gcc_jit_lvalue_add_string_attribute > > > > (gcc_jit_lvalue *variable, > > > > + enum > > > > gcc_jit_fn_attribute attribute, > > > > > > ^^ > > > > > > This got out of sync with the declaration in the header file; it > > > should > > > be enum gcc_jit_variable_attribute attribute > > > > Indeed, good catch! > > > > > I took a brief look through the handler functions and with the > > > above > > > caveat I didn't see anything obviously wrong. I'm going to assume > > > this > > > code is OK given that presumably you've been testing it within > > > rustc, > > > right? > > > > Both in rustc and in the JIT tests we added. > > > > [..snip...] > > > > I added all the missing `RETURN_IF_FAIL` you mentioned. None of the > > arguments should be `NULL` so it was a mistake not to check it. > > > > [..snip...] > > > > I removed the tests comments as you mentioned. > > > > > Please update jit.dg/all-non-failing-tests.h for the new tests; > > > it's > > > meant to list all of the (non failing) tests alphabetically. > > > > It's not always correctly sorted. Might be worth sending a patch > > after this > > one gets merged to fix that. > > > > > I *think* all of the new tests aren't suitable to be run as part of > > > a > > > shared context (e.g. due to touching the optimization level or > > > examining generated asm), so they should be listed in that header > > > with > > > comments explaining why. > > > > I added them with a comment on top of each of them. > > > > I joined the new patch version. > > > > Thanks again for the review! > > Thanks for the updated patch. I noticed a few minor issues: > > > diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.= rst > > index bb51f037b7e..b1aedc03787 100644 > > --- a/gcc/jit/docs/topics/types.rst > > +++ b/gcc/jit/docs/topics/types.rst > > @@ -553,3 +553,80 @@ Reflection API > > .. code-block:: c > > > > #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_restrict > > + > > +.. function:: void\ > > + gcc_jit_function_add_attribute (gcc_jit_function *func, > > + enum gcc_jit_fn_attribu= te attribute) > > + > > + Add an attribute ``attribute`` to a function ``func``. > > + > > + This is equivalent to the following code: > > + > > + .. code-block:: c > > + > > + __attribute__((always_inline)) > > + > > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test = for > > + its presence using > > + > > + .. code-block:: c > > + > > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES > > + > > +.. function:: void\ > > + gcc_jit_function_add_string_attribute (gcc_jit_function= *func, > > + enum gcc_jit_fn_= attribute attribute, > > + const char *valu= e) > > + > > + Add a string attribute ``attribute`` with value ``value`` to a fu= nction > > + ``func``. > > + > > + This is equivalent to the following code: > > + > > + .. code-block:: c > > + > > + __attribute__ ((alias ("xxx"))) > > + > > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test = for > > + its presence using > > + > > + .. code-block:: c > > + > > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES > > + > > +.. function:: void\ > > + gcc_jit_function_add_integer_array_attribute (gcc_jit_f= unction *func, > > + enum gcc_= jit_fn_attribute attribute, > > + const int= *value, > > + size_t le= ngth) > > + > > + Add an attribute ``attribute`` with ``length`` integer values ``v= alue`` to a > > + function ``func``. The integer values must be the same as you wou= ld write > > + them in a C code. > > + > > + This is equivalent to the following code: > > + > > + .. code-block:: c > > + > > + __attribute__ ((nonnull (1, 2))) > > + > > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test = for > > + its presence using > > + > > + .. code-block:: c > > + > > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES > > + > > +.. function:: void\ > > + gcc_jit_lvalue_add_string_attribute (gcc_jit_lvalue *va= riable, > > + enum gcc_jit_varia= ble_attribute attribute, > > + const char *value) > > + > > + Add an attribute ``attribute`` with value ``value`` to a variable= ``variable``. > > + > > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test = for > > + its presence using > > + > > + .. code-block:: c > > + > > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES > > The above looks correct, but the patch adds the entrypoint descriptions > to topics/types.rst, which seems like the wrong place. The function- > related ones should be in topics/functions.rst in the "Functions" > section and the lvalue/variable one in topics/expression.rst after the > "Global variables" section. > > > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h > b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > index e762563f9bd..84001203352 100644 > > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h > > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > [...snip...] > > > @@ -313,7 +334,7 @@ > > #undef create_code > > #undef verify_code > > > > -/* test-restrict.c: This can't be in the testcases array as it needs > > +/* test-restrict-attribute.c: This can't be in the testcases array as = it needs > > the `-O3` flag. */ > > test-restrict.c is a pre-existing testcase, so please don't delete its > entry. > BTW, the ChangeLog entry mentions adding test-restrict.c, but the patch > doesn't add it, so that part of the proposed ChangeLog is wrong. > > Does the patch pass ./contrib/gcc-changelog/git_check_commit.py ? > > [...snip...] > > > diff --git a/gcc/testsuite/jit.dg/test-cold-attribute.c b/gcc/testsuite= /jit.dg/test-cold-attribute.c > > new file mode 100644 > > index 00000000000..8dc7ec5a34b > > --- /dev/null > > +++ b/gcc/testsuite/jit.dg/test-cold-attribute.c > > @@ -0,0 +1,54 @@ > > +/* { dg-do compile { target x86_64-*-* } } */ > > + > > +#include > > +#include > > + > > +#include "libgccjit.h" > > + > > +/* We don't want set_options() in harness.h to set -O2 to see that the= cold > > + attribute affects the optimizations. */ > > Please delete the above comment. > > > +#define TEST_ESCHEWS_SET_OPTIONS > > +static void set_options (gcc_jit_context *ctxt, const char *argv0) > > +{ > > + // Set "-O2". > > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION= _LEVEL, 2); > > +} > > [...snip...] > > > diff --git a/gcc/testsuite/jit.dg/test-const-attribute.c b/gcc/testsuit= e/jit.dg/test-const-attribute.c > > new file mode 100644 > > index 00000000000..c06742d163f > > --- /dev/null > > +++ b/gcc/testsuite/jit.dg/test-const-attribute.c > > @@ -0,0 +1,134 @@ > > +/* { dg-do compile { target x86_64-*-* } } */ > > + > > +#include > > +#include > > + > > +#include "libgccjit.h" > > + > > +/* We don't want set_options() in harness.h to set -O3 to see that the= const > > + attribute affects the optimizations. */ > > Please delete the above comment. > > > +#define TEST_ESCHEWS_SET_OPTIONS > > +static void set_options (gcc_jit_context *ctxt, const char *argv0) > > +{ > > + // Set "-O3". > > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION= _LEVEL, 3); > > +} > > [...snip...] > > > diff --git a/gcc/testsuite/jit.dg/test-noinline-attribute.c b/gcc/tests= uite/jit.dg/test-noinline-attribute.c > > new file mode 100644 > > index 00000000000..a455b4493fd > > --- /dev/null > > +++ b/gcc/testsuite/jit.dg/test-noinline-attribute.c > > @@ -0,0 +1,121 @@ > > +/* { dg-do compile { target x86_64-*-* } } */ > > + > > +#include > > +#include > > + > > +#include "libgccjit.h" > > + > > +/* We don't want set_options() in harness.h to set -O2 to see that the= `noinline` > > + attribute affects the optimizations. */ > > Please delete the above comment. > > > +#define TEST_ESCHEWS_SET_OPTIONS > > +static void set_options (gcc_jit_context *ctxt, const char *argv0) > > +{ > > + // Set "-O2". > > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION= _LEVEL, 2); > > +} > > [...snip...] > > > diff --git a/gcc/testsuite/jit.dg/test-nonnull-attribute.c b/gcc/testsu= ite/jit.dg/test-nonnull-attribute.c > > new file mode 100644 > > index 00000000000..3306f890657 > > --- /dev/null > > +++ b/gcc/testsuite/jit.dg/test-nonnull-attribute.c > > @@ -0,0 +1,94 @@ > > +/* { dg-do compile { target x86_64-*-* } } */ > > + > > +#include > > +#include > > + > > +#include "libgccjit.h" > > + > > +/* We don't want set_options() in harness.h to set -O2 to see that the= nonnull > > + attribute affects the optimizations. */ > > Please delete the above comment. > > > > +#define TEST_ESCHEWS_SET_OPTIONS > > +static void set_options (gcc_jit_context *ctxt, const char *argv0) > > +{ > > + // Set "-O2". > > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION= _LEVEL, 2); > > +} > > [...snip...] > > > diff --git a/gcc/testsuite/jit.dg/test-pure-attribute.c b/gcc/testsuite= /jit.dg/test-pure-attribute.c > > new file mode 100644 > > index 00000000000..0c9ba1366e0 > > --- /dev/null > > +++ b/gcc/testsuite/jit.dg/test-pure-attribute.c > > @@ -0,0 +1,134 @@ > > +/* { dg-do compile { target x86_64-*-* } } */ > > + > > +#include > > +#include > > + > > +#include "libgccjit.h" > > + > > +/* We don't want set_options() in harness.h to set -O3 to see that the= pure > > + attribute affects the optimizations. */ > > Please delete the above comment. > > > +#define TEST_ESCHEWS_SET_OPTIONS > > +static void set_options (gcc_jit_context *ctxt, const char *argv0) > > +{ > > + // Set "-O3". > > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION= _LEVEL, 3); > > +} > > + > > [...snip...] > > > diff --git a/gcc/testsuite/jit.dg/test-restrict-attribute.c b/gcc/tests= uite/jit.dg/test-restrict-attribute.c > > new file mode 100644 > > index 00000000000..7d7444b624f > > --- /dev/null > > +++ b/gcc/testsuite/jit.dg/test-restrict-attribute.c > > @@ -0,0 +1,77 @@ > > +/* { dg-do compile { target x86_64-*-* } } */ > > + > > +#include > > +#include > > + > > +#include "libgccjit.h" > > + > > +/* We don't want set_options() in harness.h to set -O3 to see that the= restrict > > + attribute affects the optimizations. */ > > Please delete this comment. > > > +#define TEST_ESCHEWS_SET_OPTIONS > > +static void set_options (gcc_jit_context *ctxt, const char *argv0) > > +{ > > + // Set "-O3". > > + gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZAT= ION_LEVEL, 3); > > +} > > + > > [...snip...] > > Otherwise, looks good, assuming that the patch has been tested with the > full jit testsuite. > > Thanks again > Dave >