From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 1448) id 464F63858D1E; Wed, 27 Apr 2022 07:23:40 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 464F63858D1E MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: Andreas Krebbel To: gcc-cvs@gcc.gnu.org Subject: [gcc r12-8277] PR102024 - IBM Z: Add psabi diagnostics X-Act-Checkin: gcc X-Git-Author: Andreas Krebbel X-Git-Refname: refs/heads/master X-Git-Oldrev: 9715f10c0651c9549b479b69d67be50ac4bd98a6 X-Git-Newrev: bc79f0d9048375e402497d5f2ef457c9500310e4 Message-Id: <20220427072340.464F63858D1E@sourceware.org> Date: Wed, 27 Apr 2022 07:23:40 +0000 (GMT) X-BeenThere: gcc-cvs@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Apr 2022 07:23:40 -0000 https://gcc.gnu.org/g:bc79f0d9048375e402497d5f2ef457c9500310e4 commit r12-8277-gbc79f0d9048375e402497d5f2ef457c9500310e4 Author: Andreas Krebbel Date: Wed Apr 27 09:20:41 2022 +0200 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: --- gcc/config/s390/s390-protos.h | 1 - gcc/config/s390/s390.cc | 208 +++++++++++++++-------------- 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, 187 insertions(+), 104 deletions(-) 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 43352a2ac1a..7c3bd6cbe7f 100644 --- 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 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" } +}