Re: [PATCH 1/8] S/390 Vector ABI GNU Attribute.

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Jun 24, 2015 at 8:57 AM, Andreas Krebbel
<krebbel@linux.vnet.ibm.com> wrote:
> With this patch .gnu_attribute is used to mark binaries with a vector
> ABI tag.  This is required since the z13 vector support breaks the ABI
> of existing vector_size attribute generated vector types:
>
> 1. vector_size(16) and bigger vectors are aligned to 8 byte
> boundaries (formerly vectors were always naturally aligned)
>
> 2. vector_size(16) or smaller vectors are passed via VR if available
> or by value on the stack (formerly vector were passed on the stack by
> reference).
>
> The .gnu_attribute will be used by ld to emit a warning if binaries
> with incompatible ABIs are being linked together:
> https://sourceware.org/ml/binutils/2015-04/msg00316.html
>
> And it will be used by GDB to perform inferior function calls using a
> vector ABI which fits to the binary being debugged:
> https://sourceware.org/ml/gdb-patches/2015-04/msg00833.html
>
> The current implementation tries to only set the attribute if the
> vector types are really used in ABI relevant contexts in order to
> avoid false positives during linking.
>
> However, this unfortunately has some limitations like in the following
> case where an ABI relevant context cannot be detected properly:
>
> typedef int __attribute__((vector_size(16))) v4si;
> struct A
> {
>   char x;
>   v4si y;
> };
> char a[sizeof(struct A)];
>
> The number of elements in a depends on the ABI (24 with -mvx and 32
> with -mno-vx).  However, the implementation is not able to detect this
> since the struct type is not used anywhere else and consequently does
> not survive until the checking code is able to see it.
>
> Ideas about how to improve the implementation without creating too
> many false postives are welcome.

I'd be more conservative and instead hook into
targetm.vector_mode_supported_p (and thus vector_type_mode).

Yes, it will trip on "local" vector types.  But I can't see how you
can avoid this in general without seeing the whole program.

If I'd do it retro-actively I'd reverse the flag and instead mark units
which use generic non-z13 vectors...

Note that other targets simply emit -Wpsabi warnings here:

