PR102024 - IBM Z: Add psabi diagnostics

For IBM Z in particular there is a problem with structs like:

struct A { float a; int :0; };

Our ABI document allows passing a struct in an FPR only if it has
exactly one member. On the other hand it says that structs of 1,2,4,8
bytes are passed in a GPR. So this struct is expected to be passed in
a GPR. Since we don't return structs in registers (regardless of the
number of members) it is always returned in memory.

Situation is as follows:

All compiler versions tested return it in memory - as expected.

gcc 11, gcc 12, g++ 12, and clang 13 pass it in a GPR - as expected.

g++ 11 as well as clang++ 13 pass in an FPR

For IBM Z we stick to the current GCC 12 behavior, i.e. zero-width
bitfields are NOT ignored.  A struct as above will be passed in a
GPR. Rational behind this is that not affecting the C ABI is more
important here.

A patch for clang is in progress: https://reviews.llvm.org/D122388

In addition to the usual regression test I ran the compat and
struct-layout-1 testsuites comparing the compiler before and after the
patch.

gcc/ChangeLog:
	PR target/102024
	* config/s390/s390-protos.h (s390_function_arg_vector): Remove
	prototype.
	* config/s390/s390.cc (s390_single_field_struct_p): New function.
	(s390_function_arg_vector): Invoke s390_single_field_struct_p.
	(s390_function_arg_float): Likewise.

gcc/testsuite/ChangeLog:
	PR target/102024
	* g++.target/s390/pr102024-1.C: New test.
	* g++.target/s390/pr102024-2.C: New test.
	* g++.target/s390/pr102024-3.C: New test.
	* g++.target/s390/pr102024-4.C: New test.
	* g++.target/s390/pr102024-5.C: New test.
	* g++.target/s390/pr102024-6.C: New test.

diff --git a/gcc/config/s390/s390-protos.h b/gcc/config/s390/s390-protos.h
index e625159..fd4acaa 100644 (file)
--- a/gcc/config/s390/s390-protos.h
+++ b/gcc/config/s390/s390-protos.h
@@ -49,7 +49,6 @@ extern void s390_function_profiler (FILE *, int);
 extern void s390_set_has_landing_pad_p (bool);
 extern bool s390_hard_regno_rename_ok (unsigned int, unsigned int);
 extern int s390_class_max_nregs (enum reg_class, machine_mode);
-extern bool s390_function_arg_vector (machine_mode, const_tree);
 extern bool s390_return_addr_from_memory(void);
 extern bool s390_fma_allowed_p (machine_mode);
 #if S390_USE_TARGET_ATTRIBUTE

diff --git a/gcc/config/s390/s390.cc b/gcc/config/s390/s390.cc
index 43352a2..7c3bd6c 100644 (file)
--- a/gcc/config/s390/s390.cc
+++ b/gcc/config/s390/s390.cc
@@ -12142,29 +12142,26 @@ s390_function_arg_size (machine_mode mode, const_tree type)
   gcc_unreachable ();
 }
 
-/* Return true if a function argument of type TYPE and mode MODE
-   is to be passed in a vector register, if available.  */
-
-bool
-s390_function_arg_vector (machine_mode mode, const_tree type)
+/* Return true if a variable of TYPE should be passed as single value
+   with type CODE. If STRICT_SIZE_CHECK_P is true the sizes of the
+   record type and the field type must match.
+
+   The ABI says that record types with a single member are treated
+   just like that member would be.  This function is a helper to
+   detect such cases.  The function also produces the proper
+   diagnostics for cases where the outcome might be different
+   depending on the GCC version.  */
+static bool
+s390_single_field_struct_p (enum tree_code code, const_tree type,
+                           bool strict_size_check_p)
 {
-  if (!TARGET_VX_ABI)
-    return false;
-
-  if (s390_function_arg_size (mode, type) > 16)
-    return false;
-
-  /* No type info available for some library calls ...  */
-  if (!type)
-    return VECTOR_MODE_P (mode);
-
-  /* The ABI says that record types with a single member are treated
-     just like that member would be.  */
   int empty_base_seen = 0;
+  bool zero_width_bf_skipped_p = false;
   const_tree orig_type = type;
+
   while (TREE_CODE (type) == RECORD_TYPE)
     {
-      tree field, single = NULL_TREE;
+      tree field, single_type = NULL_TREE;
 
       for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
        {
@@ -12181,48 +12178,108 @@ s390_function_arg_vector (machine_mode mode, const_tree type)
              continue;
            }
 
-         if (single == NULL_TREE)
-           single = TREE_TYPE (field);
+         if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
+           {
+             zero_width_bf_skipped_p = true;
+             continue;
+           }
+
+         if (single_type == NULL_TREE)
+           single_type = TREE_TYPE (field);
          else
            return false;
        }
 
-      if (single == NULL_TREE)
+      if (single_type == NULL_TREE)
        return false;
-      else
-       {
-         /* If the field declaration adds extra byte due to
-            e.g. padding this is not accepted as vector type.  */
-         if (int_size_in_bytes (single) <= 0
-             || int_size_in_bytes (single) != int_size_in_bytes (type))
-           return false;
-         type = single;
-       }
+
+      /* Reaching this point we have a struct with a single member and
+        zero or more zero-sized bit-fields which have been skipped in the
+        past.  */
+
+      /* If ZERO_WIDTH_BF_SKIPPED_P then the struct will not be accepted.  In case
+        we are not supposed to emit a warning exit early.  */
+      if (zero_width_bf_skipped_p && !warn_psabi)
+       return false;
+
+      /* If the field declaration adds extra bytes due to padding this
+        is not accepted with STRICT_SIZE_CHECK_P.  */
+      if (strict_size_check_p
+         && (int_size_in_bytes (single_type) <= 0
+             || int_size_in_bytes (single_type) != int_size_in_bytes (type)))
+       return false;
+
+      type = single_type;
     }
 
