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 DDF403858C83 for ; Fri, 8 Apr 2022 19:01:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DDF403858C83 Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-267-pHPzSnEkN6mgqUIIiDNgVA-1; Fri, 08 Apr 2022 15:01:40 -0400 X-MC-Unique: pHPzSnEkN6mgqUIIiDNgVA-1 Received: by mail-qk1-f197.google.com with SMTP id w200-20020a3762d1000000b0067d2149318dso4181643qkb.1 for ; Fri, 08 Apr 2022 12:01:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=CvS04ou9GIGj4GF2eUjvx94Ctr7DxkB+tu/OssxeCIk=; b=n4P8nBMt4Sc/2FYqtLuOouAKZlXPytopLX0TqJgcwnh1Wl2v4qnmDjPqORLSdoEx7J CDsWSyeRZkufGXNKNcYSwmVYXg1wYC+KpbFvDwLNeICaLYhwj5qvHq3SvvZqNIj4KG4I k5tSJ+osQLZ9yR85QdZSD71Rp/wZZPwLXbZJbS9soEwv4ajLaZoT56w2kUxYV1rI39S0 sLUijZ+gMqZzTR2wJpBPKvFZgJ1EmWCtBankTtEpMeQGfvknh/DXW8zGK0AQNYAtUbkr G2ml5r9IjzhvQ2CJIhC/IXyBrNXLsYf2I02/eUERKwImD9vBdJPLVuzi8dtdQEmNnZ8Y bBaA== X-Gm-Message-State: AOAM530bzuRqlmZxXgURZyi0vfR/tsFosxJ1rB6mc9JzGAJktnNkG3+Q 0MCnKDhF+X63cTHcPvgEIu+pSYezVOAvSTZJE3tTDqRTR0w+vP52i1VSyLYrFlv5dn55QaqV9ER IqIi9IHw= X-Received: by 2002:a05:6214:238b:b0:430:842c:1863 with SMTP id fw11-20020a056214238b00b00430842c1863mr17323800qvb.105.1649444499608; Fri, 08 Apr 2022 12:01:39 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwW+iBZt/2bouuSOV7Kzd8/JA5CMwqx1srrUrqGe41+ALUoox65OUMRkrGJKlbR1N8IdCKBIQ== X-Received: by 2002:a05:6214:238b:b0:430:842c:1863 with SMTP id fw11-20020a056214238b00b00430842c1863mr17323751qvb.105.1649444499111; Fri, 08 Apr 2022 12:01:39 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.ma.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id 21-20020ac85715000000b002e1ce9605ffsm19363543qtw.65.2022.04.08.12.01.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 08 Apr 2022 12:01:38 -0700 (PDT) Message-ID: <1a077e6549786d9aedb1cf353f176b0207520c6e.camel@redhat.com> Subject: Re: [PATCH] libgccjit: Add support for setting the alignment [PR104293] From: David Malcolm To: Antoni Boucher , gcc-patches@gcc.gnu.org, jit@gcc.gnu.org Date: Fri, 08 Apr 2022 15:01:37 -0400 In-Reply-To: <7f32bff12738484e3f5d97cfc481996fd7570b01.camel@zoho.com> References: <7f32bff12738484e3f5d97cfc481996fd7570b01.camel@zoho.com> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.9 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_LOW, 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.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Apr 2022 19:01:43 -0000 On Sun, 2022-01-30 at 20:38 -0500, Antoni Boucher via Gcc-patches wrote: > Hi. > This patch adds support for setting the alignment of variables in > libgccjit. Thanks. Sorry about the delay in reviewing it. > > I was wondering if I should change it so that it takes/returns bytes > instead of bits. > What do you think? I'm not sure, but given that C refers to bytes for this: https://en.cppreference.com/w/c/language/object#Alignment https://en.cppreference.com/w/c/language/_Alignof ...I think bytes is the better choice, to maximize similarity with C. Does anything support/need a fraction-of-a-byte alignment? If so, then bits would be the way to go. > diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst > index 16cebe31a10..1957399dceb 100644 > --- a/gcc/jit/docs/topics/compatibility.rst > +++ b/gcc/jit/docs/topics/compatibility.rst > @@ -302,3 +302,13 @@ thread-local storage model of a variable: > section of a variable: > > * :func:`gcc_jit_lvalue_set_link_section` > + > +.. _LIBGCCJIT_ABI_24: > + > +``LIBGCCJIT_ABI_24`` > +----------------------- > +``LIBGCCJIT_ABI_24`` covers the addition of functions to get and set the > +alignement of a variable: > + > + * :func:`gcc_jit_lvalue_set_alignment` > + * :func:`gcc_jit_lvalue_get_alignment` > diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst > index 791a20398ca..0f5f5376d8c 100644 > --- a/gcc/jit/docs/topics/expressions.rst > +++ b/gcc/jit/docs/topics/expressions.rst > @@ -738,6 +738,45 @@ where the rvalue is computed by reading from the storage area. > > #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_link_section > > +.. function:: void > + gcc_jit_lvalue_set_alignment (gcc_jit_lvalue *lvalue, > + int alignment) > + > + Set the alignment of a variable. > + The parameter ``alignment`` is in bits. Analogous to: > + > + .. code-block:: c > + > + int variable __attribute__((aligned (16))); > + > + in C, but in bits instead of bytes. If we're doing it in bytes, this will need updating, of course. Maybe rename the int param from "alignment" to "bytes" to make this clearer. Probably should be unsigned as well. > + > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_24`; you can test for > + its presence using > + > + .. code-block:: c > + > + #ifdef LIBGCCJIT_HAVE_ALIGNMENT > + > +.. function:: int > + gcc_jit_lvalue_get_alignment (gcc_jit_lvalue *lvalue) > + > + Return the alignment of a variable set by ``gcc_jit_lvalue_set_alignment``, in bits. > + Return 0 if the alignment was not set. Analogous to: > + > + .. code-block:: c > + > + _Alignof (variable) > + > + in C, but in bits instead of bytes. Likewise this will need updating. > + > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_24`; you can test for > + its presence using > + > + .. code-block:: c > + > + #ifdef LIBGCCJIT_HAVE_ALIGNMENT > + > Global variables > **************** > [...snip...] > diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc > index 4c352e8c93d..e03f15ec9c8 100644 > --- a/gcc/jit/libgccjit.cc > +++ b/gcc/jit/libgccjit.cc > @@ -2649,6 +2649,31 @@ gcc_jit_lvalue_set_link_section (gcc_jit_lvalue *lvalue, > lvalue->set_link_section (section_name); > } > > +/* Public entrypoint. See description in libgccjit.h. > + > + After error-checking, the real work is done by the > + gcc::jit::recording::lvalue::get_link_section method in jit-recording.cc. */ Comment refers to wrong function. > + > +int > +gcc_jit_lvalue_get_alignment (gcc_jit_lvalue *lvalue) > +{ > + RETURN_VAL_IF_FAIL (lvalue, 0, NULL, NULL, "NULL lvalue"); > + return lvalue->get_alignment (); > +} Should this return unsigned? > + > +/* Public entrypoint. See description in libgccjit.h. > + > + After error-checking, the real work is done by the > + gcc::jit::recording::lvalue::set_alignment method in jit-recording.cc. */ > + > +void > +gcc_jit_lvalue_set_alignment (gcc_jit_lvalue *lvalue, > + int alignment) > +{ > + RETURN_IF_FAIL (lvalue, NULL, NULL, "NULL lvalue"); Should the alignment be unsigned? What if the user passes in negative? Does it have to be a power of two? If so, ideally we should reject non-power-of-two here. > + lvalue->set_alignment (alignment); > +} > + [...snip...] > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map > index f373fd39ac7..df51e4fdd8c 100644 > --- a/gcc/jit/libgccjit.map > +++ b/gcc/jit/libgccjit.map > @@ -243,3 +243,21 @@ LIBGCCJIT_ABI_19 { > gcc_jit_context_new_union_constructor; > gcc_jit_global_set_initializer_rvalue; > } LIBGCCJIT_ABI_18; > + > +LIBGCCJIT_ABI_20 { > +} LIBGCCJIT_ABI_19; > + > +LIBGCCJIT_ABI_21 { > +} LIBGCCJIT_ABI_20; > + > +LIBGCCJIT_ABI_22 { > +} LIBGCCJIT_ABI_21; > + > +LIBGCCJIT_ABI_23 { > +} LIBGCCJIT_ABI_22; > + > +LIBGCCJIT_ABI_24 { > + global: > + gcc_jit_lvalue_set_alignment; > + gcc_jit_lvalue_get_alignment; > +} LIBGCCJIT_ABI_23; BTW, how much of a problem would it be to you if we changed the order of some of these? At this point the API numbering may be getting in the way of getting some of the simpler changes into trunk. > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h > index 29afe064db6..72c46e81e51 100644 > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h > @@ -306,6 +306,9 @@ > #undef create_code > #undef verify_code > > +/* test-setting-alignment.c: This can't be in the testcases array as it > + doesn't have a verify_code implementation. */ My first though was that with an empty verify_code implementation it might work, but I see that the test overrides the regular options to avoid -O3, so it can't be part of the combined tests. > + > /* test-string-literal.c */ > #define create_code create_code_string_literal > #define verify_code verify_code_string_literal > diff --git a/gcc/testsuite/jit.dg/test-setting-alignment.c b/gcc/testsuite/jit.dg/test-setting-alignment.c > new file mode 100644 > index 00000000000..e87afbeacd3 > --- /dev/null > +++ b/gcc/testsuite/jit.dg/test-setting-alignment.c > @@ -0,0 +1,64 @@ > +#include > +#include > + > +#include "libgccjit.h" > + > +/* We don't want set_options() in harness.h to set -O3 so our little local > + is optimized away. */ > +#define TEST_ESCHEWS_SET_OPTIONS > +static void set_options (gcc_jit_context *ctxt, const char *argv0) > +{ > +} > + > +#define TEST_COMPILING_TO_FILE > +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER > +#define OUTPUT_FILENAME "output-of-test-setting-alignment.c.s" > +#include "harness.h" > + > +void > +create_code (gcc_jit_context *ctxt, void *user_data) > +{ > + /* Let's try to inject the equivalent of: > + int foo __attribute__((aligned (8))); > + > + int main (void) { > + int bar __attribute__((aligned (16))); > + } > + */ > + gcc_jit_type *int_type = > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT); > + gcc_jit_lvalue *foo = > + gcc_jit_context_new_global ( > + ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, int_type, "foo"); > + gcc_jit_lvalue_set_alignment(foo, 64); > + > + gcc_jit_field *field = gcc_jit_context_new_field (ctxt, > + NULL, > + int_type, > + "a"); > + gcc_jit_struct *struct_type = > + gcc_jit_context_new_struct_type(ctxt, NULL, "Type", 1, &field); > + gcc_jit_function *func_main = > + gcc_jit_context_new_function (ctxt, NULL, > + GCC_JIT_FUNCTION_EXPORTED, > + int_type, > + "main", > + 0, NULL, > + 0); > + /*gcc_jit_rvalue *zero = gcc_jit_context_zero (ctxt, int_type);*/ > + gcc_jit_lvalue *bar = > + gcc_jit_function_new_local ( > + func_main, NULL, > + gcc_jit_struct_as_type (struct_type), > + "bar"); > + gcc_jit_lvalue_set_alignment(bar, 128); > + gcc_jit_block *block = gcc_jit_function_new_block (func_main, NULL); > + /*gcc_jit_block_add_assignment (block, NULL, bar, zero);*/ > + gcc_jit_rvalue *return_value = > + gcc_jit_lvalue_as_rvalue (gcc_jit_lvalue_access_field (bar, NULL, field)); > + gcc_jit_block_end_with_return (block, NULL, return_value); > +} > + > +/* { dg-final { jit-verify-output-file-was-created "" } } */ > +/* { dg-final { jit-verify-assembler-output ".comm foo,4,8" } } */ > +/* { dg-final { jit-verify-assembler-output "movl -16\\\(%rbp), %eax" } } */ The expected output from the test is x86 specific, so it needs something like: /* { dg-do compile { target x86_64-*-* } } */ at the top. Also, there's no test coverage for gcc_jit_lvalue_get_alignment. Hope the above is constructive; thanks again for the patch Dave