On Thu, 28 May 2015, Jan Hubicka wrote: > Hi, > here is updated version of patch. It makes alias_set_subset_of to be > symmetric for > ptr_type_node and other pointer type and moves the logic of creating subsets > to get_alias_set. > > I tested that perlbmk works when built at -O3 x86_64 > > Bootstrapped/regtested x86_64-linux, OK? > > Honza > > * alias.c (alias_set_entry_d): Add is_pointer. > (alias_set_subset_of): Special case pointers. > (init_alias_set_entry): Break out from ... > (record_alias_subset): ... here. > (get_alias_set): Do less generous pointer globbing. > * gcc.dg/alias-8.c: Do not xfail. > * gcc.dg/pr62167.c: Prevent FRE. > Index: alias.c > =================================================================== > --- alias.c (revision 223772) > +++ alias.c (working copy) > @@ -183,10 +184,6 @@ struct GTY(()) alias_set_entry_d { > /* The alias set number, as stored in MEM_ALIAS_SET. */ > alias_set_type alias_set; > > - /* Nonzero if would have a child of zero: this effectively makes this > - alias set the same as alias set zero. */ > - int has_zero_child; > - > /* The children of the alias set. These are not just the immediate > children, but, in fact, all descendants. So, if we have: > > @@ -195,6 +192,15 @@ struct GTY(()) alias_set_entry_d { > continuing our example above, the children here will be all of > `int', `double', `float', and `struct S'. */ > hash_map<int, int, alias_set_traits> *children; > + > + /* Nonzero if would have a child of zero: this effectively makes this > + alias set the same as alias set zero. */ > + bool has_zero_child; > + /* Nonzero if alias set corresponds to pointer type itself (i.e. not to > + aggregate contaiing pointer. > + This is used for a special case where we need an universal pointer type > + compatible with all other pointer types. */ > + bool is_pointer; > }; > typedef struct alias_set_entry_d *alias_set_entry; > > @@ -460,12 +466,33 @@ alias_set_subset_of (alias_set_type set1 > if (set2 == 0) > return true; > > - /* Otherwise, check if set1 is a subset of set2. */ > + /* Check if set1 is a subset of set2. */ > ase = get_alias_set_entry (set2); > if (ase != 0 > && (ase->has_zero_child > || ase->children->get (set1))) > return true; > + > + /* As a special case we consider alias set of "void *" to be both subset > + and superset of every alias set of a pointer. This extra symmetry does > + not matter for alias_sets_conflict_p but it makes > aliasing_component_refs_p > + to return true on the following testcase: > + > + void *ptr; > + char **ptr2=(char **)&ptr; > + *ptr2 = ... > + > + This makes void * truly universal pointer type. See pointer handling in > + get_alias_set for more details. */ > + if (ase && ase->is_pointer) > + { > + alias_set_entry ase1 = get_alias_set_entry (set1); > + > + if (ase1 && ase1->is_pointer > + && (set1 == TYPE_ALIAS_SET (ptr_type_node) > + || set2 == TYPE_ALIAS_SET (ptr_type_node))) > + return true; > + } > return false; > } > > @@ -764,6 +791,21 @@ alias_ptr_types_compatible_p (tree t1, t > == TYPE_MAIN_VARIANT (TREE_TYPE (t2))); > } > > +/* Create emptry alias set entry. */ > + > +alias_set_entry > +init_alias_set_entry (alias_set_type set) > +{ > + alias_set_entry ase = ggc_cleared_alloc<alias_set_entry_d> ();
no need to use cleared_alloc if you also init ->is_pointer to false. > + ase->alias_set = set; > + ase->children > + = hash_map<int, int, alias_set_traits>::create_ggc (64); that seems a bit excessive, esp. for pointers which won't end up with any children? So better make children lazily allocated in record_alias_subset. > + ase->has_zero_child = 0; > + gcc_checking_assert (!get_alias_set_entry (set)); > + (*alias_sets)[set] = ase; > + return ase; > +} > + > /* Return the alias set for T, which may be either a type or an > expression. Call language-specific routine for help, if needed. */ > > @@ -903,35 +945,92 @@ 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 we 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 trigger > + 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)) > + set = TYPE_ALIAS_SET (p); > + else > + { > + set = new_alias_set (); > + TYPE_ALIAS_SET (p) = set; > + /* 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. > + > + We make ptr_type_node to be a component of every pointer > + type. Thus 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 C/C++ 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). */ > + record_alias_subset (set, get_alias_set (ptr_type_node)); I still wonder why you do this instead of changing alias_sets_conflict in the same way you changed alias_set_subset_of. Patch looks ok otherwise but please leave the patch for others to comment on for a while. Thanks, Richard. > + } > + } > + } > + /* 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. */ > @@ -953,6 +1052,15 @@ get_alias_set (tree t) > if (AGGREGATE_TYPE_P (t) || TREE_CODE (t) == COMPLEX_TYPE) > record_component_aliases (t); > > + /* We treat pointer types specially in alias_set_subset_of. */ > + if (POINTER_TYPE_P (t) && set) > + { > + alias_set_entry ase = get_alias_set_entry (set); > + if (!ase) > + ase = init_alias_set_entry (set); > + ase->is_pointer = true; > + } > + > return set; > } > > @@ -1003,12 +1111,7 @@ record_alias_subset (alias_set_type supe > { > /* Create an entry for the SUPERSET, so that we have a place to > attach the SUBSET. */ > - superset_entry = ggc_cleared_alloc<alias_set_entry_d> (); > - superset_entry->alias_set = superset; > - superset_entry->children > - = hash_map<int, int, alias_set_traits>::create_ggc (64); > - superset_entry->has_zero_child = 0; > - (*alias_sets)[superset] = superset_entry; > + superset_entry = init_alias_set_entry (superset); > } > > if (subset == 0) > Index: testsuite/gcc.dg/alias-8.c > =================================================================== > --- testsuite/gcc.dg/alias-8.c (revision 223772) > +++ testsuite/gcc.dg/alias-8.c (working copy) > @@ -8,5 +8,5 @@ struct s { > void > func(struct s *ptr) > { > - *(void **)&ptr->p = 0; /* { dg-warning "type-punned pointer" "" { xfail > *-*-* } } */ > + *(void **)&ptr->p = 0; /* { dg-warning "type-punned pointer" "" { } } */ > } > Index: testsuite/gcc.dg/pr62167.c > =================================================================== > --- testsuite/gcc.dg/pr62167.c (revision 223772) > +++ testsuite/gcc.dg/pr62167.c (working copy) > @@ -29,6 +29,8 @@ main () > > node.prev = (void *)head; > > + asm("":"=m"(node.prev)); > + > head->first = &node; > > struct node *n = head->first; > > -- Richard Biener <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)