From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) by sourceware.org (Postfix) with ESMTPS id 68F5D3858401; Fri, 12 Jan 2024 10:09:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 68F5D3858401 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 68F5D3858401 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::333 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705054200; cv=none; b=bEUR4c7MFX6IWmSbx/e7bRA1MGC1FTZUCnZt1vehZBU8udcKhUtSsbJtmSvyOyO8g5hocnVMVhMefiWS1NVrUhClbJQdB8mczQEXMWQUrcIY4o1vN3iUuaxhShddbDw+H+CriNZc+eUUyra/Qdp34WfYkyG2a8AtgwEGJI7AmMI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705054200; c=relaxed/simple; bh=4jze/MMCzSqaPrXZan592sTpIIeJ/FqfTohm3E94vY4=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=XwOBz0hMOGUnTXZTtdsQEUcP0EnIR8PprRM4JaRMRuxpW7PDfuvA+T3VNyBIQP7m5X5Aw/HT3F4O9MdIcNUupTYHZKh/x0FSb5HRTx76cIU7jHXMs77IZhWCHNkx5cIjekfVt+jxfknK9SQQ/rFvccwZG+TipT3aw2sHhgMXWi8= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-x333.google.com with SMTP id 5b1f17b1804b1-40e60e135a7so13263455e9.0; Fri, 12 Jan 2024 02:09:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1705054196; x=1705658996; 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=p++ID82mfx969FV2KGqF0WMMW8+KOg9thWDN962qepQ=; b=YRZI2rK24oACAQjZvNIJ8zVgkJydyOtOnLM6c8PMld7YZdUeCWs5FFuR6Lbw6WGPa1 EFeuE0EbTWnuVdtlsFZVEDD5Ph1o5JWZm4PyE2HD0llouFk3zD9uFcnsZJvaKfRseAXs jFnKXRakI4ZC3px201LipLXS5RWc5THyVk8pDY+SyASq/pVspP2q+f4yXJlCNb1fLrSO 1xS6lTr+29UwlWBx8VNqAw5YOAwkEhwqRK1+VcHcvhKQBW0HAYgV8anyODt/YeradoL8 sv25ZZiQ/1+/on5cAeBZ9p9iqQvMUmvANNzcwezacVXPe/JxIvzkR2ifRiHIK8CsB37b rsnQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705054196; x=1705658996; 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=p++ID82mfx969FV2KGqF0WMMW8+KOg9thWDN962qepQ=; b=tIfdGt4IQXumtjtu8vsaJyNEBEUOWYHdXojUXSG35zt4ZZ8+QUHNkBmEq39w44Zb49 lDSgs8BcGe+s8cemPsnzEuwXe8/uQu/kKQibAF86wLjna94khQvZ8PRE1hsNu0t/KUPX Vq9ivOF6metW3vRPYRFFZkpKwvcIJsrBSooWrgGxLfHF/whSRU6B3N/xFRSM640eHdSk 4+2goCEbmmfbOkgX3bBU+tSGfuXucW6PUNfAZIDOeqT5IxHcHygM2aidCYjb4wAIH6aD v14+6nhxK4GoVZVUBqDvwKyzCOT+I3+tNldOqyFzuN02Y960QOuPatABWBLO/GCTR/3j 2stQ== X-Gm-Message-State: AOJu0Yysn3Keqq/ipUFrNRZVoYPIAdv5KnKdsKnGockH2fUaRCT3RAhi 4Ie1ezfPJL2eJ1v1yn2uPDQctJKgxMupQ8dGVTJ5LHXS0q5lFw== X-Google-Smtp-Source: AGHT+IH5QtvCGqd8ZyZm/rucC8zkQB8jkSzmTpDXL9dGGeQb7PtmB0ESEgD4RJPgqI38BxEi/HO8q9lFRYxAN7HuIls= X-Received: by 2002:a05:600c:54c8:b0:40d:8380:8648 with SMTP id iw8-20020a05600c54c800b0040d83808648mr660400wmb.66.1705054195729; Fri, 12 Jan 2024 02:09:55 -0800 (PST) MIME-Version: 1.0 References: <319a998a3c63eaf0e28d7267a901ad2c016dba49.camel@redhat.com> <686f67b56fd0a95a6b3ce8d2a60826aa9c446255.camel@redhat.com> In-Reply-To: <686f67b56fd0a95a6b3ce8d2a60826aa9c446255.camel@redhat.com> From: Guillaume Gomez Date: Fri, 12 Jan 2024 11:09:43 +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.9 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: > It sounds like the patch you have locally is ready, but it has some > nontrivial changes compared to the last version you posted to the list. > Please post your latest version to the list. Sure! This patch adds the support for attributes on functions and variables. It d= oes so by adding the following functions: * gcc_jit_function_add_attribute * gcc_jit_function_add_string_attribute * gcc_jit_function_add_integer_array_attribute * gcc_jit_lvalue_add_string_attribute It adds the following types: * gcc_jit_fn_attribute * gcc_jit_variable_attribute It adds tests to ensure that the attributes are correctly applied. > Do you have push rights, or do you need me to push it for you? I have push rights so I'll merge the patch myself. But thanks for offering = to do it. Le jeu. 11 janv. 2024 =C3=A0 23:38, David Malcolm a = =C3=A9crit : > > On Thu, 2024-01-11 at 22:40 +0100, Guillaume Gomez wrote: > > Hi David, > > > > > The above looks correct, but the patch adds the entrypoint > > > descriptions > > > to topics/types.rst, which seems like the wrong place. The > > > function- > > > related ones should be in topics/functions.rst in the "Functions" > > > section and the lvalue/variable one in topics/expression.rst after > > > the > > > "Global variables" section. > > > > Ah indeed. Mix-up on my end. Fixed it. > > > > > test-restrict.c is a pre-existing testcase, so please don't delete > > > its > > > entry. > > > > Ah indeed, I went too quickly and thought it was a test I renamed... > > > > > BTW, the ChangeLog entry mentions adding test-restrict.c, but the > > > patch > > > doesn't add it, so that part of the proposed ChangeLog is wrong. > > > > > > Does the patch pass ./contrib/gcc-changelog/git_check_commit.py ? > > > > I messed up a bit, fixed it thanks to you. I didn't run the script in > > my last > > update but just did: > > > > ``` > > $ contrib/gcc-changelog/git_check_commit.py $(git log -1 --format=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 > > discovered > > that we could just add a call to `asm` to prevent this from happening > > so I > > added it. > > > > So yes, all jit tests are passing as expected. :) > > Good. > > It sounds like the patch you have locally is ready, but it has some > nontrivial changes compared to the last version you posted to the list. > Please post your latest version to the list. > > Do you have push rights, or do you need me to push it for you? > > Thanks > Dave > > > > > Le jeu. 11 janv. 2024 =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_attribute attribute) > > > > + > > > > + Add an attribute ``attribute`` to a function ``func``. > > > > + > > > > + This is equivalent to the following code: > > > > + > > > > + .. code-block:: c > > > > + > > > > + __attribute__((always_inline)) > > > > + > > > > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can > > > > test for > > > > + its presence using > > > > + > > > > + .. code-block:: c > > > > + > > > > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES > > > > + > > > > +.. function:: void\ > > > > + gcc_jit_function_add_string_attribute > > > > (gcc_jit_function *func, > > > > + enum > > > > gcc_jit_fn_attribute attribute, > > > > + const char > > > > *value) > > > > + > > > > + Add a string attribute ``attribute`` with value ``value`` > > > > to a function > > > > + ``func``. > > > > + > > > > + This is equivalent to the following code: > > > > + > > > > + .. code-block:: c > > > > + > > > > + __attribute__ ((alias ("xxx"))) > > > > + > > > > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can > > > > test for > > > > + its presence using > > > > + > > > > + .. code-block:: c > > > > + > > > > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES > > > > + > > > > +.. function:: void\ > > > > + gcc_jit_function_add_integer_array_attribute > > > > (gcc_jit_function *func, > > > > + > > > > enum gcc_jit_fn_attribute attribute, > > > > + > > > > const int *value, > > > > + > > > > size_t length) > > > > + > > > > + Add an attribute ``attribute`` with ``length`` integer > > > > values ``value`` to a > > > > + function ``func``. The integer values must be the same as > > > > you would write > > > > + them in a C code. > > > > + > > > > + This is equivalent to the following code: > > > > + > > > > + .. code-block:: c > > > > + > > > > + __attribute__ ((nonnull (1, 2))) > > > > + > > > > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can > > > > test for > > > > + its presence using > > > > + > > > > + .. code-block:: c > > > > + > > > > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES > > > > + > > > > +.. function:: void\ > > > > + gcc_jit_lvalue_add_string_attribute > > > > (gcc_jit_lvalue *variable, > > > > + enum > > > > gcc_jit_variable_attribute attribute, > > > > + const char > > > > *value) > > > > + > > > > + Add an attribute ``attribute`` with value ``value`` to a > > > > variable ``variable``. > > > > + > > > > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can > > > > test for > > > > + its presence using > > > > + > > > > + .. code-block:: c > > > > + > > > > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES > > > > > > The above looks correct, but the patch adds the entrypoint > > > descriptions > > > to topics/types.rst, which seems like the wrong place. The > > > function- > > > related ones should be in topics/functions.rst in the "Functions" > > > section and the lvalue/variable one in topics/expression.rst after > > > the > > > "Global variables" section. > > > > > > > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > > index e762563f9bd..84001203352 100644 > > > > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h > > > > > > [...snip...] > > > > > > > @@ -313,7 +334,7 @@ > > > > #undef create_code > > > > #undef verify_code > > > > > > > > -/* test-restrict.c: This can't be in the testcases array as it > > > > needs > > > > +/* test-restrict-attribute.c: This can't be in the testcases > > > > array as it needs > > > > the `-O3` flag. */ > > > > > > test-restrict.c is a pre-existing testcase, so please don't delete > > > its > > > entry. > > > BTW, the ChangeLog entry mentions adding test-restrict.c, but the > > > patch > > > doesn't add it, so that part of the proposed ChangeLog is wrong. > > > > > > Does the patch pass ./contrib/gcc-changelog/git_check_commit.py ? > > > > > > [...snip...] > > > > > > > diff --git a/gcc/testsuite/jit.dg/test-cold-attribute.c > > > > b/gcc/testsuite/jit.dg/test-cold-attribute.c > > > > new file mode 100644 > > > > index 00000000000..8dc7ec5a34b > > > > --- /dev/null > > > > +++ b/gcc/testsuite/jit.dg/test-cold-attribute.c > > > > @@ -0,0 +1,54 @@ > > > > +/* { dg-do compile { target x86_64-*-* } } */ > > > > + > > > > +#include > > > > +#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/testsuite/jit.dg/test-const-attribute.c > > > > new file mode 100644 > > > > index 00000000000..c06742d163f > > > > --- /dev/null > > > > +++ b/gcc/testsuite/jit.dg/test-const-attribute.c > > > > @@ -0,0 +1,134 @@ > > > > +/* { dg-do compile { target x86_64-*-* } } */ > > > > + > > > > +#include > > > > +#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/testsuite/jit.dg/test-noinline-attribute.c > > > > new file mode 100644 > > > > index 00000000000..a455b4493fd > > > > --- /dev/null > > > > +++ b/gcc/testsuite/jit.dg/test-noinline-attribute.c > > > > @@ -0,0 +1,121 @@ > > > > +/* { dg-do compile { target x86_64-*-* } } */ > > > > + > > > > +#include > > > > +#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/testsuite/jit.dg/test-nonnull-attribute.c > > > > new file mode 100644 > > > > index 00000000000..3306f890657 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/jit.dg/test-nonnull-attribute.c > > > > @@ -0,0 +1,94 @@ > > > > +/* { dg-do compile { target x86_64-*-* } } */ > > > > + > > > > +#include > > > > +#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/testsuite/jit.dg/test-restrict-attribute.c > > > > new file mode 100644 > > > > index 00000000000..7d7444b624f > > > > --- /dev/null > > > > +++ b/gcc/testsuite/jit.dg/test-restrict-attribute.c > > > > @@ -0,0 +1,77 @@ > > > > +/* { dg-do compile { target x86_64-*-* } } */ > > > > + > > > > +#include > > > > +#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_OPTIMIZATION_LEVEL, 3); > > > > +} > > > > + > > > > > > [...snip...] > > > > > > Otherwise, looks good, assuming that the patch has been tested with > > > the > > > full jit testsuite. > > > > > > Thanks again > > > Dave > > > > > >