Hi Richard, Thank you for these suggestions! I’m really sorry about the delay in getting back to you.
I now have updated patches for this here: https://gcc.gnu.org/pipermail/gcc-patches/2025-July/689893.html > On 6 May 2025, at 4:04 PM, Richard Sandiford <richard.sandif...@arm.com> > wrote: > > External email: Use caution opening links or attachments > > > <soum...@nvidia.com> writes: >> From: Soumya AR <soum...@nvidia.com> >> >> Hi, >> >> This RFC and subsequent patch series introduces support for printing and >> parsing >> of aarch64 tuning parameters in the form of JSON. > > Thanks for doing this. It looks really useful. My main question is: > rather than write the parsing and printing routines by hand, could we > generate the structure definitions, the parsing code, and the printing > code from the schema? > I took some time in refactoring my approach so it would be better suited for auto-generation. Turned out to be much more frustrating than I expected, but I have it in the updated patch now. Since the schema does not provide any information about the structure types, the new approach uses templates for all the routines. Now, if anyone were to update tune_params, they would only need to update the schema alongside, and regenerate the printing/parsing routines using the script. I had initially hooked up the script to automatically run when the schema is updated but this would cause a build dependency on Python, so I’m sticking to manual invocation for now. As for structure definitions, like it was discussed later in the thread, I don't see a clean way yet to handle those... > The schema would need to provide more information about the structures > compared to the current one. The approach would also presumably need > build/*.o versions of the json routines. But it seems like something > that we might want to do elsewhere, so would be worth building a bit > of infrastructure around. And it would reduce the maintenance burden > associated with adding a new field or changing an existing one. > > Much more minor, but: in patch 1, I'm all in favour of removing the > "const"s from the field declarations, such as: > > struct scale_addr_mode_cost > { > - const int hi; > - const int si; > - const int di; > - const int ti; > + int hi; > + int si; > + int di; > + int ti; > }; > > IMO those consts should never have been there. But can we keep the > predefined tables themselves as const, without things like: > > -const struct cpu_cost_table tsv110_extra_costs = > +struct cpu_cost_table tsv110_extra_costs = > > ? If we make any changes to the contents of these tables, it should > IMO be done via temporaries instead. > I naively thought that this change would be easy enough to implement but ran into one major issue — since most nested structures in tune_params are pointers and not embedded directly, if we generate temporaries and remap the pointers to point to these temps, when and where do we free up the memory? Possible solutions: 1. Simply don't bother freeing up the memory and let the memory leak be :) 2. Use the GCC garbage collector 3. Remove the pointers and have all the nested structures be embedded since I'm not too sure why this is inconsistent in tune_params. 4. (The current approach) Have each temporary structure be a static local variable. This works, but operates under the assumption that nested structures in tune_params will always have unique types, since the variable declaration is a template that is resolved by the type of the nested structures. There is an extra check added to ensure we don't overwrite accidentally, in the case we ever end up having structure members of the same type. Let me know what you think is the best way to go about this. Thanks, Soumya > Thanks, > Richard