-  if (!VECTOR_TYPE_P (type))
+  if (TREE_CODE (type) != code)
     return false;
 
-  if (warn_psabi && empty_base_seen)
+  if (warn_psabi)
     {
-      static unsigned last_reported_type_uid;
       unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type));
-      if (uid != last_reported_type_uid)
-       {
-         const char *url = CHANGES_ROOT_URL "gcc-10/changes.html#empty_base";
-         last_reported_type_uid = uid;
-         if (empty_base_seen & 1)
-           inform (input_location,
-                   "parameter passing for argument of type %qT when C++17 "
-                   "is enabled changed to match C++14 %{in GCC 10.1%}",
-                   orig_type, url);
-         else
-           inform (input_location,
-                   "parameter passing for argument of type %qT with "
-                   "%<[[no_unique_address]]%> members changed "
-                   "%{in GCC 10.1%}", orig_type, url);
+
+      if (empty_base_seen)
+       {
+         static unsigned last_reported_type_uid_empty_base;
+         if (uid != last_reported_type_uid_empty_base)
+           {
+             last_reported_type_uid_empty_base = uid;
+             const char *url = CHANGES_ROOT_URL "gcc-10/changes.html#empty_base";
+             if (empty_base_seen & 1)
+               inform (input_location,
+                       "parameter passing for argument of type %qT when C++17 "
+                       "is enabled changed to match C++14 %{in GCC 10.1%}",
+                       orig_type, url);
+             else
+               inform (input_location,
+                       "parameter passing for argument of type %qT with "
+                       "%<[[no_unique_address]]%> members changed "
+                       "%{in GCC 10.1%}", orig_type, url);
+           }
+       }
+
+      /* For C++ older GCCs ignored zero width bitfields and therefore
+        passed structs more often as single values than GCC 12 does.
+        So diagnostics are only required in cases where we do NOT
+        accept the struct to be passed as single value.  */
+      if (zero_width_bf_skipped_p)
+       {
+         static unsigned last_reported_type_uid_zero_width;
+         if (uid != last_reported_type_uid_zero_width)
+           {
+             last_reported_type_uid_zero_width = uid;
+             inform (input_location,
+                     "parameter passing for argument of type %qT with "
+                     "zero-width bit fields members changed in GCC 12",
+                     orig_type);
+           }
        }
     }
+
+  return !zero_width_bf_skipped_p;
+}
+
+
+/* Return true if a function argument of type TYPE and mode MODE
+   is to be passed in a vector register, if available.  */
+
+static bool
+s390_function_arg_vector (machine_mode mode, const_tree type)
+{
+  if (!TARGET_VX_ABI)
+    return false;
+
+  if (s390_function_arg_size (mode, type) > 16)
+    return false;
+
+  /* No type info available for some library calls ...  */
+  if (!type)
+    return VECTOR_MODE_P (mode);
+
+  if (!s390_single_field_struct_p (VECTOR_TYPE, type, true))
+    return false;
+
   return true;
 }
 
@@ -12243,64 +12300,9 @@ s390_function_arg_float (machine_mode mode, const_tree type)
   if (!type)
     return mode == SFmode || mode == DFmode || mode == SDmode || mode == DDmode;
 
-  /* The ABI says that record types with a single member are treated
-     just like that member would be.  */
-  int empty_base_seen = 0;
-  const_tree orig_type = type;
-  while (TREE_CODE (type) == RECORD_TYPE)
-    {
-      tree field, single = NULL_TREE;
-
-      for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
-       {
-         if (TREE_CODE (field) != FIELD_DECL)
-           continue;
-         if (DECL_FIELD_ABI_IGNORED (field))
-           {
-             if (lookup_attribute ("no_unique_address",
-                                   DECL_ATTRIBUTES (field)))
-               empty_base_seen |= 2;
-             else
-               empty_base_seen |= 1;
-             continue;
-           }
-
-         if (single == NULL_TREE)
-           single = TREE_TYPE (field);
-         else
-           return false;
-       }
-
-      if (single == NULL_TREE)
-       return false;
-      else
-       type = single;
-    }
-
-  if (TREE_CODE (type) != REAL_TYPE)
+  if (!s390_single_field_struct_p (REAL_TYPE, type, false))
     return false;
 
