PR102024 - IBM Z: Add psabi diagnostics
Commit Message
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
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
@@ -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
@@ -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;
}
new file mode 100644
@@ -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" }
+}
new file mode 100644
@@ -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" }
+}
new file mode 100644
@@ -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" }
+}
new file mode 100644
@@ -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" }
+}
new file mode 100644
@@ -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" }
+}
new file mode 100644
@@ -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" }
+}