diff mbox

Do less generous pointer globbing in alias.c

Message ID 20150527052850.GB88897@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka May 27, 2015, 5:28 a.m. UTC
Hi,
this patch makes it possible for non-LTO alias oracle to TBAA disambiguate
pointer types. It makes void * conflicting with all of them and does not put it
to alias set 0. It also preserves the property that qualifiers of pointer-to
type should not matter to determine the alias set and that pointer to array is
same as pointer to array element.  Finally it makes pointer void * to be
equivalent to void ** (and more *) and to types with structural equality only.

I think those are all globbing rules we discussed for the non-LTO patch.

It does two things.  First is kind of "canonicalization" where for a given pointer
it looks for non-pointer pointed-to type and then rebuilds is without qualifiers.
This is fast, because build_pointer_type will reuse existing types.

It makes void * to conflict with everyting by making its alias set to be subset
of alias set of any other pointer.  This means that writes to void * conflict
with writes to any other pointer without really need to glob all the pointers
to one equivalence class.

This patch makes quite some difference on C++.  For example on deal II the TBAA
stats reports 4344358 disambiguations and 7008576 queries, while with the patch
we get 5368737 and 5687399 queries (I did not chose deal II for reason, it is
just random C++ file)

The patch bootstrap and regtests ppc64le-linux with the following testsuite
differences:
@@ -30,7 +30,9 @@
 FAIL: c-c++-common/asan/null-deref-1.c   -O3 -g  output pattern test, is ASAN:SIGSEGV
 FAIL: c-c++-common/asan/null-deref-1.c   -Os  output pattern test, is ASAN:SIGSEGV
 FAIL: gcc.dg/cpp/_Pragma3.c (test for excess errors)
+XPASS: gcc.dg/alias-8.c  (test for warnings, line 11)
 FAIL: gcc.dg/loop-8.c scan-rtl-dump-times loop2_invariant "Decided" 1
+FAIL: gcc.dg/pr62167.c scan-tree-dump-not pre "Removing basic block"
 FAIL: gcc.dg/sms-4.c scan-rtl-dump-times sms "SMS succeeded" 1
 XPASS: gcc.dg/guality/example.c   -O0  execution test
 XPASS: gcc.dg/guality/example.c   -O1  execution test
@@ -304,6 +306,9 @@
 FAIL: c-c++-common/asan/null-deref-1.c   -O3 -g  output pattern test, is ASAN:SIGSEGV
 FAIL: g++.dg/cpp1y/vla-initlist1.C  -std=gnu++11 execution test
 FAIL: g++.dg/cpp1y/vla-initlist1.C  -std=gnu++14 execution test
+FAIL: g++.dg/ipa/ipa-icf-4.C  -std=gnu++11  scan-ipa-dump icf "Equal symbols: [67]"
+FAIL: g++.dg/ipa/ipa-icf-4.C  -std=gnu++14  scan-ipa-dump icf "Equal symbols: [67]"
+FAIL: g++.dg/ipa/ipa-icf-4.C  -std=gnu++98  scan-ipa-dump icf "Equal symbols: [67]"

ipa-icf-4 is about alias info now being more perceptive to block the merging.
pr62167 seems just confused.  The template checks that memory stores are not
unified.  It looks for BB removal message, but with the patch we get:
  <bb 2>:
  node.next = 0B;
  head.0_4 = head;
  node.prev = head.0_4;
  head.0_4->first = &node;
  k.1_7 = k;
  h_8 = &heads[k.1_7];
  heads[2].first = 0B;
  if (head.0_4 == h_8)
    goto <bb 3>;
  else
    goto <bb 5>;

  <bb 5>:
  goto <bb 4>;

  <bb 3>:
  p_10 = MEM[(struct head *)&heads][k.1_7].first;

  <bb 4>:
  # p_1 = PHI <p_10(3), &node(5)>
  _11 = p_1 != 0B;
  _12 = (int) _11;
  return _12;

before PR, the message is about the bb 5 sitting at critical edge removed.
The TBAA incompatible load it looks for is optimized away by FRE:
  head->first = &node;

  struct node *n = head->first;

  struct head *h = &heads[k];

  heads[2].first = n->next;

  if ((void*)n->prev == (void *)h)
    p = h->first;
  else
    /* Dead tbaa-unsafe load from ((struct node *)&heads[2])->next.  */
    p = n->prev->next;

here n is known to be head->first that is known to be &node.
The testcase runtime checks that result is Ok and passes.

Bootstrapped/regtested ppc64le-linux.

	* alias.c (get_alias_set): Do not glob all pointer types into one;
	just produce euqivalence classes based on canonical type of pointed
	type type; make void * equivalent to void **.
	(record_component_aliases): Make void * to conflict with all other
	pointer types.

Comments

Jan Hubicka May 27, 2015, 6:33 a.m. UTC | #1
> Hi,
> this patch makes it possible for non-LTO alias oracle to TBAA disambiguate
> pointer types. It makes void * conflicting with all of them and does not put it
> to alias set 0. It also preserves the property that qualifiers of pointer-to
> type should not matter to determine the alias set and that pointer to array is
> same as pointer to array element.  Finally it makes pointer void * to be
> equivalent to void ** (and more *) and to types with structural equality only.
> 
> I think those are all globbing rules we discussed for the non-LTO patch.
> 
> It does two things.  First is kind of "canonicalization" where for a given pointer
> it looks for non-pointer pointed-to type and then rebuilds is without qualifiers.
> This is fast, because build_pointer_type will reuse existing types.
> 
> It makes void * to conflict with everyting by making its alias set to be subset
> of alias set of any other pointer.  This means that writes to void * conflict
> with writes to any other pointer without really need to glob all the pointers
> to one equivalence class.
> 
> This patch makes quite some difference on C++.  For example on deal II the TBAA
> stats reports 4344358 disambiguations and 7008576 queries, while with the patch
> we get 5368737 and 5687399 queries (I did not chose deal II for reason, it is
> just random C++ file)
Actually there is oversight in my patch - the number of queries is really
number of non-disambiguations.  I will fix that as obvious tomorrow.  In both
cases the number of querries is about 11M.  The increase in number of
disambiguations is 23% (earlier patch did over 30% for Firefox)

Honza
Richard Biener May 27, 2015, 9:04 a.m. UTC | #2
On Wed, 27 May 2015, Jan Hubicka wrote:

> Hi, this patch makes it possible for non-LTO alias oracle to TBAA 
> disambiguate pointer types. It makes void * conflicting with all of them 
> and does not put it to alias set 0. It also preserves the property that 
> qualifiers of pointer-to type should not matter to determine the alias 
> set and that pointer to array is same as pointer to array element.  
> Finally it makes pointer void * to be equivalent to void ** (and more *) 
> and to types with structural equality only.

void * should be equivalent to incomplete-type * as well.

> I think those are all globbing rules we discussed for the non-LTO patch.
> 
> It does two things.  First is kind of "canonicalization" where for a given pointer
> it looks for non-pointer pointed-to type and then rebuilds is without qualifiers.
> This is fast, because build_pointer_type will reuse existing types.
> 
> It makes void * to conflict with everyting by making its alias set to be subset
> of alias set of any other pointer.  This means that writes to void * conflict
> with writes to any other pointer without really need to glob all the pointers
> to one equivalence class.

I think you need to make each pointer alias-set a subset of the one of 
void * as well because both of the following is valid:

  *(void *)p = ...
  ... = *(int *)p;