> gcc t.c -S -m32
t.c: In function âfooâ:
t.c:4:1: warning: SSE vector return without SSE enabled changes the
ABI [-Wpsabi]
 {
 ^
t.c:3:6: note: The ABI for passing parameters with 16-byte alignment
has changed in GCC 4.6
 v4si foo (v4si x)
      ^
t.c:3:6: warning: SSE vector argument without SSE enabled changes the
ABI [-Wpsabi]

for

typedef int v4si __attribute__((vector_size(16)));

v4si foo (v4si x)
{
  return x;
}

on i?86 without -msse2.  So you could as well do that - warn for vector
type uses on non-z13 and be done with that.

Richard.

> In particular we do not want to set the attribute for local uses of
> vector types as they would be natural for ifunc optimizations.
>
> gcc/
>         * config/s390/s390.c (s390_vector_abi): New variable definition.
>         (s390_check_type_for_vector_abi): New function.
>         (TARGET_ASM_FILE_END): New macro definition.
>         (s390_asm_file_end): New function.
>         (s390_function_arg): Call s390_check_type_for_vector_abi.
>         (s390_gimplify_va_arg): Likewise.
>         * configure: Regenerate.
>         * configure.ac: Check for .gnu_attribute Binutils feature.
>
> gcc/testsuite/
>         * gcc.target/s390/vector/vec-abi-1.c: Add gnu attribute check.
>         * gcc.target/s390/vector/vec-abi-attr-1.c: New test.
>         * gcc.target/s390/vector/vec-abi-attr-2.c: New test.
>         * gcc.target/s390/vector/vec-abi-attr-3.c: New test.
>         * gcc.target/s390/vector/vec-abi-attr-4.c: New test.
>         * gcc.target/s390/vector/vec-abi-attr-5.c: New test.
>         * gcc.target/s390/vector/vec-abi-attr-6.c: New test.
> ---
>  gcc/config/s390/s390.c                             |  121 ++++++++++++++++++++
>  gcc/configure                                      |   36 ++++++
>  gcc/configure.ac                                   |    7 ++
>  gcc/testsuite/gcc.target/s390/vector/vec-abi-1.c   |    1 +
>  .../gcc.target/s390/vector/vec-abi-attr-1.c        |   18 +++
>  .../gcc.target/s390/vector/vec-abi-attr-2.c        |   53 +++++++++
>  .../gcc.target/s390/vector/vec-abi-attr-3.c        |   18 +++
>  .../gcc.target/s390/vector/vec-abi-attr-4.c        |   17 +++
>  .../gcc.target/s390/vector/vec-abi-attr-5.c        |   19 +++
>  .../gcc.target/s390/vector/vec-abi-attr-6.c        |   24 ++++
>  10 files changed, 314 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-1.c
>  create mode 100644 gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-2.c
>  create mode 100644 gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-3.c
>  create mode 100644 gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-4.c
>  create mode 100644 gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-5.c
>  create mode 100644 gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-6.c
>
> diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
> index d6ed179..934f7c0 100644
> --- a/gcc/config/s390/s390.c
> +++ b/gcc/config/s390/s390.c
> @@ -461,6 +461,97 @@ struct GTY(()) machine_function
>  #define PREDICT_DISTANCE (TARGET_Z10 ? 384 : 2048)
>
>
> +/* Indicate which ABI has been used for passing vector args.
> +   0 - no vector type arguments have been passed where the ABI is relevant
> +   1 - the old ABI has been used
> +   2 - a vector type argument has been passed either in a vector register
> +       or on the stack by value  */
> +static int s390_vector_abi = 0;
> +
> +/* Set the vector ABI marker if TYPE is subject to the vector ABI
> +   switch.  The vector ABI affects only vector data types.  There are
> +   two aspects of the vector ABI relevant here:
> +
> +   1. vectors >= 16 bytes have an alignment of 8 bytes with the new
> +   ABI and natural alignment with the old.
> +
> +   2. vector <= 16 bytes are passed in VRs or by value on the stack
> +   with the new ABI but by reference on the stack with the old.
> +
> +   If ARG_P is true TYPE is used for a function argument or return
> +   value.  The ABI marker then is set for all vector data types.  If
> +   ARG_P is false only type 1 vectors are being checked.  */
> +
> +static void
> +s390_check_type_for_vector_abi (const_tree type, bool arg_p, bool in_struct_p)
> +{
> +  static hash_set<const_tree> visited_types_hash;
> +
> +  if (s390_vector_abi)
> +    return;
> +
> +  if (type == NULL_TREE || TREE_CODE (type) == ERROR_MARK)
> +    return;
> +
> +  if (visited_types_hash.contains (type))
> +    return;
> +
> +  visited_types_hash.add (type);
> +
> +  if (VECTOR_TYPE_P (type))
> +    {
> +      int type_size = int_size_in_bytes (type);
> +
> +      /* Outside arguments only the alignment is changing and this
> +        only happens for vector types >= 16 bytes.  */
> +      if (!arg_p && type_size < 16)
> +       return;
> +
> +      /* In arguments vector types > 16 are passed as before (GCC
> +        never enforced the bigger alignment for arguments which was
> +        required by the old vector ABI).  However, it might still be
> +        ABI relevant due to the changed alignment if it is a struct
> +        member.  */
> +      if (arg_p && type_size > 16 && !in_struct_p)
> +       return;
> +
> +      s390_vector_abi = TARGET_VX_ABI ? 2 : 1;
> +    }
> +  else if (POINTER_TYPE_P (type) || TREE_CODE (type) == ARRAY_TYPE)
> +    {
> +      /* ARRAY_TYPE: Since with neither of the ABIs we have more than
> +        natural alignment there will never be ABI dependent padding
> +        in an array type.  That's why we do not set in_struct_p to
> +        true here.  */
> +      s390_check_type_for_vector_abi (TREE_TYPE (type), arg_p, in_struct_p);
> +    }
> +  else if (TREE_CODE (type) == FUNCTION_TYPE || TREE_CODE (type) == METHOD_TYPE)
> +    {
> +      tree arg_chain;
> +
> +      /* Check the return type.  */
> +      s390_check_type_for_vector_abi (TREE_TYPE (type), true, false);
> +
> +      for (arg_chain = TYPE_ARG_TYPES (type);
> +          arg_chain;
> +          arg_chain = TREE_CHAIN (arg_chain))
> +       s390_check_type_for_vector_abi (TREE_VALUE (arg_chain), true, false);
> +    }
> +  else if (RECORD_OR_UNION_TYPE_P (type))
> +    {
> +      tree field;
> +
> +      for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
> +       {
> +         if (TREE_CODE (field) != FIELD_DECL)
> +           continue;
> +
> +         s390_check_type_for_vector_abi (TREE_TYPE (field), arg_p, true);
> +       }
> +    }
> +}
> +
> +
>  /* System z builtins.  */
>
>  #include "s390-builtins.h"
> @@ -10898,6 +10989,8 @@ s390_function_arg (cumulative_args_t cum_v, machine_mode mode,
>  {
>    CUMULATIVE_ARGS *cum = get_cumulative_args (cum_v);
>
> +  if (!named)
> +    s390_check_type_for_vector_abi (type, true, false);
>
>    if (s390_function_arg_vector (mode, type))
>      {
> @@ -11289,6 +11382,8 @@ s390_gimplify_va_arg (tree valist, tree type, gimple_seq *pre_p,
>
>    size = int_size_in_bytes (type);
>
> +  s390_check_type_for_vector_abi (type, true, false);
> +
>    if (pass_by_reference (NULL, TYPE_MODE (type), type, false))
>      {
>        if (TARGET_DEBUG_ARG)
> @@ -13651,6 +13746,29 @@ s390_vector_alignment (const_tree type)
>    return MIN (64, tree_to_shwi (TYPE_SIZE (type)));
>  }
>
> +/* Implement TARGET_ASM_FILE_END.  */
> +static void
> +s390_asm_file_end (void)
> +{
> +#ifdef HAVE_AS_GNU_ATTRIBUTE
> +  varpool_node *vnode;
> +  cgraph_node *cnode;
> +
> +  FOR_EACH_VARIABLE (vnode)
> +    if (TREE_PUBLIC (vnode->decl))
> +      s390_check_type_for_vector_abi (TREE_TYPE (vnode->decl), false, false);
> +
> +  FOR_EACH_FUNCTION (cnode)
> +    if (TREE_PUBLIC (cnode->decl))
> +      s390_check_type_for_vector_abi (TREE_TYPE (cnode->decl), false, false);
> +
> +
> +  if (s390_vector_abi != 0)
> +    fprintf (asm_out_file, "\t.gnu_attribute 8, %d\n",
> +            s390_vector_abi);
> +#endif
> +  file_end_indicate_exec_stack ();
> +}
>
>  /* Return true if TYPE is a vector bool type.  */
>  static inline bool
> @@ -13927,6 +14045,9 @@ s390_invalid_binary_op (int op ATTRIBUTE_UNUSED, const_tree type1, const_tree ty
>  #undef TARGET_INVALID_BINARY_OP
>  #define TARGET_INVALID_BINARY_OP s390_invalid_binary_op
>
> +#undef TARGET_ASM_FILE_END
> +#define TARGET_ASM_FILE_END s390_asm_file_end
> +
>  struct gcc_target targetm = TARGET_INITIALIZER;
>
>  #include "gt-s390.h"
> diff --git a/gcc/configure b/gcc/configure
> index b26a86f..3f3f578 100755
> --- a/gcc/configure
> +++ b/gcc/configure
> @@ -26708,6 +26708,42 @@ fi
>        as_fn_error "Requesting --with-nan= requires assembler support for -mnan=" "$LINENO" 5
>      fi
>      ;;
> +    s390*-*-*)
> +    { $as_echo "$as_me:${as_lineno-$LINENO}: checking assembler for .gnu_attribute support" >&5
> +$as_echo_n "checking assembler for .gnu_attribute support... " >&6; }
> +if test "${gcc_cv_as_s390_gnu_attribute+set}" = set; then :
> +  $as_echo_n "(cached) " >&6
> +else
> +  gcc_cv_as_s390_gnu_attribute=no
> +    if test $in_tree_gas = yes; then
> +    if test $gcc_cv_gas_vers -ge `expr \( \( 2 \* 1000 \) + 18 \) \* 1000 + 0`
> +  then gcc_cv_as_s390_gnu_attribute=yes
> +fi
> +  elif test x$gcc_cv_as != x; then
> +    $as_echo '.gnu_attribute 8,1' > conftest.s
> +    if { ac_try='$gcc_cv_as $gcc_cv_as_flags  -o conftest.o conftest.s >&5'
> +  { { eval echo "\"\$as_me\":${as_lineno-$LINENO}: \"$ac_try\""; } >&5
> +  (eval $ac_try) 2>&5
> +  ac_status=$?
> +  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> +  test $ac_status = 0; }; }
> +    then
> +       gcc_cv_as_s390_gnu_attribute=yes
> +    else
> +      echo "configure: failed program was" >&5
> +      cat conftest.s >&5
> +    fi
> +    rm -f conftest.o conftest.s
> +  fi
> +fi
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gcc_cv_as_s390_gnu_attribute" >&5
> +$as_echo "$gcc_cv_as_s390_gnu_attribute" >&6; }
> +if test $gcc_cv_as_s390_gnu_attribute = yes; then
> +
> +$as_echo "#define HAVE_AS_GNU_ATTRIBUTE 1" >>confdefs.h
> +
> +fi
> +    ;;
>  esac
>
>  # Mips and HP-UX need the GNU assembler.
> diff --git a/gcc/configure.ac b/gcc/configure.ac
> index c09f3ae5..85f72d5 100644
> --- a/gcc/configure.ac
> +++ b/gcc/configure.ac
> @@ -4442,6 +4442,13 @@ pointers into PC-relative form.])
>         [Requesting --with-nan= requires assembler support for -mnan=])
>      fi
>      ;;
> +    s390*-*-*)
> +    gcc_GAS_CHECK_FEATURE([.gnu_attribute support],
> +      gcc_cv_as_s390_gnu_attribute, [2,18,0],,
> +      [.gnu_attribute 8,1],,
> +      [AC_DEFINE(HAVE_AS_GNU_ATTRIBUTE, 1,
> +         [Define if your assembler supports .gnu_attribute.])])
> +    ;;
>  esac
>
>  # Mips and HP-UX need the GNU assembler.
> diff --git a/gcc/testsuite/gcc.target/s390/vector/vec-abi-1.c b/gcc/testsuite/gcc.target/s390/vector/vec-abi-1.c
> index 5484664..db18e5e 100644
> --- a/gcc/testsuite/gcc.target/s390/vector/vec-abi-1.c
> +++ b/gcc/testsuite/gcc.target/s390/vector/vec-abi-1.c
> @@ -6,6 +6,7 @@
>  /* Make sure the last argument is fetched from the argument overflow area.  */
>  /* { dg-final { scan-assembler "vl\t%v\[0-9\]*,160\\(%r15\\)" { target lp64 } } } */
>  /* { dg-final { scan-assembler "vl\t%v\[0-9\]*,96\\(%r15\\)" { target ilp32 } } } */
> +/* { dg-final { scan-assembler "gnu_attribute 8, 2" } } */
>
>  typedef double v2df __attribute__((vector_size(16)));
>
> diff --git a/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-1.c b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-1.c
> new file mode 100644
> index 0000000..a06b338
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-1.c
> @@ -0,0 +1,18 @@
> +/* Check calling convention in the vector ABI.  */
> +
> +/* { dg-do compile { target { s390*-*-* } } } */
> +/* { dg-options "-O3 -mzarch -march=z13 -mno-vx" } */
> +
> +/* The function passes arguments whose calling conventions change with
> +   -mvx/-mno-vx.  In that case GCC has to emit the ABI attribute to
> +   allow GDB and Binutils to detect this.  */
> +/* { dg-final { scan-assembler "gnu_attribute 8, 1" } } */
> +
> +typedef double v2df __attribute__((vector_size(16)));
> +
> +v2df
> +add (v2df a, v2df b, v2df c, v2df d,
> +     v2df e, v2df f, v2df g, v2df h, v2df i)
> +{
> +  return a + b + c + d + e + f + g + h + i;
> +}
> diff --git a/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-2.c b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-2.c
> new file mode 100644
> index 0000000..97b9748
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-2.c
> @@ -0,0 +1,53 @@
> +/* Check calling convention in the vector ABI.  */
> +
> +/* { dg-do compile { target { s390*-*-* } } } */
> +/* { dg-options "-O3 -mzarch -march=z13" } */
> +
> +/* No abi attribute should be emitted when nothing relevant happened.  */
> +/* { dg-final { scan-assembler-not "gnu_attribute" } } */
> +
> +#include <stdarg.h>
> +
> +/* Local use is ok.  */
> +
> +typedef int v4si __attribute__((vector_size(16)));
> +
> +static
> +v4si __attribute__((__noinline__))
> +foo (v4si a)
> +{
> +  return a + (v4si){ 1, 2, 3, 4 };
> +}
> +
> +int
> +bar (int a)
> +{
> +  return foo ((v4si){ 1, 1, 1, 1 })[1];
> +}
> +
> +/* Big vector type only used as function argument and return value
> +   without being a struct/union member.  The alignment change is not
> +   relevant here.  */
> +
> +typedef double v4df __attribute__((vector_size(32)));
> +
> +v4df
> +add (v4df a, v4df b, v4df c, v4df d,
> +     v4df e, v4df f, v4df g, v4df h, v4df i)
> +{
> +  return a + b + c + d + e + f + g + h + i;
> +}
> +
> +double
> +bar2 (int n, ...)
> +{
> +  double ret;
> +  v4df a;
> +  va_list va;
> +
> +  va_start (va, n);
> +  ret = va_arg (va, v4df)[2];
> +  va_end (va);
> +
> +  return ret;
> +}
> diff --git a/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-3.c b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-3.c
> new file mode 100644
> index 0000000..f3dc368
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-3.c
> @@ -0,0 +1,18 @@
> +/* Check calling convention in the vector ABI.  */
> +
> +/* { dg-do compile { target { s390*-*-* } } } */
> +/* { dg-options "-O3 -mzarch -march=z13" } */
> +
> +/* { dg-final { scan-assembler "gnu_attribute 8, 2" } } */
> +
> +typedef double v4df __attribute__((vector_size(32)));
> +typedef struct { v4df a; } s;
> +
> +s
> +add (v4df a, v4df b, v4df c, v4df d,
> +     v4df e, v4df f, v4df g, v4df h, v4df i)
> +{
> +  s t;
> +  t.a = a + b + c + d + e + f + g + h + i;
> +  return t;
> +}
> diff --git a/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-4.c b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-4.c
> new file mode 100644
> index 0000000..ad9b29a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-4.c
> @@ -0,0 +1,17 @@
> +/* Check calling convention in the vector ABI.  */
> +
> +/* { dg-do compile { target { s390*-*-* } } } */
> +/* { dg-options "-O3 -mzarch -march=z13" } */
> +
> +/* { dg-final { scan-assembler "gnu_attribute 8, 2" } } */
> +
> +typedef int __attribute__((vector_size(16))) v4si;
> +
> +extern void bar (v4si);
> +
> +void
> +foo (int a)
> +{
> +  v4si b = (v4si){ a, a, a, a };
> +  bar (b);
> +}
> diff --git a/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-5.c b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-5.c
> new file mode 100644
> index 0000000..fb5de4e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-5.c
> @@ -0,0 +1,19 @@
> +/* Check calling convention in the vector ABI.  */
> +
> +/* { dg-do compile { target { s390*-*-* } } } */
> +/* { dg-options "-O3 -mzarch -march=z13" } */
> +
> +/* { dg-final { scan-assembler "gnu_attribute 8, 2" } } */
> +
> +#include <stdarg.h>
> +
> +typedef int __attribute__((vector_size(16))) v4si;
> +
> +extern void bar (int, ...);
> +
> +void
> +foo (int a)
> +{
> +  v4si b = (v4si){ a, a, a, a };
> +  bar (1, b);
> +}
> diff --git a/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-6.c b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-6.c
> new file mode 100644
> index 0000000..9134fa7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/s390/vector/vec-abi-attr-6.c
> @@ -0,0 +1,24 @@
> +/* Check calling convention in the vector ABI.  */
> +
> +/* { dg-do compile { target { s390*-*-* } } } */
> +/* { dg-options "-O3 -mzarch -march=z13" } */
> +
> +/* { dg-final { scan-assembler "gnu_attribute 8, 2" } } */
> +
> +#include <stdarg.h>
> +
> +typedef int __attribute__((vector_size(16))) v4si;
> +
> +int
> +bar (int n, ...)
> +{
> +  int ret;
> +  v4si a;
> +  va_list va;
> +
> +  va_start (va, n);
> +  ret = va_arg (va, v4si)[2];
> +  va_end (va);
> +
> +  return ret;
> +}
> --
> 1.7.9.5
>