On Fri, Nov 29, 2019 at 1:17 PM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Fri, Nov 29, 2019 at 12:37 PM Martin Liška <mli...@suse.cz> wrote:
> >
> > Hello.
> >
> > The patch is about streaming of at maximum 3 tree types that are
> > used for memory references in IPA ICF. That helps rapidly to reduce
> > number of function bodies loaded in WPA phase. Based on numbers for
> > Firefox we get from:
> >
> > Init called for 87557 items (23.30%).
> > ...
> > Totally needed symbols: 40580, fraction of loaded symbols: 46.35%
> >
> > to:
> >
> > Init called for 55844 items (14.86%).
> > ...
> > Totally needed symbols: 40580, fraction of loaded symbols: 72.67%
> >
> > Where memory peak drops from 5.7GB to 4.8GB and WPA is faster by 5 seconds.
> >
> > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >
> > Ready to be installed?
>
> It looks like a hack.  First of all you do quite some useless work since
> already the caller of walk_stmt_* could check for the vector length.
> second you are streaming a quite random type here with not much
> semantic value, plus 't' is already the base of the memory reference.
>
> To me it looks like the stmt hash is incredibly weak, for example
> not considering a GIMPLE_COND comparison code,
> doing weird stuff for commutative ops (the idea is probably to
> hash in both orders?

Oh, and it doesn't consider dependences at all but seems to
hash a functions stmts as independent?  I'd have hashed SSA
names as the hash of their def stmt to factor that in.

  but the implementation is completely off),
> but not considering the same for communative comparison ops
> (or swapped ops).
>
> To me it looks like if we want to hash types we could at least
> hash basic type properties plus for aggregates hash the ODR
> name if any such exists.
>
> > Thanks,
> > Martin
> >
> > gcc/ChangeLog:
> >
> > 2019-11-27  Martin Liska  <mli...@suse.cz>
> >
> >         PR ipa/92535
> >         * ipa-icf.c (sem_function::sem_function): Initialize
> >         memory_access_types.
> >         (record_memory_op_type): New function.
> >         (sem_function::init): Walk memory accesses for GIMPLE
> >         statements.
> >         (sem_item_optimizer::write_summary): Stream memory_access_types.
> >         (sem_item_optimizer::read_section): Read memory_access_types
> >         and hash pointer to canonical types.
> >         (sem_item_optimizer::execute): Update hash by memory
> >         access type.
> >         (sem_item_optimizer::update_hash_by_memory_access_type):
> >         New.
> >         * ipa-icf.h (memory_access_types): New.
> >         (m_canonical_types_hash): Likewise.
> >         (update_hash_by_memory_access_type): Likewise.
> > ---
> >   gcc/ipa-icf.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++--
> >   gcc/ipa-icf.h | 10 +++++++++
> >   2 files changed, 68 insertions(+), 2 deletions(-)
> >
> >

Reply via email to