> -----Original Message----- > From: Soumya AR <[email protected]> > Sent: 26 February 2026 09:39 > To: [email protected] > Cc: Tamar Christina <[email protected]>; Kyrylo Tkachov > <[email protected]>; David Malcolm <[email protected]> > Subject: [PATCH] aarch64: Fix NULL structures in base tunings being silently > ignored by JSON parser > > In the JSON parser's parse_object_helper for const pointer members, we have > a > if (!member) return; guard at entry. > > This is because the function works by dereferencing the member pointer and > making a non-const copy of the object it points to. The parser can then > override this copy with new values and re-assign the pointer to the new > object. > > This is problematic if member is NULL in the base tunings, but provided in the > JSON file, which can happen with some combinations of -mcpu and > -muser-provided-CPU. > > For example, with no -mcpu, if the generic tunings has issue_info set to > nullptr (eg. generic_armv8_a), then even if the user does provide issue_info > data in the JSON file, parse_object_helper will silently ignore it. > > The naive fix for this is to create a zero-initialized copy of the object if > it is NULL, and then override it with the new values from the JSON file. > > However, this results in another problem: if the user provides only selective > fields of the strucutre, the rest of the fields will be set to zero and > potentially interfere with costing decisions. > > I think at that point the best we can do is emit a warning. With David > Malcolm's > improved JSON diagnostics, we can be specific about the problematic > structure as > well.
Agreed. > > ---- > > Since we potentially zero-initialize objects, I had to add a default > constructor > to objects that only had parameterized constructors. Is this OK? I think it's OK since the normal usage of them are through the header files which fully populates them or explicitly sets to NULL. So don't think any usages would break. > > Bootstrapped and regtested on aarch64-linux-gnu, OK for trunk? > OK. Thanks, Tamar > Signed-off-by: Soumya AR <[email protected]> > > gcc/ChangeLog: > > * config/aarch64/aarch64-json-tunings-parser.cc > (parse_object_helper): > Zero-initialize objects that are NULL in the base tunings, if provided > in JSON tunings. > * config/aarch64/aarch64-protos.h (struct sve_vec_cost): Add default > constructor. > (struct aarch64_simd_vec_issue_info): Likewise. > (struct aarch64_sve_vec_issue_info): Likewise. > > gcc/testsuite/ChangeLog: > > * gcc.target/aarch64/aarch64-json-tunings/nullptr-issue-info.c: > New test. > * gcc.target/aarch64/aarch64-json-tunings/nullptr-issue-info.json: > New test input. > --- > .../aarch64/aarch64-json-tunings-parser.cc | 8 ++-- > gcc/config/aarch64/aarch64-protos.h | 6 +++ > .../aarch64-json-tunings/nullptr-issue-info.c | 20 ++++++++++ > .../nullptr-issue-info.json | 38 +++++++++++++++++++ > 4 files changed, 68 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/aarch64/aarch64-json- > tunings/nullptr-issue-info.c > create mode 100644 gcc/testsuite/gcc.target/aarch64/aarch64-json- > tunings/nullptr-issue-info.json > > diff --git a/gcc/config/aarch64/aarch64-json-tunings-parser.cc > b/gcc/config/aarch64/aarch64-json-tunings-parser.cc > index 9b7bdf535c9..19598034761 100644 > --- a/gcc/config/aarch64/aarch64-json-tunings-parser.cc > +++ b/gcc/config/aarch64/aarch64-json-tunings-parser.cc > @@ -109,9 +109,6 @@ static std::enable_if_t<std::is_pointer<T>::value > parse_object_helper (const json::object *field_obj, T &member, > parse_func_type<T> parse_func) > { > - if (!member) > - return; > - > /* Use static storage for the non-const copy. > This works because tune_params does not have nested structures of the > same type, but has room for errors if we end up having pointers to the > @@ -125,7 +122,10 @@ parse_object_helper (const json::object *field_obj, > T &member, > } > already_initialized = true; > using NonConstType = std::remove_const_t<std::remove_pointer_t<T>>; > - static NonConstType new_obj = *member; > + if (!member) > + warning (0, "JSON tuning overrides an unspecified structure in the base " > + "tuning; fields not provided in JSON will default to 0"); > + static NonConstType new_obj = member ? *member : NonConstType{}; > parse_func (field_obj, new_obj); > member = &new_obj; > } > diff --git a/gcc/config/aarch64/aarch64-protos.h > b/gcc/config/aarch64/aarch64-protos.h > index d1f2873f208..3f359b0069d 100644 > --- a/gcc/config/aarch64/aarch64-protos.h > +++ b/gcc/config/aarch64/aarch64-protos.h > @@ -255,6 +255,8 @@ typedef struct simd_vec_cost advsimd_vec_cost; > /* SVE-specific extensions to the information provided by simd_vec_cost. */ > struct sve_vec_cost : simd_vec_cost > { > + sve_vec_cost () = default; > + > CONSTEXPR sve_vec_cost (const simd_vec_cost &base, > unsigned int clast_cost, > unsigned int fadda_f16_cost, > @@ -365,6 +367,8 @@ using aarch64_scalar_vec_issue_info = > aarch64_base_vec_issue_info; > Advanced SIMD and SVE. */ > struct aarch64_simd_vec_issue_info : aarch64_base_vec_issue_info > { > + aarch64_simd_vec_issue_info () = default; > + > CONSTEXPR aarch64_simd_vec_issue_info (aarch64_base_vec_issue_info > base, > unsigned int ld2_st2_general_ops, > unsigned int ld3_st3_general_ops, > @@ -393,6 +397,8 @@ using aarch64_advsimd_vec_issue_info = > aarch64_simd_vec_issue_info; > is a concept of "predicate operations". */ > struct aarch64_sve_vec_issue_info : aarch64_simd_vec_issue_info > { > + aarch64_sve_vec_issue_info () = default; > + > CONSTEXPR aarch64_sve_vec_issue_info > (aarch64_simd_vec_issue_info base, > unsigned int pred_ops_per_cycle, > diff --git a/gcc/testsuite/gcc.target/aarch64/aarch64-json-tunings/nullptr- > issue-info.c b/gcc/testsuite/gcc.target/aarch64/aarch64-json-tunings/nullptr- > issue-info.c > new file mode 100644 > index 00000000000..3984c63b39e > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/aarch64-json-tunings/nullptr-issue- > info.c > @@ -0,0 +1,20 @@ > +/* Test that a structure provided in JSON is parsed even when the base tuning > + model has the structure set to NULL. */ > + > +/* { dg-do compile } */ > +/* { dg-additional-options "-muser-provided- > CPU=${srcdir}/gcc.target/aarch64/aarch64-json-tunings/nullptr-issue- > info.json -fdump-tuning-model=temp-nullptr-issue-info.json" } */ > + > +/* { dg-warning "JSON tuning overrides an unspecified structure in the base > tuning; fields not provided in JSON will default to 0" "" { target *-*-* } 0 > } */ > +/* { dg-warning "JSON tuning file does not contain version information" "" { > target *-*-* } 0 } */ > + > +/* { dg-final { scan-file "temp-nullptr-issue-info.json" > "\"loads_stores_per_cycle\": 4" } } */ > +/* { dg-final { scan-file "temp-nullptr-issue-info.json" > "\"general_ops_per_cycle\": 8" } } */ > + > +/* { dg-final { scan-file "temp-nullptr-issue-info.json" > "\"ld2_st2_general_ops\": 2" } } */ > +/* { dg-final { scan-file "temp-nullptr-issue-info.json" > "\"ld4_st4_general_ops\": 3" } } */ > + > +/* { dg-final { scan-file "temp-nullptr-issue-info.json" > "\"pred_ops_per_cycle\": 2" } } */ > +/* { dg-final { scan-file "temp-nullptr-issue-info.json" "\"while_pred_ops\": > 1" } } */ > +/* { dg-final { scan-file "temp-nullptr-issue-info.json" > "\"gather_scatter_pair_general_ops\": 1" } } */ > + > +int main () {} > diff --git a/gcc/testsuite/gcc.target/aarch64/aarch64-json-tunings/nullptr- > issue-info.json b/gcc/testsuite/gcc.target/aarch64/aarch64-json- > tunings/nullptr-issue-info.json > new file mode 100644 > index 00000000000..b6fefa87544 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/aarch64-json-tunings/nullptr-issue- > info.json > @@ -0,0 +1,38 @@ > +{ > + "tune_params": { > + "vec_costs": { > + "issue_info": { > + "scalar": { > + "loads_stores_per_cycle": 4, > + "stores_per_cycle": 2, > + "general_ops_per_cycle": 8, > + "fp_simd_load_general_ops": 0, > + "fp_simd_store_general_ops": 1 > + }, > + "advsimd": { > + "loads_stores_per_cycle": 3, > + "stores_per_cycle": 2, > + "general_ops_per_cycle": 6, > + "fp_simd_load_general_ops": 0, > + "fp_simd_store_general_ops": 1, > + "ld2_st2_general_ops": 2, > + "ld3_st3_general_ops": 2, > + "ld4_st4_general_ops": 3 > + }, > + "sve": { > + "loads_stores_per_cycle": 3, > + "stores_per_cycle": 2, > + "general_ops_per_cycle": 6, > + "fp_simd_load_general_ops": 0, > + "fp_simd_store_general_ops": 1, > + "pred_ops_per_cycle": 2, > + "while_pred_ops": 1, > + "int_cmp_pred_ops": 0, > + "fp_cmp_pred_ops": 0, > + "gather_scatter_pair_general_ops": 1, > + "gather_scatter_pair_pred_ops": 1 > + } > + } > + } > + } > +} > -- > 2.43.0
