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