ICF fixes

ICF fixes

[Jan Hubicka]

Hi,
this patchs fixes few issues in ipa-icf.  First I drop the use of TYPE_CANONICAL
because that is no longer part of type compatibility machinery.
Second I also hash TYPE_MODE for aggregates, becuase we now always require a match
and I check that we don't match types that are incomplete where this would give
false negatives.

Second change is in func_checker::compatible_types_p where I disabled comparing
of alias sets of types that do not have alias sets (and thus can never be read).
I hoped to switch ipa-icf-gimple.c to use of operand_equal_p and drop all such
wrong type mathcing, but I suppose it will need to wait for next stage1. 

Bootstrapped/regtested x86_64-linux, comitted.

Honza
	
	* ipa-icf.c (sem_item::add_type): Do not look for TYPE_CANONICAL;
	do not check AGGREGATE_TYPE_P when adding TYPE_MODE;
	Check that all record types are complete.
	* ipa-icf-gimple.c (func_checker::compatible_types_p): Do not
	compare alias sets for types w/o alias sets.
Index: ipa-icf.c
===================================================================
--- ipa-icf.c	(revision 230683)
+++ ipa-icf.c	(working copy)
@@ -1543,11 +1543,8 @@ sem_item::add_type (const_tree type, inc
     }
 
   type = TYPE_MAIN_VARIANT (type);
-  if (TYPE_CANONICAL (type))
-    type = TYPE_CANONICAL (type);
 
-  if (!AGGREGATE_TYPE_P (type))
-    hstate.add_int (TYPE_MODE (type));
+  hstate.add_int (TYPE_MODE (type));
 
   if (TREE_CODE (type) == COMPLEX_TYPE)
     {
@@ -1574,6 +1571,7 @@ sem_item::add_type (const_tree type, inc
     }
   else if (RECORD_OR_UNION_TYPE_P (type))
     {
+      gcc_checking_assert (COMPLETE_TYPE_P (type));
       hashval_t *val = optimizer->m_type_hash_cache.get (type);
 
       if (!val)
Index: ipa-icf-gimple.c
===================================================================
--- ipa-icf-gimple.c	(revision 230683)
+++ ipa-icf-gimple.c	(working copy)
@@ -233,7 +233,15 @@ func_checker::compatible_types_p (tree t
   if (!types_compatible_p (t1, t2))
     return return_false_with_msg ("types are not compatible");
 
-  if (get_alias_set (t1) != get_alias_set (t2))
+  /* We do a lot of unnecesary matching of types that are not being
+     accessed and thus do not need to be compatible.  In longer term we should
+     remove these checks on all types which are not accessed as memory
+     locations.
+
+     For time being just avoid calling get_alias_set on types that are not
+     having alias sets defined at all.  */
+  if (type_with_alias_set_p (t1) && type_with_alias_set_p (t2)
+      && get_alias_set (t1) != get_alias_set (t2))
     return return_false_with_msg ("alias sets are different");
 
   return true;

Re: ICF fixes

[Eric Botcazou]

> this patchs fixes few issues in ipa-icf.  First I drop the use of
> TYPE_CANONICAL because that is no longer part of type compatibility
> machinery.

That doesn't seem right, as the check on TYPE_CANONICAL was restored for 
aggregate types because of the serious issues we found.

> Second I also hash TYPE_MODE for aggregates, becuase we now always require a
> match and I check that we don't match types that are incomplete where this
> would give false negatives.

We clearly know that TYPE_MODE is far from being sufficient for aggregates.

-- 
Eric Botcazou

Re: ICF fixes

[Jan Hubicka]

> > this patchs fixes few issues in ipa-icf.  First I drop the use of
> > TYPE_CANONICAL because that is no longer part of type compatibility
> > machinery.
> 
> That doesn't seem right, as the check on TYPE_CANONICAL was restored for 
> aggregate types because of the serious issues we found.
> 
> > Second I also hash TYPE_MODE for aggregates, becuase we now always require a
> > match and I check that we don't match types that are incomplete where this
> > would give false negatives.
> 
> We clearly know that TYPE_MODE is far from being sufficient for aggregates.

Yes, this is just a hash.  It should give same equivalence as:
/* Return true if types are compatible from perspective of ICF.  */
bool
func_checker::compatible_types_p (tree t1, tree t2)
{
  if (TREE_CODE (t1) != TREE_CODE (t2))
    return return_false_with_msg ("different tree types");

  if (TYPE_RESTRICT (t1) != TYPE_RESTRICT (t2))
    return return_false_with_msg ("restrict flags are different");

  if (!types_compatible_p (t1, t2))
    return return_false_with_msg ("types are not compatible");

  if (get_alias_set (t1) != get_alias_set (t2))
    return return_false_with_msg ("alias sets are different");

  return true;
}

which does the TYPE_CANONICAL test via types_compatible_p and additionally
check alias set sequivalence.  (this checking needs to be dismantled for
semantic equivalence done via operand_equal_p and TBAA equivalcne done for
loads/stores, but only next stage1).

The hash needs to be stable from compilation to WPA to ltrans.  The problem
here is that TYPE_CANONICAL changes between compilatoin stage and WPA and
thus we end up with different hashes for otherwise same types.
(such as TYPE_CANONICAL of ptrdiff_t being ptrdiff_t at compile stage and
size_t at lto stage).  This corrupts the hashtable.  

The function does deep recurse into aggregates and hash in the fields later
on.

Honza
> 
> -- 
> Eric Botcazou