PR102024 - IBM Z: Add psabi diagnostics

Message ID 20220325174541.58451-1-krebbel@linux.ibm.com
State New
Headers
Series PR102024 - IBM Z: Add psabi diagnostics |

Commit Message

Andreas Krebbel March 25, 2022, 5:45 p.m. UTC
  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.
---
 gcc/config/s390/s390-protos.h              |   1 -
 gcc/config/s390/s390.cc                    | 212 +++++++++++----------
 gcc/testsuite/g++.target/s390/pr102024-1.C |  12 ++
 gcc/testsuite/g++.target/s390/pr102024-2.C |  14 ++
 gcc/testsuite/g++.target/s390/pr102024-3.C |  15 ++
 gcc/testsuite/g++.target/s390/pr102024-4.C |  15 ++
 gcc/testsuite/g++.target/s390/pr102024-5.C |  14 ++
 gcc/testsuite/g++.target/s390/pr102024-6.C |  12 ++
 8 files changed, 195 insertions(+), 100 deletions(-)
 create mode 100644 gcc/testsuite/g++.target/s390/pr102024-1.C
 create mode 100644 gcc/testsuite/g++.target/s390/pr102024-2.C
 create mode 100644 gcc/testsuite/g++.target/s390/pr102024-3.C
 create mode 100644 gcc/testsuite/g++.target/s390/pr102024-4.C
 create mode 100644 gcc/testsuite/g++.target/s390/pr102024-5.C
 create mode 100644 gcc/testsuite/g++.target/s390/pr102024-6.C
  

Comments

Jakub Jelinek March 26, 2022, 9:48 a.m. UTC | #1
On Fri, Mar 25, 2022 at 06:45:41PM +0100, Andreas Krebbel wrote:
> 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.

LGTM, except:

> +      /* 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 (!single_p && zero_width_bf_seen_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);

You could use %{GCC 12.1%} and refer to
gcc-12/changes.html#zero_width_bitfields
like i386 does (not written yet, but the intent is to cover the various
arch decisions there).

	Jakub
  

Patch

diff --git a/gcc/config/s390/s390-protos.h b/gcc/config/s390/s390-protos.h
index e6251595870..fd4acaae44a 100644
--- 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 d2af6d8813d..6cfa586b9cd 100644
--- a/gcc/config/s390/s390.cc
+++ b/gcc/config/s390/s390.cc
@@ -12148,29 +12148,29 @@  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_seen_p = false;
   const_tree orig_type = type;
+  bool single_p = true;
+
   while (TREE_CODE (type) == RECORD_TYPE)
     {
-      tree field, single = NULL_TREE;
+      tree field, single_type = NULL_TREE;
+      int num_zero_width_bf_seen = 0;
+      int num_fields_seen = 0;
 
       for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
 	{
@@ -12187,48 +12187,117 @@  s390_function_arg_vector (machine_mode mode, const_tree type)
 	      continue;
 	    }
 
-	  if (single == NULL_TREE)
-	    single = TREE_TYPE (field);
+	  num_fields_seen++;
+
+	  if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
+	    {
+	      num_zero_width_bf_seen++;
+	      continue;
+	    }
+
+	  if (single_type == NULL_TREE)
+	    single_type = TREE_TYPE (field);
 	  else
 	    return false;
 	}
 
-      if (single == NULL_TREE)
+      /* If the field declaration adds extra bytes due to padding this
+	 is not accepted with STRICT_SIZE_CHECK_P.  */
+      if (single_type != NULL_TREE
+	  && strict_size_check_p
+	  && (int_size_in_bytes (single_type) <= 0
+	      || int_size_in_bytes (single_type) != int_size_in_bytes (type)))
+	return false;
+
+      if (single_type == NULL_TREE)
 	return false;
       else
+	type = single_type;
+
+      /* Remember that we have seen a situation which results in a
+	 different outcome with older GCC versions.  */
+      if (num_fields_seen - num_zero_width_bf_seen == 1)
+	zero_width_bf_seen_p = true;
+
+      if (num_fields_seen != 1)
 	{
-	  /* 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))
+	  /* The struct will not be accepted as single value.
+	     However, we have to continue if we are supposed to emit
+	     diagnostics.  */
+	  if (!zero_width_bf_seen_p || !warn_psabi)
 	    return false;
-	  type = single;
+	  single_p = false;
 	}
     }
 
-  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 (!single_p && zero_width_bf_seen_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 single_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;
 }
 
@@ -12249,64 +12318,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
index 00000000000..a2cdd40df2a
--- /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
index 00000000000..3ca7ce666d9
--- /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
index 00000000000..51514c3fcf5
--- /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
index 00000000000..075d5713f49
--- /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
index 00000000000..a4355e71da2
--- /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
index 00000000000..9dd506e59c8
--- /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" }
+}