Thanks for doing this. Matthew Malcomson <matthew.malcom...@arm.com> writes: > 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 currently emitted multiple > times > for each problem, but I feel this is not much of a problem and can be fixed > later if needs be.
Agreed. We need a better way of handling this in general (for all targets). > Testing: > Bootstrapped on aarch64-linux. > Jakub has provided a testsuite change that catches the original problem. > https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544204.html > This patch makes that testcase pass. > > gcc/ChangeLog: > > 2020-04-21 Matthew Malcomson <matthew.malcom...@arm.com> > Jakub Jelinek <ja...@redhat.com> > > PR target/94383 > * config/aarch64/aarch64.c (enum cpp17empty_state): New. > (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. > > > > ############### Attachment also inlined for ease of reply > ############### > > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index > c90de65de127992cc0bd9603a32684ebb5bd4fdf..f9f40d52c846afae0d8d2cd9ed4737cdc4b88896 > 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -15909,13 +15909,30 @@ aarch64_conditional_register_usage (void) > } > } > > +enum cpp17empty_state { > + DONT_AVOID = 0, > + AVOID = 1, > + AVOID_DONE = 3 Should only be two spaces of indentation. Some kind of prefix might be nice, since the names are very generic. But maybe we can leave this as-is for now and make it a scoped enum once we switch to C++11. > +}; > + > /* Walk down the type tree of TYPE counting consecutive base elements. > 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_C17EMPTY_FIELD argument is to allow the caller to check whether > + this function has changed its behaviour after the fix for PR94384 -- this behavior (alas) > + fix is to avoid artificial fields in empty base classes. > + When called pointing at a value of DONT_AVOID then 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 which has the AVOID bit set, this function > + avoids such artificial fields and sets the value to AVOID_DONE 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, > + enum cpp17empty_state *avoid_c17empty_field) > { > machine_mode mode; > HOST_WIDE_INT size; > @@ -15992,7 +16009,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_c17empty_field); > if (count == -1 > || !index > || !TYPE_MAX_VALUE (index) > @@ -16030,7 +16048,22 @@ 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 (DECL_ARTIFICIAL (field) > + && DECL_NAME (field) == NULL_TREE > + && RECORD_OR_UNION_TYPE_P (TREE_TYPE (field)) > + && DECL_SIZE (field) > + && integer_zerop (DECL_SIZE (field)) > + && (*avoid_c17empty_field & AVOID)) > + { For the record, it feels a bit weird to be checking DECL_NAME in an ABI decision like this. I think any zero-sized field should really be handled in the same way (and they explicitly are for "Pure Scalable Types"). But that's a deeper issue and I realise the intention is just to restrict the fix as much as possible to the C++17 case. I haven't audited the compiler to see if there could be other artificial decls with the same properties. I don't think that's necessary though, since any such decls ought to be handled in the same way as the C++17 ones. I.e. the "risk" in that direction is that we'll accidentally catch other cases that need the same fix. > + *avoid_c17empty_field = AVOID_DONE; > + continue; > + } > + > + sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep, > + avoid_c17empty_field); > if (sub_count < 0) > return -1; > count += sub_count; > @@ -16063,7 +16096,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_c17empty_field); > if (sub_count < 0) > return -1; > count = count > sub_count ? count : sub_count; > @@ -16180,10 +16214,22 @@ 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); > + enum cpp17empty_state after_fix = AVOID; > + int ag_count = aapcs_vfp_sub_candidate (type, &new_mode, &after_fix); > > if (ag_count > 0 && ag_count <= HA_MAX_NUM_FLDS) > { > + int alt; > + enum cpp17empty_state before_fix = DONT_AVOID; > + if (warn_psabi > + && after_fix == AVOID_DONE > + && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, > + &before_fix)) > + != ag_count) > + && (alt <= 0 || alt > HA_MAX_NUM_FLDS)) I think this last line should be a gcc_assert before the inform(). Something has gone wrong if we return a different in-range aggregate size here. > + inform (input_location, "parameter passing of type %qT changed for " > + "std=c++17 in GCC 10", type); I tripped over this wording a bit. It sounded like it was saying that the choice of standard started to matter in GCC 10, whereas it's really the opposite. Not sure I can suggest anything better, but would it work to turn it around and say: prior to GCC 10, parameters of type %qT were passed in an incompatible way for C++17 or just: prior to GCC 10, parameters of type %qT were handled incorrectly for C++17 Other suggestions welcome. :-) LGTM otherwise. Thanks, Richard
