From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 98ABB386F44D for ; Thu, 11 Jan 2024 18:46:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 98ABB386F44D Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 98ABB386F44D Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704998784; cv=none; b=iLlxZw9f9eQbE5l1aC/+Tzjczc1izKWhzdjeI3VFxCmFD2AiKdqkzfKY7zVDnhP5QsCxJHCq9YC0DtBdHdehWUaxTpMFSMPnuGzcHn9sRnJMIz4e4DRCSGzBJyWWPa1vOujtZVfQ6Nho73AuOietsqjhNVVeIxknXeBcz7EUE6k= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704998784; c=relaxed/simple; bh=GHPOvU6OJV6skb2HtbzDgGD3DmElqXawz7huLyvniSY=; h=DKIM-Signature:Message-ID:Subject:From:To:Date:MIME-Version; b=Xy5pgPxXS9jOnJIxB2rli4tHWL8qRs997zT7hbElZwdNP9Ms2IDHudhRubonWyaqGfcIqmRvSyHmEOJLyIS3aUZPj/AD6p5QWP3EIiiJ2zEjbZZSrPW2XlhvkR5/NWQhulsQlUPFnB5YS/KeYEi9Ie/4beT5dAiXS6pX4Q1Cycw= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1704998782; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=U4z1+rWGH7v3d0hsv6fOCn+Nbvmw5/W8lvMgw3NtQH0=; b=XCPyjXmtmYb2jDPygVyw8Zct8tFkZ/BkGtudUrQLtOaO4MXXH3lr/+FY/e2sDeBNLxone7 Icmp/7tt16QjF46koUE6FvSmrRnl6hpZDy3kdzX6K9JKeEoqcbApSuKjPZtOXsKaMKcfxv 4h+c7ND6jWqSt8ypEZlS+6Aq82T21Nk= Received: from mail-yb1-f198.google.com (mail-yb1-f198.google.com [209.85.219.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-511-O9NItO6KOiGPX_4kVNZXDQ-1; Thu, 11 Jan 2024 13:46:20 -0500 X-MC-Unique: O9NItO6KOiGPX_4kVNZXDQ-1 Received: by mail-yb1-f198.google.com with SMTP id 3f1490d57ef6-db402e6f61dso7501729276.3 for ; Thu, 11 Jan 2024 10:46:20 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704998780; x=1705603580; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=YNvq+x4hal9XG2oLAwJMuiym3hssd2k6i95p6q3++Wo=; b=JwkU1NysnRKeni0X2Z8TfoXUkQbbUAWCu/rmJidWPDaFe5ieQRUfwzfBbsd4Dc3z6+ fS7PaEixrm/D/f9kIpgX6jFH6CwpFP2GAcdYZADgNPccG5hHEf/hyYbDqVZDmD6CRnZ3 n5ZcXsRCxkCf0VL+ZZGrdRLlTM4ileEeSi+UVJ4m8cLk6JXtnjImegQjqIH+pmB/s9rd +MpKgAKJvRAVBhdD1nIIDU58XwD+djQ837erOLnVdynkEMPrO6Lz2lCG78naXqTUtzyN qvU926GyvkZJuqhE96O3YbDu4zyMWjTvH1agDlJtdYyGLvsRdGvRuCprRr2OJodB//Ri fQAQ== X-Gm-Message-State: AOJu0YycevvpGRdQc2zYD7TgPeCwTemELKCkHWJDb7dhd8TnBHxPIhaj /lHI3XKhE6j9X5a/XEUtmSXS0Q8d2vfpnwe9Mh8G3zhCj9NtGBhip0pxNrTZjuqT7lZBx3MmJiM uREeA1yZN7v/tuOM= X-Received: by 2002:a25:83c8:0:b0:dbe:328a:3d9b with SMTP id v8-20020a2583c8000000b00dbe328a3d9bmr78793ybm.17.1704998780064; Thu, 11 Jan 2024 10:46:20 -0800 (PST) X-Google-Smtp-Source: AGHT+IF/XqQjRENQerYWIMwWbxua6HMePtrgEY7eb0BSbi6HCfD00b/vNXWWcLpphmvPx1UKLosYuw== X-Received: by 2002:a25:83c8:0:b0:dbe:328a:3d9b with SMTP id v8-20020a2583c8000000b00dbe328a3d9bmr78786ybm.17.1704998779658; Thu, 11 Jan 2024 10:46:19 -0800 (PST) Received: from t14s.localdomain (c-76-28-97-5.hsd1.ma.comcast.net. [76.28.97.5]) by smtp.gmail.com with ESMTPSA id ie4-20020a05622a698400b00429943beea4sm632212qtb.89.2024.01.11.10.46.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 11 Jan 2024 10:46:19 -0800 (PST) Message-ID: Subject: Re: [PATCH] Add support for function attributes and variable attributes From: David Malcolm To: Guillaume Gomez Cc: jit@gcc.gnu.org, Antoni , gcc-patches@gcc.gnu.org Date: Thu, 11 Jan 2024 13:46:18 -0500 In-Reply-To: References: <319a998a3c63eaf0e28d7267a901ad2c016dba49.camel@redhat.com> User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-13.7 required=5.0 tests=BAYES_00,BODY_8BITS,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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: On Thu, 2024-01-11 at 01:00 +0100, Guillaume Gomez wrote: > Hi David. >=20 > Thanks for the review! >=20 > > > +.. function::=C2=A0 void\ > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 gcc_jit_lvalue_add_string_attribute > > > (gcc_jit_lvalue *variable, > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 enum > > > gcc_jit_fn_attribute attribute, > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 > > ^^ > >=20 > > This got out of sync with the declaration in the header file; it > > should > > be enum gcc_jit_variable_attribute attribute >=20 > Indeed, good catch! >=20 > > I took a brief look through the handler functions and with the > > above > > caveat I didn't see anything obviously wrong.=C2=A0 I'm going to assume > > this > > code is OK given that presumably you've been testing it within > > rustc, > > right? >=20 > Both in rustc and in the JIT tests we added. >=20 > [..snip...] >=20 > 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. >=20 > [..snip...] >=20 > I removed the tests comments as you mentioned. >=20 > > 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. >=20 > It's not always correctly sorted. Might be worth sending a patch > after this > one gets merged to fix that. >=20 > > 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. >=20 > I added them with a comment on top of each of them. >=20 > I joined the new patch version. >=20 > 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.rs= t > 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 > =20 > #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 fo= r > + 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_at= tribute attribute, > + const char *value) > + > + Add a string attribute ``attribute`` with value ``value`` to a func= tion > + ``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 fo= r > + its presence using > + > + .. code-block:: c > + > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES > + > +.. function:: void\ > + gcc_jit_function_add_integer_array_attribute (gcc_jit_fun= ction *func, > + enum gcc_ji= t_fn_attribute attribute, > + const int *= value, > + size_t leng= th) > + > + Add an attribute ``attribute`` with ``length`` integer values ``val= ue`` 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 fo= r > + its presence using > + > + .. code-block:: c > + > + #ifdef LIBGCCJIT_HAVE_ATTRIBUTES > + > +.. function:: void\ > + gcc_jit_lvalue_add_string_attribute (gcc_jit_lvalue *vari= able, > + enum gcc_jit_variabl= e_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 fo= r > + 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 > =20 > -/* 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/j= it.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 c= old > + 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_L= EVEL, 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 c= onst > + 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_L= EVEL, 3); > +} [...snip...] > diff --git a/gcc/testsuite/jit.dg/test-noinline-attribute.c b/gcc/testsui= te/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_L= EVEL, 2); > +} [...snip...] > diff --git a/gcc/testsuite/jit.dg/test-nonnull-attribute.c b/gcc/testsuit= e/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 n= onnull > + 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_L= EVEL, 2); > +} [...snip...] > diff --git a/gcc/testsuite/jit.dg/test-pure-attribute.c b/gcc/testsuite/j= it.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 p= ure > + 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_L= EVEL, 3); > +} > + [...snip...] > diff --git a/gcc/testsuite/jit.dg/test-restrict-attribute.c b/gcc/testsui= te/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 r= estrict > +=09 attribute affects the optimizations. */ Please delete this comment. > +#define TEST_ESCHEWS_SET_OPTIONS > +static void set_options (gcc_jit_context *ctxt, const char *argv0) > +{ > +=09// Set "-O3". > +=09gcc_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