On Fri, Jul 24, 2020 at 5:43 PM Erick Ochoa <erick.oc...@theobroma-systems.com> wrote: > > This patchset brings back struct reorg to GCC. > > We’ve been working on improving cache utilization recently and would > like to share our current implementation to receive some feedback on it. > > Essentially, we’ve implemented the following components: > > Type-based escape analysis to determine if we can reorganize a type > at link-time > > Dead-field elimination to remove unused fields of a struct at > link-time > > The type-based escape analysis provides a list of types, that are not > visible outside of the current linking unit (e.g. parameter types of > external functions). > > The dead-field elimination pass analyses non-escaping structs for fields > that are not used in the linking unit and thus can be removed. The > resulting struct has a smaller memory footprint, which allows for a > higher cache utilization. > > As a side-effect a couple of new infrastructure code has been written > (e.g. a type walker, which we were really missing in GCC), which can be > of course reused for other passes as well. > > We’ve prepared a patchset in the following branch: > > refs/vendors/ARM/heads/arm-struct-reorg-wip
Just had some time to peek into this. Ugh. The code doesn't look like GCC code looks :/ It doesn't help to have one set of files per C++ class (25!). The code itself is undocumented - it's hard to understand what the purpose of all the Walker stuff is. You also didn't seem to know walk_tree () nor walk_gimple* (). Take as example - I figured to look for IPA pass entries, then I see + +static void +collect_types () +{ + GimpleTypeCollector collector; + collector.walk (); + collector.print_collected (); + ptrset_t types = collector.get_pointer_set (); + GimpleCaster caster (types); + caster.walk (); + if (flag_print_cast_analysis) + caster.print_reasons (); + ptrset_t casting = caster.get_sets (); + fix_escaping_types_in_set (casting); + GimpleAccesser accesser; + accesser.walk (); + if (flag_print_access_analysis) + accesser.print_accesses (); + record_field_map_t record_field_map = accesser.get_map (); + TypeIncompleteEquality equality; + bool has_fields_that_can_be_deleted = false; + typedef std::set<unsigned> field_offsets_t; there's no comments (not even file-level) that explains how type escape is computed. Sorry, but this isn't even close to be coarsely reviewable. > We’ve also added a subsection in the GCC internals document to allow > other compiler devs to better understand our design and implementation. > A generated PDF can be found here: > > https://cloud.theobroma-systems.com/s/aWwxPiDJ3nCgc7F > https://cloud.theobroma-systems.com/s/aWwxPiDJ3nCgc7F/download > > page: 719 > > We’ve been testing the pass against a range of in-tree tests and > real-life applications (e.g. all SPEC CPU2017 C benchmarks). For > testing, please see testing subsection in the gcc internals we prepared. > > Currently we see the following limitations: > > * It is not a "true" ipa pass yes. That is, we can only succeed with > -flto-partition=none. > * Currently it is not safe to use -fipa-sra. > * Brace constructors not supported now. We handle this gracefully. > * Only C as of now. > * Results of sizeof() and offsetof() are generated in the compiler > frontend and thus can’t be changed later at link time. There are a > couple of ideas to resolve this, but that’s currently unimplemented. > * At this point we’d like to thank the GCC community for their patient > help so far on the mailing list and in other channels. And we ask for > your support in terms of feedback, comments and testing. > > Thanks!