From e73a32d6d47ec7c5fb5a5fe7eb896c0e1258ea68 Mon Sep 17 00:00:00 2001 From: Matthew Malcomson Date: Thu, 23 Apr 2020 15:33:55 +0100 Subject: [PATCH] [AArch64] (PR94383) Avoid C++17 empty base field checking for HVA/HFA In C++17, an empty class deriving from an empty base is not an aggregate, while in C++14 it is. In order to implement this, GCC adds an artificial field to such classes. This artificial field has no mapping to Fundamental Data Types in the AArch64 PCS ABI and hence should not count towards determining whether an object can be passed using the vector registers as per section "6.4.2 Parameter Passing Rules" in the AArch64 PCS. https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#the-base-procedure-call-standard This patch avoids counting this artificial field in aapcs_vfp_sub_candidate, and hence calculates whether such objects should be passed in vector registers in the same manner as C++14 (where the artificial field does not exist). Before this change, the test below would pass the arguments to `f` in general registers. After this change, the test passes the arguments to `f` using the vector registers. The new behaviour matches the behaviour of `armclang`, and also matches the behaviour when run with `-std=gnu++14`. > gcc -std=gnu++17 test.cpp ``` test.cpp struct base {}; struct pair : base { float first; float second; pair (float f, float s) : first(f), second(s) {} }; void f (pair); int main() { f({3.14, 666}); return 1; } ``` We add a `-Wpsabi` warning to catch cases where this fix has changed the ABI for some functions. Unfortunately this warning is not emitted twice for multiple calls to the same function, but I feel this is not much of a problem and can be fixed later if needs be. (i.e. if `main` called `f` twice in a row we only emit a diagnostic for the first). Testing: Bootstrap and regression test on aarch64-linux. All struct-layout-1 tests now pass. gcc/ChangeLog: 2020-04-23 Matthew Malcomson Jakub Jelinek PR target/94383 * config/aarch64/aarch64.c (aapcs_vfp_sub_candidate): Account for C++17 empty base class artificial fields. (aarch64_vfp_is_call_or_return_candidate): Warn when ABI PCS decision is different after this fix. --- gcc/ChangeLog | 9 ++++++ gcc/config/aarch64/aarch64.c | 53 +++++++++++++++++++++++++++++++----- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 69f07c4f663..71c480a24c6 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,12 @@ +2020-04-23 Matthew Malcomson + Jakub Jelinek + + PR target/94383 + * config/aarch64/aarch64.c (aapcs_vfp_sub_candidate): Account for C++17 + empty base class artificial fields. + (aarch64_vfp_is_call_or_return_candidate): Warn when ABI PCS decision is + different after this fix. + 2020-04-23 Jakub Jelinek PR target/94707 diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index f728ac530a4..a81b0b2ac04 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -16442,9 +16442,19 @@ aarch64_member_type_forces_blk (const_tree field_or_array, machine_mode mode) If *MODEP is VOIDmode, then set it to the first valid floating point type. If a non-floating point type is found, or if a floating point type that doesn't match a non-VOIDmode *MODEP is found, then return -1, - otherwise return the count in the sub-tree. */ + otherwise return the count in the sub-tree. + + The AVOID_CXX17_EMPTY_BASE argument is to allow the caller to check whether + this function has changed its behavior after the fix for PR94384 -- this fix + is to avoid artificial fields in empty base classes. + When called with this argument as a NULL pointer this function does not + avoid the artificial fields -- this is useful to check whether the function + returns something different after the fix. + When called pointing at a value, this function avoids such artificial fields + and sets the value to TRUE when one of these fields has been set. */ static int -aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep) +aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep, + bool *avoid_cxx17_empty_base) { machine_mode mode; HOST_WIDE_INT size; @@ -16520,7 +16530,8 @@ aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep) || TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST) return -1; - count = aapcs_vfp_sub_candidate (TREE_TYPE (type), modep); + count = aapcs_vfp_sub_candidate (TREE_TYPE (type), modep, + avoid_cxx17_empty_base); if (count == -1 || !index || !TYPE_MAX_VALUE (index) @@ -16558,7 +16569,18 @@ aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep) if (TREE_CODE (field) != FIELD_DECL) continue; - sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep); + /* Ignore C++17 empty base fields, while their type indicates + they do contain padding, they have zero size and thus don't + contain any padding. */ + if (cxx17_empty_base_field_p (field) + && avoid_cxx17_empty_base) + { + *avoid_cxx17_empty_base = true; + continue; + } + + sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep, + avoid_cxx17_empty_base); if (sub_count < 0) return -1; count += sub_count; @@ -16591,7 +16613,8 @@ aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep) if (TREE_CODE (field) != FIELD_DECL) continue; - sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep); + sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep, + avoid_cxx17_empty_base); if (sub_count < 0) return -1; count = count > sub_count ? count : sub_count; @@ -16718,10 +16741,26 @@ aarch64_vfp_is_call_or_return_candidate (machine_mode mode, } else if (type && composite_p) { - int ag_count = aapcs_vfp_sub_candidate (type, &new_mode); - + bool avoided = false; + int ag_count = aapcs_vfp_sub_candidate (type, &new_mode, &avoided); if (ag_count > 0 && ag_count <= HA_MAX_NUM_FLDS) { + static unsigned last_reported_type_uid; + unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); + int alt; + if (warn_psabi + && avoided + && uid != last_reported_type_uid + && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL)) + != ag_count)) + { + gcc_assert (alt == -1); + last_reported_type_uid = uid; + inform (input_location, "parameter passing for argument of type " + "%qT when C++17 is enabled changed to match C++14 " + "in GCC 10.1", type); + } + if (is_ha != NULL) *is_ha = true; *count = ag_count; } -- 2.30.2