[Arm] Account for C++17 artificial field determining Homogeneous Aggregates
Kyrylo Tkachov
Kyrylo.Tkachov@arm.com
Tue Apr 28 10:21:29 GMT 2020
Hi Matthew,
> -----Original Message-----
> From: Matthew Malcomson <Matthew.Malcomson@arm.com>
> Sent: 27 April 2020 16:32
> To: gcc-patches@gcc.gnu.org
> Cc: nd <nd@arm.com>; nickc@redhat.com; Richard Earnshaw
> <Richard.Earnshaw@arm.com>; Ramana Radhakrishnan
> <Ramana.Radhakrishnan@arm.com>; Kyrylo Tkachov
> <Kyrylo.Tkachov@arm.com>; jakub@redhat.com
> Subject: [Arm] Account for C++17 artificial field determining Homogeneous
> Aggregates
>
> NOTE:
> There is another patch in the making to handle the APCS abi (selected with
> `-mabi=apcs-gnu`). That patch has a very different change in behaviour and
> is in a different part of the code so I'm keeping it in a different patch.
>
> In C++14, an empty class deriving from an empty base is not an
> aggregate, while in C++17 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
> Arm PCS ABI and hence should not count towards determining whether an
> object can be passed using the vector registers as per section
> "7.1.2 Procedure Calling" in the arm PCS
> https://developer.arm.com/docs/ihi0042/latest?_ga=2.60211309.150685319
> 6.1533541889-405231439.1528186050
>
> 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 GCC behaviour when run with `-std=gnu++14`.
>
> > gcc -std=gnu++17 -march=armv8-a+simd -mfloat-abi=hard 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:
> Bootstrapped and regression tested on arm-linux.
> This change fixes the struct-layout-1 tests Jakub added
> https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544204.html
> Regression tested on arm-none-eabi.
>
This looks like it follows the logic of the other port patches.
Ok.
Thanks,
Kyrill
> gcc/ChangeLog:
>
> 2020-04-27 Matthew Malcomson <matthew.malcomson@arm.com>
> Jakub Jelinek <jakub@redhat.com>
>
> PR target/94383
> * config/arm/arm.c (aapcs_vfp_sub_candidate): Account for C++17
> empty
> base class artificial fields.
> (aapcs_vfp_is_call_or_return_candidate): Warn when PCS ABI
> decision is different after this fix.
>
>
>
> ############### Attachment also inlined for ease of reply
> ###############
>
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index
> 0151bda90d961ae1a001c61cd5e94d6ec67e3aea..0db444c3fdb2ba6fb7ebad4
> 10310fb5b1bc0e304 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -6139,9 +6139,19 @@ aapcs_vfp_cum_init (CUMULATIVE_ARGS *pcum
> ATTRIBUTE_UNUSED,
> 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;
> @@ -6213,7 +6223,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)
> @@ -6251,7 +6262,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;
> @@ -6284,7 +6306,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;
> @@ -6346,10 +6369,27 @@ aapcs_vfp_is_call_or_return_candidate (enum
> arm_pcs pcs_variant,
> out from the mode. */
> if (type)
> {
> - 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 <= 4)
> - *count = ag_count;
> + {
> + 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);
> + }
> + *count = ag_count;
> + }
> else
> return false;
> }
More information about the Gcc-patches
mailing list