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

[Andreas Krebbel <krebbel at linux dot vnet dot ibm dot com>]

On 06/24/2015 12:14 PM, Richard Biener wrote:
> On Wed, Jun 24, 2015 at 8:57 AM, Andreas Krebbel
>> 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.

This would mean that the GNU ABI marker would be set for code which makes use of one of the builtins
locally to optimize a special case wrapped in a runtime check. This is what I was trying to avoid
actually.
We do have some optimizations for Glibc. However, these are all written in assembler what would not
trigger the ABI flag to be set. So for Glibc the more conservative approach would be no problem so far.

The current implementation has the hole that an ABI vector type usage might not be detected if the
type is used in a sizeof construct without being used anywhere else. One question is how big that
problem actually is? Another is if there are more cases which might slip through? If it turns out to
be too risky I probably have to go with one of the more conservative approaches :(

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

Yes. I've seen this and plan to implement this ontop of the other mechanism. But we would still need
something like the GNU ABI marker for GDB.

Bye,

-Andreas-


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