On Fri, Nov 29, 2019 at 1:33 PM Jan Hubicka <hubi...@ucw.cz> wrote: > > > > 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. > > I agree that hash calculation needs fine tuning (and we want to be sure > we do have checking that hasher agrees with comparer). > > I did not look in detail into the Martin's patch. The idea of hashing > access type at WPA time came from our discussion. > If you make statistics, the dominating reason for giving up compare > after reading body is mismatch in memory operands. This is extremely > comon for instances of templates where functions often have ideantical > body except that basetypes of all accesses are different.
Is it? > Here we get same hash and eventually call operand_equal_p which walks > whole path and calls types_compatible_p that eventually finds the > basetype and returns false comparing TYPE_CANONICAL of both. > > Because TYPE_CANONICAL is not known at compile time I was considering > two options: break out canonical type hashes out of lto-common.c or > simply update the hash at compile time. But this is done at WPA time, after we merged types. We can use ODR types at compile-time already. We could also use the canonical type hash for all types I guess. > Well, this all is aimed to solve copmile time/memory use problem caused > by not merging these functions. Of course, we ought to merge. I think > we should not use operand_equal_p for memory references and implement > separate comparsion that does two separate things: > 1) compare semantics of the access path (i.e. how big chunk of memory > with what alignment is read) > For constant offset addresses this is basicaly > get_ref_base_and-offset > 2) what additional info tree-ssa-alias can use. > (dependence clique, ref alias type, base alias type and the chain of > refs used by nonoverlapping_refs_p and friends). > I have prototype patch for that somewhere. Sure, I think this is what the old code did (or tried to do). I see we're quite elaborately hashing memory ref trees but do almost nothing for regular stmt. That seems backwards since we'd want to do almost nothing for memory refs to be able to do the above fancy at compare stage. > For ADDR_EXPR we are basically interested in base and offset only. > > Honza > > > > 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(-) > > > > > > > >
