> 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?
It seems to me that most of unmerged functions are like that. Marin has some stats, I started -fdump-ipa-icf-details on Firefox now and will send some examples. > > > 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. Yep, hashing ODR names if available was also something I tought of. Problem here is that only types with linkage have ODR names > > 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). Yep, I kind of returned original comparsion logic except for adding the missing the access path comparsion. I will finish the patch. Honza > > 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(-) > > > > > > > > > >
