Re: [PATCH] libgccjit: Add support for setting the alignment [PR104293]

[David Malcolm <dmalcolm@redhat.com>]

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 <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