From: David Malcolm <dmalcolm@redhat.com>
To: Antoni Boucher <bouanto@zoho.com>,
gcc-patches@gcc.gnu.org, jit@gcc.gnu.org
Subject: Re: [PATCH] libgccjit: Add support for setting the alignment [PR104293]
Date: Tue, 12 Apr 2022 17:51:34 -0400 [thread overview]
Message-ID: <0daad6110a15a9aab4e924c415405547248cd873.camel@redhat.com> (raw)
In-Reply-To: <7c11cc644be07afa0753dbabc2fb607c8af48af6.camel@zoho.com>
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 <stdlib.h>
> > > +#include <stdio.h>
> > > +
> > > +#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
> >
> >
>
next prev parent reply other threads:[~2022-04-12 21:51 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-31 1:38 Antoni Boucher
2022-04-08 19:01 ` David Malcolm
2022-04-09 17:50 ` Antoni Boucher
2022-04-12 21:51 ` David Malcolm [this message]
2022-04-13 11:47 ` Bernhard Reutner-Fischer
2022-04-16 20:26 ` Marc Nieper-Wißkirchen
2022-04-17 2:28 ` Antoni Boucher
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0daad6110a15a9aab4e924c415405547248cd873.camel@redhat.com \
--to=dmalcolm@redhat.com \
--cc=bouanto@zoho.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jit@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).