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.133.124]) by sourceware.org (Postfix) with ESMTPS id C3D583858D28 for ; Tue, 12 Apr 2022 21:51:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C3D583858D28 Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-586-paDz6Q1BN_aQknrGCdoIeg-1; Tue, 12 Apr 2022 17:51:37 -0400 X-MC-Unique: paDz6Q1BN_aQknrGCdoIeg-1 Received: by mail-qv1-f70.google.com with SMTP id jr12-20020a0562142a8c00b0044429017bcbso172482qvb.20 for ; Tue, 12 Apr 2022 14:51:37 -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=Skgo5E0YEdi77FwO6cJ9LhDV1YEHKuuvr/S+g9kAcso=; b=lTWrcCKV6WflGF0TNuoJ+Fy1GP6WAynD/koZg6QxYqTRgUiYFfHpcHxxA8OL9t5B8/ 5Pl5nT2oXiCEJtRTmasj9Wv7r+bXfDViSZvyolqcsMu0/8kD5Hlfm0yLp3akTE1t9DrM Y3eMvvrJ8zcUzYEPIDRkufEGIgLVSpz1pczHWBVDVw6JNRo69pccQf5g552Wl2BVPXFI lE5pvtPGDPTsrM6maWZxEAwhJDrOsz5WlTRlvrr6+sSzvNStN/LR3IvM8v6fTdwyUQG9 R1sOapx9VdhnimAcqZE8hz/SjG88Ir8oVi8vlHBIjmogfI5h9GcRQjHIOLcXwuXq5Kkh OEAw== X-Gm-Message-State: AOAM530WhXNxqdVTn4R/TyLF3wuS3g1XXyXus8zw1FNu4AAZW6RVibEG ehKwur0lfYnvJvBK+rbUi+7+SH1snhcHOnv5kRdgUbn08RfipAO+t5Q2LOQ/bYth9rcrapWBPch RNNLMl8E= X-Received: by 2002:ac8:57ca:0:b0:2e2:131b:6f0e with SMTP id w10-20020ac857ca000000b002e2131b6f0emr5105465qta.664.1649800296985; Tue, 12 Apr 2022 14:51:36 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwqBON/1hOtUMsS5INY5e7Gxh8mBI/HWIRhaZD/1f2K5Hl+hRLtyELQX3I9yP9B+gJRhzQdtw== X-Received: by 2002:ac8:57ca:0:b0:2e2:131b:6f0e with SMTP id w10-20020ac857ca000000b002e2131b6f0emr5105457qta.664.1649800296661; Tue, 12 Apr 2022 14:51:36 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id c70-20020a379a49000000b0069bdce177e2sm8657640qke.55.2022.04.12.14.51.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Apr 2022 14:51:35 -0700 (PDT) Message-ID: <0daad6110a15a9aab4e924c415405547248cd873.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: Tue, 12 Apr 2022 17:51:34 -0400 In-Reply-To: <7c11cc644be07afa0753dbabc2fb607c8af48af6.camel@zoho.com> References: <7f32bff12738484e3f5d97cfc481996fd7570b01.camel@zoho.com> <1a077e6549786d9aedb1cf353f176b0207520c6e.camel@redhat.com> <7c11cc644be07afa0753dbabc2fb607c8af48af6.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: 8bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham 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: Tue, 12 Apr 2022 21:51:41 -0000 On Sat, 2022-04-09 at 13:50 -0400, Antoni Boucher wrote: > Here's the updated patch. > > On Fri, 2022-04-08 at 15:01 -0400, David Malcolm wrote: > > 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. > > Ok, I updated it to use bytes. Thanks. I updated the patch slightly: * fixed up some hunks that didn't quite apply * tweaked the comment in all-non-failing-tests.h * some whitespace fixes, and a couple of "alignement" to "alignment" spelling fixes * added an ", in bytes" to the doc for gcc_jit_lvalue_set_alignment. * regenerated .texinfo from .rst Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. I've pushed the patch to trunk for GCC 12 as r12-8120-g6e5ad1cc24a315. https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=6e5ad1cc24a315d07f24c95d76c269cafe2a8182 Thanks again for all the patches Dave > > > > 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? > > That's not an issue: I have no problem changing the order. > > > > > 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 > > > > >