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 D78E53858C74 for ; Tue, 9 Jan 2024 19:59:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D78E53858C74 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 D78E53858C74 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=1704830364; cv=none; b=iq3pTNAWqq1COy0i+lvISNUEx+HDRjJHNzEdCHNAq6TRPAio60Phvz4clf7fzVzXCQJB3kH7CldyXViYLJkrMzVwp05BWtu43RmwBEZruAV0eW/zNnORGd70TGQSCnDanjJvkLegXN+Af2QgZoxliEJJiud9UgTXn1/BRgwd8C8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704830364; c=relaxed/simple; bh=dbiUpsQ1nULPYE8XTJ3dfK7f35F3GTxGzwZFFt3tx8c=; h=DKIM-Signature:Message-ID:Subject:From:To:Date:MIME-Version; b=L46XTfaQiPIiOG9TIAQPHHWmO3ypMvdY4iHOo8MqpCqWpXNN9GLQn6V4ityfHPaJ6Pd+yGIRoCUeGX9ytQ4mOqXbIHQ3Aq1R/TMvgTPho/+1wCuq/bLQxirwJ9h3gw9B7MoLlDo6FtPq0gtRQq1jgc+e8G4krvFQrgwYVCGzFq0= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1704830361; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IhHeV7dr6LFXyOtA6DwYMbl5/jPWhbfuB4uHWA05810=; b=DlPkMumjmmC4/eTjmBflN+2JZJdOJggcgISCkeNwawlj/LuIi2JtyVw6vV7KK3+FPTjWAe D7LgieMEzjCWjyYTV4/bGNENxK0mc+ZwxjgUaguGh1OKjilbm2NwLkbN5mIiuDwLWngLVl tEWkeLUvpD7y3Hmpu9agy5/Z45GDqVw= Received: from mail-yb1-f199.google.com (mail-yb1-f199.google.com [209.85.219.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-523--r3ZopYgNdiZclKdkczciA-1; Tue, 09 Jan 2024 14:59:19 -0500 X-MC-Unique: -r3ZopYgNdiZclKdkczciA-1 Received: by mail-yb1-f199.google.com with SMTP id 3f1490d57ef6-dbe053d5d91so4170648276.2 for ; Tue, 09 Jan 2024 11:59:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704830358; x=1705435158; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:to:from:subject:message-id:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=D7oVYUxfefta6MRfiTDwS5Y3YhsWZHDUMqtgFy3HEY0=; b=rSVjwvdaCjvkuqffFdUv/uCgugWVyB296uWYqykn2wda+r4gttRUlM8kObsGG6YRIQ AtBIy3yf3KIwCEHV2pOUQU/364lY+zN3JckHda+cy8ex8kt0c4cVWbgIRxCbJ4WIdQP6 Oum/FJmHHchART9zNn23K3bDT7i45lGycP7ixO9VBqfiy5n8gc24MN7eQvNsFqN2f9+3 Tia7REe+r7eNTxl32UK4CSm/GL/Exfk8p0exvqYN1Ejsia1vQZ3GCV0SI/+MSYGtmEOk D93BjxMkwSllzl3ANofUWQjmsKnkggyEfWucL/Ja8yi3PvH50TeLBtOL1SfyOkEV4ePZ ITsA== X-Gm-Message-State: AOJu0Yw95lp+oCWiBJo479oyrTVKNSIua32cPlRXxDWseslF09h1LuNL 5I8lA2I/37G+tICTTVaa8N3/Llvenx08krATsJLnW4AyQ6K5/WGIx3OfiNmYgQy0MuJ0QzPs7Ao aboCUCi5QL0VVfUo= X-Received: by 2002:a25:5f48:0:b0:dbe:d379:fcea with SMTP id h8-20020a255f48000000b00dbed379fceamr3452968ybm.1.1704830358429; Tue, 09 Jan 2024 11:59:18 -0800 (PST) X-Google-Smtp-Source: AGHT+IE+qFnDqKg47JCQ4thFC4W5hsN9F78dUb/j7JS7qQqMG27iQZqOclV2qgVZjr+KuwPyd81UkQ== X-Received: by 2002:a25:5f48:0:b0:dbe:d379:fcea with SMTP id h8-20020a255f48000000b00dbed379fceamr3452960ybm.1.1704830357952; Tue, 09 Jan 2024 11:59:17 -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 f15-20020ac8470f000000b00427e0e9c22dsm1142265qtp.54.2024.01.09.11.59.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 09 Jan 2024 11:59:17 -0800 (PST) Message-ID: <319a998a3c63eaf0e28d7267a901ad2c016dba49.camel@redhat.com> Subject: Re: [PATCH] Add support for function attributes and variable attributes From: David Malcolm To: Guillaume Gomez , jit@gcc.gnu.org, Antoni , gcc-patches@gcc.gnu.org Date: Tue, 09 Jan 2024 14:59:16 -0500 In-Reply-To: References: 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=-15.2 required=5.0 tests=BAYES_00,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=unavailable 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 Wed, 2023-11-15 at 17:53 +0100, Guillaume Gomez wrote: > Hi, >=20 > This patch adds the (incomplete) support for function and variable > attributes. The added attributes are the ones we're using in > rustc_codegen_gcc but all the groundwork is done to add more (and we > will very likely add more as we didn't add all the ones we use in > rustc_codegen_gcc yet). >=20 > The only big question with this patch is about `inline`. We currently > handle it as an attribute because it is more convenient for us but is > it ok or should we create a separate function to mark a function as > inlined? >=20 > Thanks in advance for the review. Thanks for the patch; sorry for the delay in reviewing. At a high-level I think the API is OK as-is, but I have some nitpicks with the implementation: [...snip...] > diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.rs= t > index d8c1d15d69d..6c72c99cbd9 100644 > --- a/gcc/jit/docs/topics/types.rst > +++ b/gcc/jit/docs/topics/types.rst [...snip...] > +.. function:: void\ > + gcc_jit_lvalue_add_string_attribute (gcc_jit_lvalue *vari= able, > + enum gcc_jit_fn_attr= ibute attribute, ^^ This got out of sync with the declaration in the header file; it should be enum gcc_jit_variable_attribute attribute [...snip...] > diff --git a/gcc/jit/dummy-frontend.cc b/gcc/jit/dummy-frontend.cc > index a729086bafb..898b4d6e7f8 100644 > --- a/gcc/jit/dummy-frontend.cc > +++ b/gcc/jit/dummy-frontend.cc It's unfortunate that jit/dummy-frontend.cc has its own copy of the material in c-common/c-attribs.cc. I glanced through this code, and it seems that there are already various differences between the two copies in the existing code, and the patch adds more such differences. Bother - but I think this part of the patch is inevitable (and OK) given the existing state of attribute handling here. [...snip...] 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 rustcc, right? [..snip...] > diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc > index 0451b4df7f9..337d4ea3b95 100644 > --- a/gcc/jit/libgccjit.cc > +++ b/gcc/jit/libgccjit.cc > @@ -3965,6 +3965,51 @@ gcc_jit_type_get_aligned (gcc_jit_type *type, > return (gcc_jit_type *)type->get_aligned (alignment_in_bytes); > } > =20 > +void > +gcc_jit_function_add_attribute (gcc_jit_function *func, > +=09=09=09=09gcc_jit_fn_attribute attribute) > +{ > + RETURN_IF_FAIL (func, NULL, NULL, "NULL func"); > + > + func->add_attribute (attribute); Ideally should validate parameter "attribute" here with a RETURN_IF_FAIL. > +} > + > +void > +gcc_jit_function_add_string_attribute (gcc_jit_function *func, > +=09=09=09=09 gcc_jit_fn_attribute attribute, > +=09=09=09=09 const char* value) > +{ > + RETURN_IF_FAIL (func, NULL, NULL, "NULL func"); Likewise, ideally should validate parameter "attribute" here with a RETURN_IF_FAIL. Can "value" be NULL? If not, then we should add a RETURN_IF_FAIL for it here at the API boundary. > + > + func->add_string_attribute (attribute, value); > +} > + > +/* This function adds an attribute with multiple integer values. For ex= ample > + `nonnull(1, 2)`. The numbers in `values` are supposed to map how the= y > + should be written in C code. So for `nonnull(1, 2)`, you should pass= `1` > + and `2` in `values` (and set `length` to `2`). */ > +void > +gcc_jit_function_add_integer_array_attribute (gcc_jit_function *func, > +=09=09=09=09=09 gcc_jit_fn_attribute attribute, > +=09=09=09=09=09 const int* values, > +=09=09=09=09=09 size_t length) > +{ > + RETURN_IF_FAIL (func, NULL, NULL, "NULL func"); As before, ideally should validate parameter "attribute" here with a RETURN_IF_FAIL. > + RETURN_IF_FAIL (values, NULL, NULL, "NULL values"); > + > + func->add_integer_array_attribute (attribute, values, length); > +} > + > +void > +gcc_jit_lvalue_add_string_attribute (gcc_jit_lvalue *variable, > +=09=09=09=09 gcc_jit_variable_attribute attribute, > +=09=09=09=09 const char* value) > +{ > + RETURN_IF_FAIL (variable, NULL, NULL, "NULL variable"); As before, we should validate parameters "attribute" and "value" here with RETURN_IF_FAILs. We should also validate here that "variable" is indeed a variable, not some arbitrary lvalue e.g. the address of the element of an array (or whatever). > + > + variable->add_string_attribute (attribute, value); > +} > + [...snip...] > diff --git a/gcc/testsuite/jit.dg/jit.exp b/gcc/testsuite/jit.dg/jit.exp > index 8bf7e51c24f..657b454a003 100644 > --- a/gcc/testsuite/jit.dg/jit.exp > +++ b/gcc/testsuite/jit.dg/jit.exp > @@ -899,8 +899,41 @@ proc jit-verify-assembler-output { args } { > =09pass "${asm_filename} output pattern test, ${dg-output-text}" > =09verbose "Passed test for output pattern ${dg-output-text}" 3 > } > +} > + > +# Assuming that a .s file has been written out named > +# OUTPUT_FILENAME, check that the argument doesn't match > +# the output file. > +proc jit-verify-assembler-output-not { args } { > + verbose "jit-verify-assembler: $args" > + > + set dg-output-text [lindex $args 0] > + verbose "dg-output-text: ${dg-output-text}" > + > + upvar 2 name name > + verbose "name: $name" > + > + upvar 2 prog prog > + verbose "prog: $prog" > + set asm_filename [jit-get-output-filename $prog] > + verbose " asm_filename: ${asm_filename}" > =20 > + # Read the assembly file. > + set f [open $asm_filename r] > + set content [read $f] > + close $f > + > + # Verify that the assembly matches the regex. > + if { [regexp ${dg-output-text} $content] } { > +=09fail "${asm_filename} output pattern test, is ${content}, should matc= h ${dg-output-text}" The wording of the "fail" message seems wrong; presumably this should read "should not match", rather than "should match". > +=09verbose "Failed test for output pattern ${dg-output-text}" 3 > + } else { > +=09pass "${asm_filename} output pattern test, ${dg-output-text}" > +=09verbose "Passed test for output pattern ${dg-output-text}" 3 > + } > } > + > + > # Assuming that a .o file has been written out named > # OUTPUT_FILENAME, invoke the driver to try to turn it into > # an executable, and try to run the result. [...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. */ The above comment doesn't make sense to me; harness.h effectively sets -O3; and -O2 is wanted by this test, right? I think the comment can be omitted given that the intent below is clear. > +#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. */ Again, this comment doesn't make sense to me, but I think it can be removed. > +#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...] > + > + /* if (x >>=3D 1) */ > + /* Since gccjit doesn't (yet?) have support for `>>=3D` operator, we w= ill decompose it into: > + `if (x =3D x >> 1)` */ I think it does (in theory, at least), via: gcc_jit_block_add_assignment_op with GCC_JIT_BINARY_OP_RSHIFT But I haven't tried it, and there's no need to update the test to make use of it. [...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..84933e60010 > --- /dev/null > +++ b/gcc/testsuite/jit.dg/test-noinline-attribute.c > @@ -0,0 +1,114 @@ > +/* { 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. */ Again, please lose this 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. */ Likewise. > +#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. */ Likewise. > +#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...] 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. 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. Thanks again for the patch. Dave