On Tue, 2021-09-21 at 17:35 -0500, Bill Schmidt wrote:
> Hi!
>
> Previously zero-width bit fields were removed from structs, so that otherwise
> homogeneous aggregates were treated as such and passed in FPRs and VSRs.
> This was incorrect behavior per the ELFv2 ABI. Now that these fields are no
> longer being removed, we generate the correct parameter passing code. Alert
> the unwary user in the rare cases where this behavior changes.
>
> As noted in the PR, once the GCC 12 Changes page has text describing this
> issue,
> we can update the diagnostic message to reference that URL. I'll handle that
> in a follow-up patch.
>
> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.
> Is this okay for trunk?
How previously? is this one that will need all the backports?
>
> Thanks!
> Bill
>
>
> 2021-09-21 Bill Schmidt <wschm...@linux.ibm.com>
>
> gcc/
> PR target/102024
> * config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Detect
> zero-width bit fields and return indicator.
> (rs6000_discover_homogeneous_aggregate): Diagnose when the
> presence of a zero-width bit field changes parameter passing in
> GCC 12.
>
> gcc/testsuite/
> PR target/102024
> * g++.target/powerpc/pr102024.C: New.
ok
> ---
> gcc/config/rs6000/rs6000-call.c | 39 ++++++++++++++++++---
> gcc/testsuite/g++.target/powerpc/pr102024.C | 23 ++++++++++++
> 2 files changed, 57 insertions(+), 5 deletions(-)
> create mode 100644 gcc/testsuite/g++.target/powerpc/pr102024.C
>
> diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c
> index 7d485480225..c02b202b0cd 100644
> --- a/gcc/config/rs6000/rs6000-call.c
> +++ b/gcc/config/rs6000/rs6000-call.c
> @@ -6227,7 +6227,7 @@ const struct altivec_builtin_types
> altivec_overloaded_builtins[] = {
>
> static int
> rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
> - int *empty_base_seen)
> + int *empty_base_seen, int *zero_width_bf_seen)
> {
> machine_mode mode;
> HOST_WIDE_INT size;
> @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type,
> machine_mode *modep,
> return -1;
>
> count = rs6000_aggregate_candidate (TREE_TYPE (type), modep,
> - empty_base_seen);
> + empty_base_seen,
> + zero_width_bf_seen);
> if (count == -1
> || !index
> || !TYPE_MAX_VALUE (index)
> @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type,
> machine_mode *modep,
> if (TREE_CODE (field) != FIELD_DECL)
> continue;
>
> + if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field))
> + {
> + *zero_width_bf_seen = 1;
> + continue;
> + }
> +
Noting that the definition comes from tree.h and is
#define SET_DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD(NODE, VAL) \
do { \
gcc_checking_assert (DECL_BIT_FIELD (NODE)); \
FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0 = (VAL); \
} while (0)
ok.
> if (DECL_FIELD_ABI_IGNORED (field))
> {
> if (lookup_attribute ("no_unique_address",
> @@ -6347,7 +6354,8 @@ rs6000_aggregate_candidate (const_tree type,
> machine_mode *modep,
> }
>
> sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
> - empty_base_seen);
> + empty_base_seen,
> + zero_width_bf_seen);
> if (sub_count < 0)
> return -1;
> count += sub_count;
> @@ -6381,7 +6389,8 @@ rs6000_aggregate_candidate (const_tree type,
> machine_mode *modep,
> continue;
>
> sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
> - empty_base_seen);
> + empty_base_seen,
> + zero_width_bf_seen);
> if (sub_count < 0)
> return -1;
> count = count > sub_count ? count : sub_count;
> @@ -6423,8 +6432,10 @@ rs6000_discover_homogeneous_aggregate (machine_mode
> mode, const_tree type,
> {
> machine_mode field_mode = VOIDmode;
> int empty_base_seen = 0;
> + int zero_width_bf_seen = 0;
> int field_count = rs6000_aggregate_candidate (type, &field_mode,
> - &empty_base_seen);
> + &empty_base_seen,
> + &zero_width_bf_seen);
>
That appears to be all of the callers of rs6000_aggregate_candidate.
(ok).
> if (field_count > 0)
> {
> @@ -6460,6 +6471,24 @@ rs6000_discover_homogeneous_aggregate (machine_mode
> mode, const_tree type,
> last_reported_type_uid = uid;
> }
> }
> + if (zero_width_bf_seen && warn_psabi)
> + {
> + static unsigned last_reported_type_uid;
> + unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
> + if (uid != last_reported_type_uid)
> + {
> + inform (input_location,
> + "parameter passing for an argument containing "
> + "zero-width bit fields but that is otherwise a "
> + "homogeneous aggregate changed in GCC 12.1");
> + last_reported_type_uid = uid;
> + }
> + if (elt_mode)
> + *elt_mode = mode;
> + if (n_elts)
> + *n_elts = 1;
> + return false;
> + }
In comparison to the other nearby warn_psabi logic, this seems
reasonable. (ok)
> return true;
> }
> }
> diff --git a/gcc/testsuite/g++.target/powerpc/pr102024.C
> b/gcc/testsuite/g++.target/powerpc/pr102024.C
> new file mode 100644
> index 00000000000..29c9678acfd
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/powerpc/pr102024.C
> @@ -0,0 +1,23 @@
> +// PR target/102024
> +// { dg-do compile { target powerpc_elfv2 } }
> +// { dg-options "-O2" }
> +
> +// Test that a zero-width bit field in an otherwise homogeneous aggregate
> +// generates a psabi warning and passes arguments in GPRs.
> +
> +// { dg-final { scan-assembler-times {\mstd\M} 4 } }
> +
> +struct a_thing
> +{
> + double x;
> + double y;
> + double z;
> + int : 0;
> + double w;
> +};
> +
> +double
> +foo (a_thing a) // { dg-message "parameter passing for an argument
> containing zero-width bit fields but that is otherwise a homogeneous
> aggregate changed in GCC 12.1" }
> +{
> + return a.x * a.y + a.z - a.w;
> +}
lgtm
thanks
-Will
