On Mon, Jul 27, 2020 at 2:59 PM Christoph Müllner
<christoph.muell...@theobroma-systems.com> wrote:
>
> Hi Richard,
>
> On 7/27/20 2:36 PM, Richard Biener wrote:
> > 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!).
>
> Any suggestions how to best structure these?
As "bad" as it sounds, put everything into one file (maybe separate out
type escape analysis from the actual transform). Add a toplevel comment
per file explaining things.
> Are there some coding guidelines in the GCC project,
> which can help us to match the expectation?
Look at existing passes, otherwise there's mostly conventions on
formatting.
> > 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* ().
>
> True, we were not aware of that code.
> Thanks for pointing to that code.
> We will have a look.
>
> > 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.
>
> Sad to hear.
> We'll work on the input that you provided and provide a new version.
>
> Thanks,
> Christoph
>
> >
> >> 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!