-  if (warn_psabi && empty_base_seen)
-    {
-      static unsigned last_reported_type_uid;
-      unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type));
-      if (uid != last_reported_type_uid)
-       {
-         const char *url = CHANGES_ROOT_URL "gcc-10/changes.html#empty_base";
-         last_reported_type_uid = uid;
-         if (empty_base_seen & 1)
-           inform (input_location,
-                   "parameter passing for argument of type %qT when C++17 "
-                   "is enabled changed to match C++14 %{in GCC 10.1%}",
-                   orig_type, url);
-         else
-           inform (input_location,
-                   "parameter passing for argument of type %qT with "
-                   "%<[[no_unique_address]]%> members changed "
-                   "%{in GCC 10.1%}", orig_type, url);
-       }
-    }
-
   return true;
 }
 

diff --git a/gcc/testsuite/g++.target/s390/pr102024-1.C b/gcc/testsuite/g++.target/s390/pr102024-1.C
new file mode 100644 (file)
index 0000000..a2cdd40
--- /dev/null
+++ b/gcc/testsuite/g++.target/s390/pr102024-1.C
@@ -0,0 +1,12 @@
+// PR target/102024
+// { dg-do compile }
+
+struct S { float a; int : 0; };
+void foo (struct S x);
+
+void
+bar (void)
+{
+  struct S s = { 0.0f };
+  foo (s);     // { dg-message "with zero-width bit fields members changed in GCC 12" }
+}

diff --git a/gcc/testsuite/g++.target/s390/pr102024-2.C b/gcc/testsuite/g++.target/s390/pr102024-2.C
new file mode 100644 (file)
index 0000000..3ca7ce6
--- /dev/null
+++ b/gcc/testsuite/g++.target/s390/pr102024-2.C
@@ -0,0 +1,14 @@
+// PR target/102024
+// { dg-do compile }
+
+/* struct would never be passed in an FPR so no warning expected.  */
+
+struct S { float a; int :0; float b; };
+void foo (struct S x);
+
+void
+bar (void)
+{
+  struct S s = { 0.0f };
+  foo (s);     // { dg-bogus "with zero-width bit fields members changed in GCC 12" }
+}

diff --git a/gcc/testsuite/g++.target/s390/pr102024-3.C b/gcc/testsuite/g++.target/s390/pr102024-3.C
new file mode 100644 (file)
index 0000000..51514c3
--- /dev/null
+++ b/gcc/testsuite/g++.target/s390/pr102024-3.C
@@ -0,0 +1,15 @@
+// PR target/102024
+// { dg-do compile }
+
+/* struct S would not be passed as single value anyway so no warning expected.  */
+
+struct T { float a; float b; };
+struct S { struct T t; int :0; };
+void foo (struct S x);
+
+void
+bar (void)
+{
+  struct S s = { { 0.0f, 0.0f } };
+  foo (s);     // { dg-bogus "with zero-width bit fields members changed in GCC 12" }
+}

diff --git a/gcc/testsuite/g++.target/s390/pr102024-4.C b/gcc/testsuite/g++.target/s390/pr102024-4.C
new file mode 100644 (file)
index 0000000..075d571
--- /dev/null
+++ b/gcc/testsuite/g++.target/s390/pr102024-4.C
@@ -0,0 +1,15 @@
+// PR target/102024
+// { dg-do compile }
+
+/* struct S would not be passed as single value anyway so no warning expected.  */
+
+struct T { float a; int :0; };
+struct S { struct T t; int x; };
+void foo (struct S x);
+
+void
+bar (void)
+{
+  struct S s = { { 0.0f }, 0 };
+  foo (s);     // { dg-bogus "with zero-width bit fields members changed in GCC 12" }
+}

diff --git a/gcc/testsuite/g++.target/s390/pr102024-5.C b/gcc/testsuite/g++.target/s390/pr102024-5.C
new file mode 100644 (file)
index 0000000..a4355e7
--- /dev/null
+++ b/gcc/testsuite/g++.target/s390/pr102024-5.C
@@ -0,0 +1,14 @@
+// PR target/102024
+// { dg-do compile }
+
+struct U { float a; int :0; };
+struct T { struct U u; };
+struct S { struct T t; };
+void foo (struct S x);
+
+void
+bar (void)
+{
+  struct S s = { { { 0.0f } } };
+  foo (s);     // { dg-message "with zero-width bit fields members changed in GCC 12" }
+}

diff --git a/gcc/testsuite/g++.target/s390/pr102024-6.C b/gcc/testsuite/g++.target/s390/pr102024-6.C
new file mode 100644 (file)
index 0000000..9dd506e
--- /dev/null
+++ b/gcc/testsuite/g++.target/s390/pr102024-6.C
@@ -0,0 +1,12 @@
+// PR target/102024
+// { dg-do compile }
+
+struct S { int :0; float a; };
+void foo (struct S x);
+
+void
+bar (void)
+{
+  struct S s = { 0.0f };
+  foo (s);     // { dg-message "with zero-width bit fields members changed in GCC 12" }
+}