and

  *(int *)p = ...
  ... = *(void *)p;

not sure if it's possible to create a testcase that fails if you do
subsetting only one-way (because alias_sets_conflict queries both
ways and I think alias_set_subset_of is not used very much, only
by tree-ssa-alias.c:aliasing_component_refs_p which won't ever
use it on two pointer alias sets).  In theory true vs. anti-dependence
should use alias_set_subset_of and trigger the above cases.  But
as those queries are done wrong a lot (in the past?) we use
alias_sets_conflict there.

For efficiency you could use a new flag similar to has_zero_child
in alias_set_entry_d ... 

More comments inline below

> This patch makes quite some difference on C++.  For example on deal II the TBAA
> stats reports 4344358 disambiguations and 7008576 queries, while with the patch
> we get 5368737 and 5687399 queries (I did not chose deal II for reason, it is
> just random C++ file)
> 
> The patch bootstrap and regtests ppc64le-linux with the following testsuite
> differences:
> @@ -30,7 +30,9 @@
>  FAIL: c-c++-common/asan/null-deref-1.c   -O3 -g  output pattern test, is ASAN:SIGSEGV
>  FAIL: c-c++-common/asan/null-deref-1.c   -Os  output pattern test, is ASAN:SIGSEGV
>  FAIL: gcc.dg/cpp/_Pragma3.c (test for excess errors)
> +XPASS: gcc.dg/alias-8.c  (test for warnings, line 11)
>  FAIL: gcc.dg/loop-8.c scan-rtl-dump-times loop2_invariant "Decided" 1
> +FAIL: gcc.dg/pr62167.c scan-tree-dump-not pre "Removing basic block"
>  FAIL: gcc.dg/sms-4.c scan-rtl-dump-times sms "SMS succeeded" 1
>  XPASS: gcc.dg/guality/example.c   -O0  execution test
>  XPASS: gcc.dg/guality/example.c   -O1  execution test
> @@ -304,6 +306,9 @@
>  FAIL: c-c++-common/asan/null-deref-1.c   -O3 -g  output pattern test, is ASAN:SIGSEGV
>  FAIL: g++.dg/cpp1y/vla-initlist1.C  -std=gnu++11 execution test
>  FAIL: g++.dg/cpp1y/vla-initlist1.C  -std=gnu++14 execution test
> +FAIL: g++.dg/ipa/ipa-icf-4.C  -std=gnu++11  scan-ipa-dump icf "Equal symbols: [67]"
> +FAIL: g++.dg/ipa/ipa-icf-4.C  -std=gnu++14  scan-ipa-dump icf "Equal symbols: [67]"
> +FAIL: g++.dg/ipa/ipa-icf-4.C  -std=gnu++98  scan-ipa-dump icf "Equal symbols: [67]"
> 
> ipa-icf-4 is about alias info now being more perceptive to block the merging.
> pr62167 seems just confused.  The template checks that memory stores are not
> unified.  It looks for BB removal message, but with the patch we get:
>   <bb 2>:
>   node.next = 0B;
>   head.0_4 = head;
>   node.prev = head.0_4;
>   head.0_4->first = &node;
>   k.1_7 = k;
>   h_8 = &heads[k.1_7];
>   heads[2].first = 0B;
>   if (head.0_4 == h_8)
>     goto <bb 3>;
>   else
>     goto <bb 5>;
> 
>   <bb 5>:
>   goto <bb 4>;
> 
>   <bb 3>:
>   p_10 = MEM[(struct head *)&heads][k.1_7].first;
> 
>   <bb 4>:
>   # p_1 = PHI <p_10(3), &node(5)>
>   _11 = p_1 != 0B;
>   _12 = (int) _11;
>   return _12;
> 
> before PR, the message is about the bb 5 sitting at critical edge removed.
> The TBAA incompatible load it looks for is optimized away by FRE:
>   head->first = &node;
> 
>   struct node *n = head->first;
> 
>   struct head *h = &heads[k];
> 
>   heads[2].first = n->next;
> 
>   if ((void*)n->prev == (void *)h)
>     p = h->first;
>   else
>     /* Dead tbaa-unsafe load from ((struct node *)&heads[2])->next.  */
>     p = n->prev->next;
> 
> here n is known to be head->first that is known to be &node.
> The testcase runtime checks that result is Ok and passes.
> 
> Bootstrapped/regtested ppc64le-linux.
> 
> 	* alias.c (get_alias_set): Do not glob all pointer types into one;
> 	just produce euqivalence classes based on canonical type of pointed
> 	type type; make void * equivalent to void **.
> 	(record_component_aliases): Make void * to conflict with all other
> 	pointer types.
> Index: alias.c
> ===================================================================
> --- alias.c	(revision 223633)
> +++ alias.c	(working copy)
> @@ -903,35 +906,79 @@ get_alias_set (tree t)
>       the pointed-to types.  This issue has been reported to the
>       C++ committee.
>  
> -     In addition to the above canonicalization issue, with LTO
> -     we should also canonicalize `T (*)[]' to `T *' avoiding
> -     alias issues with pointer-to element types and pointer-to
> -     array types.
> -
> -     Likewise we need to deal with the situation of incomplete
> -     pointed-to types and make `*(struct X **)&a' and
> -     `*(struct X {} **)&a' alias.  Otherwise we will have to
> -     guarantee that all pointer-to incomplete type variants
> -     will be replaced by pointer-to complete type variants if
> -     they are available.
> -
> -     With LTO the convenient situation of using `void *' to
> -     access and store any pointer type will also become
> -     more apparent (and `void *' is just another pointer-to
> -     incomplete type).  Assigning alias-set zero to `void *'
> -     and all pointer-to incomplete types is a not appealing
> -     solution.  Assigning an effective alias-set zero only
> -     affecting pointers might be - by recording proper subset
> -     relationships of all pointer alias-sets.
> -
> -     Pointer-to function types are another grey area which
> -     needs caution.  Globbing them all into one alias-set
> -     or the above effective zero set would work.
> -
> -     For now just assign the same alias-set to all pointers.
> -     That's simple and avoids all the above problems.  */
> -  else if (POINTER_TYPE_P (t)
> -	   && t != ptr_type_node)
> +     For this reason go to canonical type of the unqalified pointer type.
> +     Until GCC 6 this code set all pointers sets to have alias set of
> +     ptr_type_node but that is a bad idea, because it prevents disabiguations
> +     in between pointers.  For Firefox this accounts about 20% of all
> +     disambiguations in the program.  */
> +  else if (POINTER_TYPE_P (t) && t != ptr_type_node && !in_lto_p)
> +    {
> +      tree p;
> +      auto_vec <bool, 8> reference;
> +
> +      /* Unnest all pointers and references.
> +         We also want to make pointer to array equivalent to pointer to its
> +         element. So skip all array types, too.  */
> +      for (p = t; POINTER_TYPE_P (p)
> +	   || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p));

As we glob vector<int> to int we should look through VECTOR_TYPE as well
here.

> +	   p = TREE_TYPE (p))
> +	{
> +	  if (TREE_CODE (p) == REFERENCE_TYPE)
> +	    reference.safe_push (true);
> +	  if (TREE_CODE (p) == POINTER_TYPE)
> +	    reference.safe_push (false);
> +	}
> +      p = TYPE_MAIN_VARIANT (p);
> +
> +      /* Make void * compatible with char * and also void **.
> +	 Programs are commonly violating TBAA by this.
> +
> +	 We also make void * to conflict with every pointer
> +	 (see record_component_aliases) and thus it is safe it to use it for
> +	 pointers to types with TYPE_STRUCTURAL_EQUALITY_P.  */
> +      if (TREE_CODE (p) == VOID_TYPE || TYPE_STRUCTURAL_EQUALITY_P (p))

This should also conver incomplete types (and luckily void is incomplete).
I'm quite sure TYPE_STRUCTURAL_EQUALITY_P doesn't cover all incomplete
types.

> +	set = get_alias_set (ptr_type_node);
> +      else
> +	{
> +	  /* Rebuild pointer type from starting from canonical types using
> +	     unqualified pointers and references only.  This way all such
> +	     pointers will have the same alias set and will conflict with
> +	     each other.
> +
> +	     Most of time we already have pointers or references of a given type.
> +	     If not build new one just to be sure that if someone later (probably
> +	     only middle-end can, as we should assign all alias classes only after
> +	     finishing translation unit) builds the pointer type, the canonical type
> +	     will match.  */
> +	  p = TYPE_CANONICAL (p);
> +	  while (!reference.is_empty ())
> +	    {
> +	      if (reference.pop ())
> +		p = build_reference_type (p);
> +	      else
> +		p = build_pointer_type (p);
> +	      p = TYPE_CANONICAL (TYPE_MAIN_VARIANT (p));

err - the pointer you build should be a main variant already, and
canonical as well.  So the assert belongs here ;)

Btw, you should definitely glob POINTER_TYPE and REFERENCE_TYPE.

class T;
void foo (T **);

void bar(T *p)
{
  T*&r = p;
  foo (&r)
}

compiles fine (so foo can store T* to r).

Please verify that your code globs at least those pointer types
the former c-family get_alias_set langhook globbed.

> +	    }
> +          gcc_checking_assert (TYPE_CANONICAL (p) == p);
> +
> +	  /* Assign the alias set to both p and t.
> +	     We can not call get_alias_set (p) here as that would triger
> +	     infinite recursion when p == t.
> +	     In other cases it would just trigger unnecesary legwork of
> +	     rebuilding the pointer again.  */
> +	  if (TYPE_ALIAS_SET_KNOWN_P (p))
> +	    /* Return early; we do not need to record component aliases.  */
> +	    return TYPE_ALIAS_SET (t) = TYPE_ALIAS_SET (p);

Well, see below.  I don't like that coding-style btw, so just do

         set = TYPE_ALIAS_SET (p);

> +	  else
> +	    {
> +	      set = new_alias_set ();
> +	      TYPE_ALIAS_SET (p) = set;
> +	    }
> +	}
> +    }
> +  /* In LTO the rules above needs to be part of canonical type machinery.
> +     For now just punt.  */

I see no reason for punting for LTO here.

Btw, please check if SPEC perl still works without -fno-strict-aliasing
(it finally did after the change to do pointer globbing).

> +  else if (POINTER_TYPE_P (t) && t != ptr_type_node && in_lto_p)
>      set = get_alias_set (ptr_type_node);
>  
>    /* Otherwise make a new alias set for this type.  */
> @@ -950,7 +997,8 @@ get_alias_set (tree t)
>  
>    /* If this is an aggregate type or a complex type, we must record any
>       component aliasing information.  */
> -  if (AGGREGATE_TYPE_P (t) || TREE_CODE (t) == COMPLEX_TYPE)
> +  if (AGGREGATE_TYPE_P (t) || TREE_CODE (t) == COMPLEX_TYPE
> +      || TREE_CODE (t) == POINTER_TYPE)
>      record_component_aliases (t);

Err...
>  
>    return set;
> @@ -1050,6 +1098,26 @@ record_component_aliases (tree type)
>  
>    switch (TREE_CODE (type))
>      {
> +    /* We want void * to be compatible with any other pointer without really
> +       dropping it to alias set 0. Doing so would make it compatible with
> +       all non-pointer types too.
> +
> +       Make thus ptr_type_node to be a component of every pointer type.
> +       Tus memory operations of type "void *" conflict with operations of type
> +       "T *" without impossing a conflict with memory operations of type "Q *"
> +       in case T and Q do not conflict.
> + 
> +       This is not strictly necessary by the language standards, but avoids
> +       common type punning mistakes.  In addition to that, we need the existence
> +       of such universal pointer to implement Fortran's C_PTR type (which is
> +       defined as type compatible with all C pointers) and we use it in
> +       get_alias_set to give alias set to pointers to types without
> +       canonical types (those are typically nameless incomplete types)
> +       that needs to be also compatible with each other and with pointers to
> +       complete types.  */
> +    case POINTER_TYPE:
> +      record_alias_subset (superset, get_alias_set (ptr_type_node));
> +      break;

... I'd rather add this to the pointer handling in get_alias_set directly.
It's not really components.  Also see my comment about symmetry.

>      case RECORD_TYPE:
>      case UNION_TYPE:
>      case QUAL_UNION_TYPE:
> 
>
Jan Hubicka May 27, 2015, 2:42 p.m. UTC | #3
> > Hi, this patch makes it possible for non-LTO alias oracle to TBAA 
> > disambiguate pointer types. It makes void * conflicting with all of them 
> > and does not put it to alias set 0. It also preserves the property that 
> > qualifiers of pointer-to type should not matter to determine the alias 
> > set and that pointer to array is same as pointer to array element.  
> > Finally it makes pointer void * to be equivalent to void ** (and more *) 
> > and to types with structural equality only.
> 
> void * should be equivalent to incomplete-type * as well.

It will be in conflict with struct FOO * when FOO is incomplete.
In non-LTO build struct FOO * do not need to conflict wqith struct BAR *.
Or do I miss something here?
> 
> > I think those are all globbing rules we discussed for the non-LTO patch.
> > 
> > It does two things.  First is kind of "canonicalization" where for a given pointer
> > it looks for non-pointer pointed-to type and then rebuilds is without qualifiers.
> > This is fast, because build_pointer_type will reuse existing types.
> > 
> > It makes void * to conflict with everyting by making its alias set to be subset
> > of alias set of any other pointer.  This means that writes to void * conflict
> > with writes to any other pointer without really need to glob all the pointers
> > to one equivalence class.
> 
> I think you need to make each pointer alias-set a subset of the one of 
> void * as well because both of the following is valid:
> 
>   *(void *)p = ...
>   ... = *(int *)p;
> 
> and
> 
>   *(int *)p = ...
>   ... = *(void *)p;

Yes, so is

struct foo {struct bar a;};

  a.a = ...
  ... = a;

and

  a = ...
  ... = a.a;

this is why conflict is symmetrization of the subset relation.

You can not record both edges into the DAG, because record_alias_subset
compute transitive closure and it would end up in loop.  I will be hapy
to add the extra flag (has_zero_child), but I would like to make it
clear it an optimization.
> 
> not sure if it's possible to create a testcase that fails if you do
> subsetting only one-way (because alias_sets_conflict queries both
> ways and I think alias_set_subset_of is not used very much, only
> by tree-ssa-alias.c:aliasing_component_refs_p which won't ever
> use it on two pointer alias sets).  In theory true vs. anti-dependence

Yep, I noticed that subsets are querried by tree-ssa-alias.  I will try to
think if it is safe WRT the code above.

> should use alias_set_subset_of and trigger the above cases.  But
> as those queries are done wrong a lot (in the past?) we use
> alias_sets_conflict there.
> 
> For efficiency you could use a new flag similar to has_zero_child
> in alias_set_entry_d ... 

Yes, I can use new flag, but it should be unnecesary.  The alias set 0
is also just common subset of all aliases (that is not done by the code).
> 
> I see no reason for punting for LTO here.

I would rather go with non-LTO first and work on solving the canonical type
issues.  Yes, I think it should work for LTO as it is and I bootstrapped and
regtested it.  I only wanted to do one step at a time.

What I do not like is that build_pointer_type simply does not do the right
thing here.  Consdier

struct a {int a};
struct b {char b};

Now if you LTO in struct *a and struct *b their canonical type will be the same.
If you call build_pointer_type, it will assign different canonical types to them.

This does not lead to wrong code, because incomplete types no longer get
TYPE_CANONICAL, but I would like first to chase out the bugs out of canonical
type computation and arrange middle-end build pointer types to be the same as
LTOed-in pointer types.
> 
> Btw, please check if SPEC perl still works without -fno-strict-aliasing
> (it finally did after the change to do pointer globbing).

OK, I have SPEC perl available, so I will do.

I am teaching now, but so will reply in detail afterwards. I was just hoping
to discuss the symmetry thing above.  I think it is not needed.

I have no problem with moving the subset code to get_alias_set and will update
the patch (including testsuite compensation).

Honza
Jan Hubicka May 27, 2015, 2:58 p.m. UTC | #4
> Yes, so is
> 
> struct foo {struct bar a;};
> 
>   a.a = ...
>   ... = a;
> 
> and
> 
>   a = ...
>   ... = a.a;
> 
> this is why conflict is symmetrization of the subset relation.


OK the statement above is true, but subsets alone are not quite right for use
in aliasing_component_refs_p

 void *a;
 char **ptr=&a;
 *ptr = ....

is defined for us, but the structure-substructure equivalent is not.
I will implement the variant with extra flag after teaching and send updated
patch.

Thanks,
Honza
Jan Hubicka May 27, 2015, 3:04 p.m. UTC | #5
> > Yes, so is
> > 
> > struct foo {struct bar a;};
> > 
> >   a.a = ...
> >   ... = a;
> > 
> > and
> > 
> >   a = ...
> >   ... = a.a;
> > 
> > this is why conflict is symmetrization of the subset relation.
> 
> 
> OK the statement above is true, but subsets alone are not quite right for use
> in aliasing_component_refs_p
> 
>  void *a;
>  char **ptr=&a;
>  *ptr = ....
> 
> is defined for us, but the structure-substructure equivalent is not.
> I will implement the variant with extra flag after teaching and send updated
> patch.

Hmm, what about

union t {int a; char b;};

int a;
uniont t *ptr=&a;
*ptr = ...

If we want to define this, aliasing_component_refs_p would IMO need to be symmetrized, too.
I am happy leaving this undefined.

Honza
Richard Biener May 27, 2015, 3:25 p.m. UTC | #6
On May 27, 2015 5:04:13 PM GMT+02:00, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > Yes, so is
>> > 
>> > struct foo {struct bar a;};
>> > 
>> >   a.a = ...
>> >   ... = a;
>> > 
>> > and
>> > 
>> >   a = ...
>> >   ... = a.a;
>> > 
>> > this is why conflict is symmetrization of the subset relation.
>> 
>> 
>> OK the statement above is true, but subsets alone are not quite right
>for use
>> in aliasing_component_refs_p
>> 
>>  void *a;
>>  char **ptr=&a;
>>  *ptr = ....
>> 
>> is defined for us, but the structure-substructure equivalent is not.
>> I will implement the variant with extra flag after teaching and send
>updated
>> patch.
>
>Hmm, what about
>
>union t {int a; char b;};
>
>int a;
>uniont t *ptr=&a;
>*ptr = ...
>
>If we want to define this, aliasing_component_refs_p would IMO need to
>be symmetrized, too.
>I am happy leaving this undefined.

Globbing all pointers was soo  simple... :)

Note that we are in the middle-end here and have to find cross-language common grounds.  People may experience regressions towards the previous globbing so I guess the question is which is the globbing we want to remove - that is, what makes the most difference in code-generation?

Richard.

>Honza
Jan Hubicka May 27, 2015, 3:42 p.m. UTC | #7
> >Hmm, what about
> >
> >union t {int a; char b;};
> >
> >int a;
> >uniont t *ptr=&a;
> >*ptr = ...
> >
> >If we want to define this, aliasing_component_refs_p would IMO need to
> >be symmetrized, too.
> >I am happy leaving this undefined.
> 
> Globbing all pointers was soo  simple... :)

Indeed, but too restrictive ;)
The testcase above is not about globbing pointers, I do not think it is going
to be handled in defined manner by mainline (or any release).
> 
> Note that we are in the middle-end here and have to find cross-language common grounds.  People may experience regressions towards the previous globbing so I guess the question is which is the globbing we want to remove - that is, what makes the most difference in code-generation?

Yes, I expect to see some PRs with regress towards the previous globbing.  I
think the globbing as proposed by my patch should be generous enough for common
bugs in user code and it is quite easy to add new rules on demand.

For high-level C++ code definitely the most important point is that you have
many different class types and we care about differentiating these (struct *a
wrt struct *b).  We also want to make difference between vtbl pointer (that is
pointer to array of functions) and other stuff.

I think I will modify the patch the following way:
1) I will move the code adding subset to get_alias_set
2) I will add flag "is_pointer" to alias set datastructure
3) I will make alias_set_subset_of to additionally consider
   every "is_pointer" set to be subset of alias set of ptr_type_node's set.

This will fix the symmetry with void *a; variable and incompatible pointer write.

We need to do two things - arrange alias set to be subset of all pointer's alias sets
and all their superset and force equivalence between pointer alias sets.
While the first can be also done by means of special flag "contains_pointer"
I think it is cleaner to keep the DAG reprsented explicitely.  After all we do not
have that many alias sets and the hash table lookups should be fast enough
(we may special case lookup in hash of size 1)

Hona
> 
> Richard.
> 
> >Honza
>
Andreas Schwab May 30, 2015, 9:08 p.m. UTC | #8
Jan Hubicka <hubicka@ucw.cz> writes:

> 	* alias.c (alias_set_entry_d): Add is_pointer and has_pointer.
> 	(alias_stats): Add num_universal.
> 	(alias_set_subset_of): Special case pointers; be ready for NULL
> 	children.
> 	(alias_sets_conflict_p): Special case pointers; be ready for NULL
> 	children.
> 	(init_alias_set_entry): Break out from ...
> 	(record_alias_subset): ... here; propagate new fields;
> 	allocate children only when really needed.
> 	(get_alias_set): Do less generous pointer globbing.
> 	(dump_alias_stats_in_alias_c): Update statistics.
> 	* gcc.dg/alias-8.c: Do not xfail.
> 	* gcc.dg/pr62167.c: Prevent FRE.
> 	* gcc.dg/alias-14.c: New testcase.

This is causing a miscompilation of the stage2 compiler on ia64.

Andreas.
Jan Hubicka May 30, 2015, 9:52 p.m. UTC | #9
> Jan Hubicka <hubicka@ucw.cz> writes:
> 
> > 	* alias.c (alias_set_entry_d): Add is_pointer and has_pointer.
> > 	(alias_stats): Add num_universal.
> > 	(alias_set_subset_of): Special case pointers; be ready for NULL
> > 	children.
> > 	(alias_sets_conflict_p): Special case pointers; be ready for NULL
> > 	children.
> > 	(init_alias_set_entry): Break out from ...
> > 	(record_alias_subset): ... here; propagate new fields;
> > 	allocate children only when really needed.
> > 	(get_alias_set): Do less generous pointer globbing.
> > 	(dump_alias_stats_in_alias_c): Update statistics.
> > 	* gcc.dg/alias-8.c: Do not xfail.
> > 	* gcc.dg/pr62167.c: Prevent FRE.
> > 	* gcc.dg/alias-14.c: New testcase.
> 
> This is causing a miscompilation of the stage2 compiler on ia64.

Hmm, lovely :( 
According to GCC compile farm wiki, GCC 60 and 66 are ia64 but they are not.
I am not sure I have access to working ia64 box.  Can you possibly help me
to debug this?  Is it devirtualization related?
With earlier versions of the patch I run into issue of ipa-icf ICEing in 
sem_function::parse because ipa_polymorphic_call_context::get_dynamic_type
missed initialization of VPTR.  I pushed out just partial fix to the issue
as I want to test it on Firefox first and I am running into intresting
issues with Firefox LTO right now (unrelated to the aliasing).
With some luck it is the same problem because IA-64 has different representation
of function pointers.

Honza
> 
> Andreas.
> 
> -- 
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."
Andreas Schwab May 31, 2015, 7:13 a.m. UTC | #10
Jan Hubicka <hubicka@ucw.cz> writes:

> I am not sure I have access to working ia64 box.  Can you possibly help me
> to debug this?  Is it devirtualization related?

Here's a backtrace:

0x4000000000422311 in bitmap_obstack_alloc_stat (
    bit_obstack=0x400000000194cf20 <ipa_icf::sem_item_optimizer::parse_funcs_and_vars()+1312>) at ../../gcc/bitmap.c:288
288         bit_obstack->heads = (struct bitmap_head *) map->first;
(gdb) bt full
#0  0x4000000000422311 in bitmap_obstack_alloc_stat (
    bit_obstack=0x400000000194cf20 <ipa_icf::sem_item_optimizer::parse_funcs_and_vars()+1312>) at ../../gcc/bitmap.c:288
        map = 0x8401880020420020
#1  0x400000000192bf10 in ipa_icf::sem_item::setup (this=0x2000000000ab0350, 
    stack=0x400000000194cf20 <ipa_icf::sem_item_optimizer::parse_funcs_and_vars()+1312>) at ../../gcc/ipa-icf.c:224
No locals.
#2  0x400000000194c730 in ipa_icf::sem_variable::sem_variable (
    this=0x2000000000ab0350, node=0x6000000000309430, _hash=3188864, 
    stack=0x400000000194cf20 <ipa_icf::sem_item_optimizer::parse_funcs_and_vars()+1312>) at ../../gcc/ipa-icf.c:1847
No locals.
#3  0x400000000194c690 in sem_function (stack=0x600000000008c4d8 <dump_flags>, 
    hash=0, node=0x7fffffff, this=0x2000000000966920)
    at ../../gcc/ipa-icf.c:286
No locals.
#4  ipa_icf::sem_function::parse (node=0x7fffffff, 
    stack=0x600000000008c4d8 <dump_flags>) at ../../gcc/ipa-icf.c:1701
        fndecl = <optimized out>
        func = <optimized out>
        __FUNCTION__ = "parse"
#5  0x4000000000d24110 in execute_ipa_summary_passes (
    ipa_pass=0x2000000000ab0350) at ../../gcc/passes.c:2143
        pass = 0x2000000000ab0350
#6  0x400000000194eb00 in ipa_icf::ipa_icf_generate_summary ()
    at ../../gcc/ipa-icf.c:3502
No locals.
#7  0x60000000001230b0 in default_target_ira_int ()
No symbol table info available.
#8  0x400000000192bf10 in ipa_icf::sem_item::setup (this=0xffffffffffffffc0, 
    stack=0x0) at ../../gcc/ipa-icf.c:224
No locals.

Andreas.
Andreas Schwab May 31, 2015, 8:39 a.m. UTC | #11
The problem is that ipa_icf::sem_function::parse is lacking the f->init
call and the function epilog, falling through to the next function that
happens to follow.

Revision r223897:

Dump of assembler code for function ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*):
   0x400000000194c480 <+0>:     [MMI]       alloc r36=ar.pfs,12,6,0
   0x400000000194c481 <+1>:                 adds r14=16,r32
   0x400000000194c482 <+2>:                 mov r35=b0
   0x400000000194c490 <+16>:    [MMI]       mov r37=r1;;
   0x400000000194c491 <+17>:                ld8 r38=[r14]
   0x400000000194c492 <+18>:                nop.i 0x0;;
   0x400000000194c4a0 <+32>:    [MMI]       ld2 r14=[r38];;
   0x400000000194c4a1 <+33>:                cmp4.eq p6,p7=32,r14
   0x400000000194c4a2 <+34>:                nop.i 0x0;;
   0x400000000194c4b0 <+48>:    [MMI] (p07) addl r39=-833288,r1
   0x400000000194c4b1 <+49>:          (p07) mov r43=r0
   0x400000000194c4b2 <+50>:                nop.i 0x0
   0x400000000194c4c0 <+64>:    [MMI] (p07) mov r42=32
   0x400000000194c4c1 <+65>:          (p07) mov r40=1693
   0x400000000194c4c2 <+66>:          (p07) addl r41=-628512,r1;;
   0x400000000194c4d0 <+80>:    [MIB] (p07) ld8 r39=[r39]
   0x400000000194c4d1 <+81>:                nop.i 0x0
   0x400000000194c4d2 <+82>:          (p07) br.call.spnt.many b0=0x4000000001530200 <tree_check_failed(tree_node const*, char const*, int, char const*, ...)>;;
   0x400000000194c4e0 <+96>:    [MMI]       adds r14=152,r38;;
   0x400000000194c4e1 <+97>:                ld8 r14=[r14]
   0x400000000194c4e2 <+98>:                nop.i 0x0;;
   0x400000000194c4f0 <+112>:   [MIB]       cmp.eq p6,p7=0,r14
   0x400000000194c4f1 <+113>:               nop.i 0x0
   0x400000000194c4f2 <+114>:         (p06) br.cond.dpnt.few 0x400000000194c5f0 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+368>;;
   0x400000000194c500 <+128>:   [MMI]       nop.m 0x0
   0x400000000194c501 <+129>:               ld8 r14=[r32]
   0x400000000194c502 <+130>:               nop.i 0x0;;
   0x400000000194c510 <+144>:   [MIB]       nop.m 0x0
   0x400000000194c511 <+145>:               tbit.z p6,p7=r14,16
   0x400000000194c512 <+146>:         (p06) br.cond.dptk.few 0x400000000194c610 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+400>
   0x400000000194c520 <+160>:   [MMI]       adds r15=387,r32;;
   0x400000000194c521 <+161>:               ld1 r15=[r15]
   0x400000000194c522 <+162>:               nop.i 0x0;;
   0x400000000194c530 <+176>:   [MIB]       nop.m 0x0
   0x400000000194c531 <+177>:               cmp4.eq p7,p6=0,r15
   0x400000000194c532 <+178>:         (p06) br.cond.dptk.few 0x400000000194c550 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+208>;;
   0x400000000194c540 <+192>:   [MIB]       nop.m 0x0
   0x400000000194c541 <+193>:               tbit.z p7,p6=r14,17
   0x400000000194c542 <+194>:         (p06) br.cond.dptk.few 0x400000000194c5f0 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+368>
   0x400000000194c550 <+208>:   [MMI]       addl r14=-911832,r1;;
   0x400000000194c551 <+209>:               ld8 r14=[r14]
   0x400000000194c552 <+210>:               nop.i 0x0;;
   0x400000000194c560 <+224>:   [MMI]       adds r14=2059,r14;;
   0x400000000194c561 <+225>:               ld1 r14=[r14]
   0x400000000194c562 <+226>:               nop.i 0x0;;
   0x400000000194c570 <+240>:   [MII]       cmp4.eq p6,p7=1,r14
   0x400000000194c571 <+241>:               nop.i 0x0;;
   0x400000000194c572 <+242>:         (p07) addl r40=-833288,r1
   0x400000000194c580 <+256>:   [MMI] (p07) addl r42=-628512,r1
   0x400000000194c581 <+257>:         (p07) mov r41=1698
   0x400000000194c582 <+258>:         (p07) mov r39=11;;
   0x400000000194c590 <+272>:   [MIB] (p07) ld8 r40=[r40]
   0x400000000194c591 <+273>:               nop.i 0x0
   0x400000000194c592 <+274>:         (p07) br.call.spnt.many b0=0x4000000001532200 <tree_contains_struct_check_failed(tree_node const*, tree_node_structure_enum, char const*, int, char const*)>;;
   0x400000000194c5a0 <+288>:   [MMI]       adds r38=88,r38
   0x400000000194c5a1 <+289>:               nop.m 0x0
   0x400000000194c5a2 <+290>:               mov r39=4;;
   0x400000000194c5b0 <+304>:   [MMI]       ld8 r40=[r38]
   0x400000000194c5b1 <+305>:               nop.m 0x0
   0x400000000194c5b2 <+306>:               addl r38=-832208,r1;;
   0x400000000194c5c0 <+320>:   [MIB]       cmp.eq p7,p6=0,r40
   0x400000000194c5c1 <+321>:               nop.i 0x0
   0x400000000194c5c2 <+322>:         (p07) br.cond.dpnt.few 0x400000000194c650 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+464>;;
   0x400000000194c5d0 <+336>:   [MIB]       ld8 r38=[r38]
   0x400000000194c5d1 <+337>:               nop.i 0x0
   0x400000000194c5d2 <+338>:               br.call.sptk.many b0=0x4000000001534f40 <private_lookup_attribute_by_prefix(char const*, unsigned long, tree_node*)>;;
   0x400000000194c5e0 <+352>:   [MIB]       mov r1=r37
   0x400000000194c5e1 <+353>:               cmp.eq p6,p7=0,r8
   0x400000000194c5e2 <+354>:         (p06) br.cond.spnt.few 0x400000000194c650 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+464>
   0x400000000194c5f0 <+368>:   [MMI]       mov r8=r0
   0x400000000194c5f1 <+369>:               nop.m 0x0
   0x400000000194c5f2 <+370>:               mov.i ar.pfs=r36;;
   0x400000000194c600 <+384>:   [MIB]       nop.m 0x0
   0x400000000194c601 <+385>:               mov b0=r35
   0x400000000194c602 <+386>:               br.ret.sptk.many b0
   0x400000000194c610 <+400>:   [MMI]       adds r14=387,r32;;
   0x400000000194c611 <+401>:               ld1 r14=[r14]
   0x400000000194c612 <+402>:               nop.i 0x0;;
   0x400000000194c620 <+416>:   [MIB]       cmp4.eq p7,p6=0,r14
   0x400000000194c621 <+417>:               nop.i 0x0
   0x400000000194c622 <+418>:         (p06) br.cond.dpnt.few 0x400000000194c550 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+208>;;
   0x400000000194c630 <+432>:   [MMI]       mov r8=r0
   0x400000000194c631 <+433>:               nop.m 0x0
   0x400000000194c632 <+434>:               mov.i ar.pfs=r36;;
   0x400000000194c640 <+448>:   [MIB]       nop.m 0x0
   0x400000000194c641 <+449>:               mov b0=r35
   0x400000000194c642 <+450>:               br.ret.sptk.many b0;;
   0x400000000194c650 <+464>:   [MIB]       mov r38=216
   0x400000000194c651 <+465>:               nop.i 0x0
   0x400000000194c652 <+466>:               br.call.sptk.many b0=0x4000000001b83200 <operator new(unsigned long)>;;
   0x400000000194c660 <+480>:   [MMI]       mov r1=r37
   0x400000000194c661 <+481>:               mov r34=r8
   0x400000000194c662 <+482>:               mov r38=r8
   0x400000000194c670 <+496>:   [MMI]       mov r42=r33
   0x400000000194c671 <+497>:               mov r41=r0
   0x400000000194c672 <+498>:               mov r40=r32;;
   0x400000000194c680 <+512>:   [MIB]       mov r39=r0
   0x400000000194c681 <+513>:               nop.i 0x0
   0x400000000194c682 <+514>:               br.call.sptk.many b0=0x400000000194c2c0 <ipa_icf::sem_item::sem_item(ipa_icf::sem_item_type, symtab_node*, unsigned int, bitmap_obstack*)>;;
   0x400000000194c690 <+528>:   [MMI]       mov r1=r37
   0x400000000194c691 <+529>:               mov r8=r34
   0x400000000194c692 <+530>:               adds r17=208,r34
   0x400000000194c6a0 <+544>:   [MMI]       adds r16=152,r34
   0x400000000194c6a1 <+545>:               adds r15=168,r34
   0x400000000194c6a2 <+546>:               adds r14=192,r34;;
   0x400000000194c6b0 <+560>:   [MMI]       addl r18=-628624,r1
   0x400000000194c6b1 <+561>:               st8 [r17]=r0
   0x400000000194c6b2 <+562>:               nop.i 0x0
   0x400000000194c6c0 <+576>:   [MMI]       st8 [r16]=r0;;
   0x400000000194c6c1 <+577>:               ld8 r18=[r18]
   0x400000000194c6c2 <+578>:               nop.i 0x0
   0x400000000194c6d0 <+592>:   [MMI]       st8 [r15]=r0
   0x400000000194c6d1 <+593>:               st8 [r14]=r0
   0x400000000194c6d2 <+594>:               nop.i 0x0;;
   0x400000000194c6e0 <+608>:   [MMI]       nop.m 0x0
   0x400000000194c6e1 <+609>:               st8 [r8]=r18,200
   0x400000000194c6e2 <+610>:               nop.i 0x0;;
   0x400000000194c6f0 <+624>:   [MMI]       st8 [r8]=r0
   0x400000000194c6f1 <+625>:               nop.m 0x0
   0x400000000194c6f2 <+626>:               nop.i 0x0;;

Revision r223856:

Dump of assembler code for function ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*):
   0x400000000154fe00 <+0>:     [MMI]       alloc r36=ar.pfs,11,6,0
   0x400000000154fe01 <+1>:                 adds r14=16,r32
   0x400000000154fe02 <+2>:                 mov r35=b0
   0x400000000154fe10 <+16>:    [MMI]       mov r37=r1;;
   0x400000000154fe11 <+17>:                ld8 r14=[r14]
   0x400000000154fe12 <+18>:                nop.i 0x0;;
   0x400000000154fe20 <+32>:    [MMI]       adds r15=152,r14;;
   0x400000000154fe21 <+33>:                ld8 r15=[r15]
   0x400000000154fe22 <+34>:                nop.i 0x0;;
   0x400000000154fe30 <+48>:    [MIB]       cmp.eq p7,p6=0,r15
   0x400000000154fe31 <+49>:                nop.i 0x0
   0x400000000154fe32 <+50>:          (p07) br.cond.dpnt.few 0x400000000154fee0 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+224>;;
   0x400000000154fe40 <+64>:    [MMI]       nop.m 0x0
   0x400000000154fe41 <+65>:                ld8 r15=[r32]
   0x400000000154fe42 <+66>:                nop.i 0x0;;
   0x400000000154fe50 <+80>:    [MIB]       nop.m 0x0
   0x400000000154fe51 <+81>:                tbit.z p6,p7=r15,16
   0x400000000154fe52 <+82>:          (p06) br.cond.dptk.few 0x400000000154ff10 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+272>
   0x400000000154fe60 <+96>:    [MMI]       adds r16=387,r32;;
   0x400000000154fe61 <+97>:                ld1 r16=[r16]
   0x400000000154fe62 <+98>:                nop.i 0x0;;
   0x400000000154fe70 <+112>:   [MIB]       nop.m 0x0
   0x400000000154fe71 <+113>:               cmp4.eq p7,p6=0,r16
   0x400000000154fe72 <+114>:         (p06) br.cond.dptk.few 0x400000000154fe90 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+144>;;
   0x400000000154fe80 <+128>:   [MIB]       nop.m 0x0
   0x400000000154fe81 <+129>:               tbit.z p7,p6=r15,17
   0x400000000154fe82 <+130>:         (p06) br.cond.dptk.few 0x400000000154fee0 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+224>
   0x400000000154fe90 <+144>:   [MMI]       adds r14=88,r14
   0x400000000154fe91 <+145>:               addl r38=-855468,r1
   0x400000000154fe92 <+146>:               mov r39=4;;
   0x400000000154fea0 <+160>:   [MMI]       nop.m 0x0
   0x400000000154fea1 <+161>:               ld8 r40=[r14]
   0x400000000154fea2 <+162>:               nop.i 0x0;;
   0x400000000154feb0 <+176>:   [MIB]       cmp.eq p7,p6=0,r40
   0x400000000154feb1 <+177>:               nop.i 0x0
   0x400000000154feb2 <+178>:         (p07) br.cond.dpnt.few 0x400000000154ff40 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+320>;;
   0x400000000154fec0 <+192>:   [MIB]       ld8 r38=[r38]
   0x400000000154fec1 <+193>:               nop.i 0x0
   0x400000000154fec2 <+194>:               br.call.sptk.many b0=0x40000000011aa700 <private_lookup_attribute_by_prefix(char const*, unsigned long, tree_node*)>;;
   0x400000000154fed0 <+208>:   [MIB]       mov r1=r37
   0x400000000154fed1 <+209>:               cmp.eq p7,p6=0,r8
   0x400000000154fed2 <+210>:         (p07) br.cond.dpnt.few 0x400000000154ff40 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+320>
   0x400000000154fee0 <+224>:   [MMI]       mov r8=r0
   0x400000000154fee1 <+225>:               nop.m 0x0
   0x400000000154fee2 <+226>:               nop.i 0x0
   0x400000000154fef0 <+240>:   [MII]       nop.m 0x0
   0x400000000154fef1 <+241>:               mov.i ar.pfs=r36
   0x400000000154fef2 <+242>:               nop.i 0x0;;
   0x400000000154ff00 <+256>:   [MIB]       nop.m 0x0
   0x400000000154ff01 <+257>:               mov b0=r35
   0x400000000154ff02 <+258>:               br.ret.sptk.many b0
   0x400000000154ff10 <+272>:   [MMI]       adds r15=387,r32
   0x400000000154ff11 <+273>:               nop.m 0x0
   0x400000000154ff12 <+274>:               mov r8=r0;;
   0x400000000154ff20 <+288>:   [MMI]       nop.m 0x0
   0x400000000154ff21 <+289>:               ld1 r15=[r15]
   0x400000000154ff22 <+290>:               nop.i 0x0;;
   0x400000000154ff30 <+304>:   [MBB]       cmp4.eq p7,p6=0,r15
   0x400000000154ff31 <+305>:         (p06) br.cond.dpnt.few 0x400000000154fe90 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+144>
   0x400000000154ff32 <+306>:               br.few 0x400000000154fef0 <ipa_icf::sem_function::parse(cgraph_node*, bitmap_obstack*)+240>
   0x400000000154ff40 <+320>:   [MIB]       mov r38=216
   0x400000000154ff41 <+321>:               nop.i 0x0
   0x400000000154ff42 <+322>:               br.call.sptk.many b0=0x40000000017531c0 <operator new(unsigned long)>;;
   0x400000000154ff50 <+336>:   [MMI]       mov r1=r37
   0x400000000154ff51 <+337>:               mov r34=r8
   0x400000000154ff52 <+338>:               mov r38=r8
   0x400000000154ff60 <+352>:   [MMI]       mov r42=r33
   0x400000000154ff61 <+353>:               mov r41=r0
   0x400000000154ff62 <+354>:               mov r40=r32;;
   0x400000000154ff70 <+368>:   [MIB]       mov r39=r0
   0x400000000154ff71 <+369>:               nop.i 0x0
   0x400000000154ff72 <+370>:               br.call.sptk.many b0=0x400000000154fc40 <ipa_icf::sem_item::sem_item(ipa_icf::sem_item_type, symtab_node*, unsigned int, bitmap_obstack*)>;;
   0x400000000154ff80 <+384>:   [MMI]       mov r1=r37
   0x400000000154ff81 <+385>:               mov r14=r34
   0x400000000154ff82 <+386>:               adds r18=208,r34
   0x400000000154ff90 <+400>:   [MMI]       adds r17=152,r34
   0x400000000154ff91 <+401>:               adds r16=168,r34
   0x400000000154ff92 <+402>:               adds r15=192,r34;;
   0x400000000154ffa0 <+416>:   [MMI]       addl r19=-672196,r1
   0x400000000154ffa1 <+417>:               nop.m 0x0
   0x400000000154ffa2 <+418>:               mov r38=r34
   0x400000000154ffb0 <+432>:   [MMI]       st8 [r18]=r0
   0x400000000154ffb1 <+433>:               st8 [r17]=r0
   0x400000000154ffb2 <+434>:               nop.i 0x0;;
   0x400000000154ffc0 <+448>:   [MMI]       ld8 r19=[r19]
   0x400000000154ffc1 <+449>:               st8 [r16]=r0
   0x400000000154ffc2 <+450>:               nop.i 0x0
   0x400000000154ffd0 <+464>:   [MMI]       st8 [r15]=r0;;
   0x400000000154ffd1 <+465>:               st8 [r14]=r19,200
   0x400000000154ffd2 <+466>:               nop.i 0x0;;
   0x400000000154ffe0 <+480>:   [MIB]       st8 [r14]=r0
   0x400000000154ffe1 <+481>:               nop.i 0x0
   0x400000000154ffe2 <+482>:               br.call.sptk.many b0=0x4000000001546f00 <ipa_icf::sem_function::init()>;;
   0x400000000154fff0 <+496>:   [MMI]       mov r8=r34
   0x400000000154fff1 <+497>:               mov r1=r37
   0x400000000154fff2 <+498>:               mov.i ar.pfs=r36;;
   0x4000000001550000 <+512>:   [MIB]       nop.m 0x0
   0x4000000001550001 <+513>:               mov b0=r35
   0x4000000001550002 <+514>:               br.ret.sptk.many b0;;

Andreas.
Jan Hubicka May 31, 2015, 1:50 p.m. UTC | #12
> The problem is that ipa_icf::sem_function::parse is lacking the f->init
> call and the function epilog, falling through to the next function that
> happens to follow.

Thank you, that is indeed the devirtualization issue - I suppose the function
descriptors confuses the code even more.  I will finish the fix for that tonight.

Honza
diff mbox

Patch

Index: alias.c
===================================================================
--- alias.c	(revision 223633)
+++ alias.c	(working copy)
@@ -903,35 +906,79 @@  get_alias_set (tree t)
      the pointed-to types.  This issue has been reported to the
      C++ committee.
 
-     In addition to the above canonicalization issue, with LTO
-     we should also canonicalize `T (*)[]' to `T *' avoiding
-     alias issues with pointer-to element types and pointer-to
-     array types.
-
-     Likewise we need to deal with the situation of incomplete
-     pointed-to types and make `*(struct X **)&a' and
-     `*(struct X {} **)&a' alias.  Otherwise we will have to
-     guarantee that all pointer-to incomplete type variants
-     will be replaced by pointer-to complete type variants if
-     they are available.
-
-     With LTO the convenient situation of using `void *' to
-     access and store any pointer type will also become
-     more apparent (and `void *' is just another pointer-to
-     incomplete type).  Assigning alias-set zero to `void *'
-     and all pointer-to incomplete types is a not appealing
-     solution.  Assigning an effective alias-set zero only
-     affecting pointers might be - by recording proper subset
-     relationships of all pointer alias-sets.
-
-     Pointer-to function types are another grey area which
-     needs caution.  Globbing them all into one alias-set
-     or the above effective zero set would work.
-
-     For now just assign the same alias-set to all pointers.
-     That's simple and avoids all the above problems.  */
-  else if (POINTER_TYPE_P (t)
-	   && t != ptr_type_node)
+     For this reason go to canonical type of the unqalified pointer type.
+     Until GCC 6 this code set all pointers sets to have alias set of
+     ptr_type_node but that is a bad idea, because it prevents disabiguations
+     in between pointers.  For Firefox this accounts about 20% of all
+     disambiguations in the program.  */
+  else if (POINTER_TYPE_P (t) && t != ptr_type_node && !in_lto_p)
+    {
+      tree p;
+      auto_vec <bool, 8> reference;
+
+      /* Unnest all pointers and references.
+         We also want to make pointer to array equivalent to pointer to its
+         element. So skip all array types, too.  */
+      for (p = t; POINTER_TYPE_P (p)
+	   || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p));
+	   p = TREE_TYPE (p))
+	{
+	  if (TREE_CODE (p) == REFERENCE_TYPE)
+	    reference.safe_push (true);
+	  if (TREE_CODE (p) == POINTER_TYPE)
+	    reference.safe_push (false);
+	}
+      p = TYPE_MAIN_VARIANT (p);
+
+      /* Make void * compatible with char * and also void **.
+	 Programs are commonly violating TBAA by this.
+
+	 We also make void * to conflict with every pointer
+	 (see record_component_aliases) and thus it is safe it to use it for
+	 pointers to types with TYPE_STRUCTURAL_EQUALITY_P.  */
+      if (TREE_CODE (p) == VOID_TYPE || TYPE_STRUCTURAL_EQUALITY_P (p))
+	set = get_alias_set (ptr_type_node);
+      else
+	{
+	  /* Rebuild pointer type from starting from canonical types using
+	     unqualified pointers and references only.  This way all such
+	     pointers will have the same alias set and will conflict with
+	     each other.
+
+	     Most of time we already have pointers or references of a given type.
+	     If not build new one just to be sure that if someone later (probably
+	     only middle-end can, as we should assign all alias classes only after
+	     finishing translation unit) builds the pointer type, the canonical type
+	     will match.  */
+	  p = TYPE_CANONICAL (p);
+	  while (!reference.is_empty ())
+	    {
+	      if (reference.pop ())
+		p = build_reference_type (p);
+	      else
+		p = build_pointer_type (p);
+	      p = TYPE_CANONICAL (TYPE_MAIN_VARIANT (p));
+	    }
+          gcc_checking_assert (TYPE_CANONICAL (p) == p);
+
+	  /* Assign the alias set to both p and t.
+	     We can not call get_alias_set (p) here as that would triger
+	     infinite recursion when p == t.
+	     In other cases it would just trigger unnecesary legwork of
+	     rebuilding the pointer again.  */
+	  if (TYPE_ALIAS_SET_KNOWN_P (p))
+	    /* Return early; we do not need to record component aliases.  */
+	    return TYPE_ALIAS_SET (t) = TYPE_ALIAS_SET (p);
+	  else
+	    {
+	      set = new_alias_set ();
+	      TYPE_ALIAS_SET (p) = set;
+	    }
+	}
+    }
+  /* In LTO the rules above needs to be part of canonical type machinery.
+     For now just punt.  */
+  else if (POINTER_TYPE_P (t) && t != ptr_type_node && in_lto_p)
     set = get_alias_set (ptr_type_node);
 
   /* Otherwise make a new alias set for this type.  */
@@ -950,7 +997,8 @@  get_alias_set (tree t)
 
   /* If this is an aggregate type or a complex type, we must record any
      component aliasing information.  */
-  if (AGGREGATE_TYPE_P (t) || TREE_CODE (t) == COMPLEX_TYPE)
+  if (AGGREGATE_TYPE_P (t) || TREE_CODE (t) == COMPLEX_TYPE
+      || TREE_CODE (t) == POINTER_TYPE)
     record_component_aliases (t);
 
   return set;
@@ -1050,6 +1098,26 @@  record_component_aliases (tree type)
 
   switch (TREE_CODE (type))
     {
+    /* We want void * to be compatible with any other pointer without really
+       dropping it to alias set 0. Doing so would make it compatible with
+       all non-pointer types too.
+
+       Make thus ptr_type_node to be a component of every pointer type.
+       Tus memory operations of type "void *" conflict with operations of type
+       "T *" without impossing a conflict with memory operations of type "Q *"
+       in case T and Q do not conflict.
+ 
+       This is not strictly necessary by the language standards, but avoids
+       common type punning mistakes.  In addition to that, we need the existence
+       of such universal pointer to implement Fortran's C_PTR type (which is
+       defined as type compatible with all C pointers) and we use it in
+       get_alias_set to give alias set to pointers to types without
+       canonical types (those are typically nameless incomplete types)
+       that needs to be also compatible with each other and with pointers to
+       complete types.  */
+    case POINTER_TYPE:
+      record_alias_subset (superset, get_alias_set (ptr_type_node));
+      break;
     case RECORD_TYPE:
     case UNION_TYPE:
     case QUAL_UNION_TYPE: