| Message ID | 20151202080716.GV5527@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
On Wed, 2 Dec 2015, Jan Hubicka wrote: > Hi, > this patch makes the type system to be unchanged by flag_strict_aliasing. > This is needed to prevent optimization loss in flag_strict_aliasing code where > some !flag_strict_aliasing code put alias set 0 into a type (this happens > in all cases I modified in my original patch). It is also necessary to validate > ipa-icf and operand_equal_p transformations to be safe for code transitions > !flag_strict_aliasing->flag_strict_aliasing that I wasn to do in the inliner. > > This patch goes the opposite way than my previous attempt (and is short unlike > the explanation ;). Instead of adding extra parameter to get_alias_set it > makes get_alias_set do ignore flag_strict_aliasing. To make sure that no TBAA > is used when !flag_strict_aliasing I can simply disable alias_set_subset_of and > alias_sets_conflict_p which are the only way TBAA oracle can disambiguate > items. > > Next there are cases where optimizations are disabled to keep TBAA right. > I audited the code and found only function.c (that uses object_must_conflict > for packing) and ipa-icf/fold-const. This patch updates objects_must_conflict_p, fold-const > already check flag_strict_aliasing and I did not update ipa-icf because I would > have to disable non-strict-aliasing path in the followup patch. > > I checked that there is no code difference with -fno-strict-aliasing -fno-ipa-icf > with this patch on tramp3d and dealII > > > Bootstrapped/regtested x86_64-linux and also lto-bootstraped. Looks OK? > > * alias.c (alias_set_subset_of, alias_sets_conflict_p, > objects_must_conflict_p): Short circuit for !flag_strict_aliasing > (get_alias_set): Remove flag_strict_aliasing check. > (new_alias_set): Likewise. > Index: alias.c > =================================================================== > --- alias.c (revision 231081) > +++ alias.c (working copy) > @@ -405,6 +405,10 @@ alias_set_subset_of (alias_set_type set1 > { > alias_set_entry *ase2; > > + /* Disable TBAA oracle with !flag_strict_aliasing. */ > + if (!flag_strict_aliasing) > + return true; > + > /* Everything is a subset of the "aliases everything" set. */ > if (set2 == 0) > return true; > @@ -466,6 +470,10 @@ alias_sets_conflict_p (alias_set_type se > alias_set_entry *ase1; > alias_set_entry *ase2; > > + /* Disable TBAA oracle with !flag_strict_aliasing. */ > + if (!flag_strict_aliasing) > + return true; > + > /* The easy case. */ > if (alias_sets_must_conflict_p (set1, set2)) > return 1; > @@ -561,6 +569,9 @@ objects_must_conflict_p (tree t1, tree t > { > alias_set_type set1, set2; > > + if (!flag_strict_aliasing) > + return 1; > + Rather than adjusting this function please adjust alias_sets_must_conflict_p. Otherwise this looks ok and indeed much nicer. Thanks, Richard. > /* If neither has a type specified, we don't know if they'll conflict > because we may be using them to store objects of various types, for > example the argument and local variables areas of inlined functions. */ > @@ -816,10 +827,12 @@ get_alias_set (tree t) > { > alias_set_type set; > > - /* If we're not doing any alias analysis, just assume everything > - aliases everything else. Also return 0 if this or its type is > - an error. */ > - if (! flag_strict_aliasing || t == error_mark_node > + /* We can not give up with -fno-strict-aliasing because we need to build > + proper type representation for possible functions which are build with > + -fstirct-aliasing. */ > + > + /* return 0 if this or its type is an error. */ > + if (t == error_mark_node > || (! TYPE_P (t) > && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) > return 0; > @@ -1085,15 +1098,10 @@ get_alias_set (tree t) > alias_set_type > new_alias_set (void) > { > - if (flag_strict_aliasing) > - { > - if (alias_sets == 0) > - vec_safe_push (alias_sets, (alias_set_entry *) NULL); > - vec_safe_push (alias_sets, (alias_set_entry *) NULL); > - return alias_sets->length () - 1; > - } > - else > - return 0; > + if (alias_sets == 0) > + vec_safe_push (alias_sets, (alias_set_entry *) NULL); > + vec_safe_push (alias_sets, (alias_set_entry *) NULL); > + return alias_sets->length () - 1; > } > > /* Indicate that things in SUBSET can alias things in SUPERSET, but that
> > Rather than adjusting this function please adjust > alias_sets_must_conflict_p. > > Otherwise this looks ok and indeed much nicer. OK, will update it. Will hold this patch until we resolve what we want to do with the debug dumps. I do not seem to be able to reproduce any -fcompare-debug issues with that, so perhaps we already strip the alias sets and the whoe -fstrict-alias-set fiddling is unnecesary. Honza
> Bootstrapped/regtested x86_64-linux and also lto-bootstraped. Looks OK? > > * alias.c (alias_set_subset_of, alias_sets_conflict_p, > objects_must_conflict_p): Short circuit for !flag_strict_aliasing > (get_alias_set): Remove flag_strict_aliasing check. > (new_alias_set): Likewise. Not clear whether it's this patch specifically or another one in the series, but the compiler now hangs on simple Ada code it used to compile instantly. A couple of testcases is attached. It looks like the compiler is now stuck in get_alias_set endlessly pushing references onto a vector.
> > Bootstrapped/regtested x86_64-linux and also lto-bootstraped. Looks OK? > > > > * alias.c (alias_set_subset_of, alias_sets_conflict_p, > > objects_must_conflict_p): Short circuit for !flag_strict_aliasing > > (get_alias_set): Remove flag_strict_aliasing check. > > (new_alias_set): Likewise. > > Not clear whether it's this patch specifically or another one in the series, > but the compiler now hangs on simple Ada code it used to compile instantly. > > A couple of testcases is attached. It looks like the compiler is now stuck in > get_alias_set endlessly pushing references onto a vector. uhm, sorry. I will take a look. Honza > > -- > Eric Botcazou > package Access1 is > > type R; > type S is access R; > type R is new S; > > end Access1; > package Access2 is > > type Priv; > type Inc is access Priv; > type Priv is access Inc; > C : constant Priv := new Inc; > > end Access2;
> > > Bootstrapped/regtested x86_64-linux and also lto-bootstraped. Looks OK? > > > > > > * alias.c (alias_set_subset_of, alias_sets_conflict_p, > > > objects_must_conflict_p): Short circuit for !flag_strict_aliasing > > > (get_alias_set): Remove flag_strict_aliasing check. > > > (new_alias_set): Likewise. > > > > Not clear whether it's this patch specifically or another one in the series, > > but the compiler now hangs on simple Ada code it used to compile instantly. > > > > A couple of testcases is attached. It looks like the compiler is now stuck in > > get_alias_set endlessly pushing references onto a vector. > uhm, sorry. I will take a look. The problem is with the type: (gdb) p debug_tree (p) <pointer_type 0x7ffff6af02a0 type <pointer_type 0x7ffff6af02a0> sizes-gimplified public visited unsigned DI size <integer_cst 0x7ffff6ad7bb8 type <integer_type 0x7ffff6adb2a0 bitsizetype> constant visited 64> unit size <integer_cst 0x7ffff6ad7bd0 type <integer_type 0x7ffff6adb1f8 sizetype> constant visited 8> align 64 symtab 0 alias set -1 canonical type 0x7ffff6af02a0 pointer_to_this <pointer_type 0x7ffff6af02a0>> it is a recursive pointer to itself. Does this make sense in Ada? If so we will need to add a recursion guard into the loop and put the alias set into voidptr_alias_set. It more looks like a frontend bug to me - I can not think of a use for this beast. Honza
> The problem is with the type: > (gdb) p debug_tree (p) > <pointer_type 0x7ffff6af02a0 type <pointer_type 0x7ffff6af02a0> > sizes-gimplified public visited unsigned DI > size <integer_cst 0x7ffff6ad7bb8 type <integer_type 0x7ffff6adb2a0 > bitsizetype> constant visited 64> unit size <integer_cst 0x7ffff6ad7bd0 > type <integer_type 0x7ffff6adb1f8 sizetype> constant visited 8> align 64 > symtab 0 alias set -1 canonical type 0x7ffff6af02a0 > pointer_to_this <pointer_type 0x7ffff6af02a0>> > > it is a recursive pointer to itself. Does this make sense in Ada? If so we > will need to add a recursion guard into the loop and put the alias set into > voidptr_alias_set. It more looks like a frontend bug to me - I can not > think of a use for this beast. This one (access1) is admittedly border line and we can probably kludge around it in the front-end, but what about access2 and more generally pointer cycles?
On Tue, 8 Dec 2015, Eric Botcazou wrote: > > The problem is with the type: > > (gdb) p debug_tree (p) > > <pointer_type 0x7ffff6af02a0 type <pointer_type 0x7ffff6af02a0> > > sizes-gimplified public visited unsigned DI > > size <integer_cst 0x7ffff6ad7bb8 type <integer_type 0x7ffff6adb2a0 > > bitsizetype> constant visited 64> unit size <integer_cst 0x7ffff6ad7bd0 > > type <integer_type 0x7ffff6adb1f8 sizetype> constant visited 8> align 64 > > symtab 0 alias set -1 canonical type 0x7ffff6af02a0 > > pointer_to_this <pointer_type 0x7ffff6af02a0>> > > > > it is a recursive pointer to itself. Does this make sense in Ada? If so we > > will need to add a recursion guard into the loop and put the alias set into > > voidptr_alias_set. It more looks like a frontend bug to me - I can not > > think of a use for this beast. > > This one (access1) is admittedly border line and we can probably kludge around > it in the front-end, but what about access2 and more generally pointer cycles? Usually cycles happen through structure members and it might be that all other frontends have the pointed-to type incomplete. But the above recursion shouldn't apply for the structure case. Not sure how your other examples look like.
> Usually cycles happen through structure members and it might be that > all other frontends have the pointed-to type incomplete. But the > above recursion shouldn't apply for the structure case. All types are equal in Ada and can be forward declared; the language specifies that their "elaboration" can be delayed until a "freeze" point (in particular, you cannot declare an object of the type until after it), from which all the incomplete references must be resolved to the final type. > Not sure how your other examples look like. Pure pointer cycles of any length.
> > Usually cycles happen through structure members and it might be that > > all other frontends have the pointed-to type incomplete. But the > > above recursion shouldn't apply for the structure case. > > All types are equal in Ada and can be forward declared; the language specifies > that their "elaboration" can be delayed until a "freeze" point (in particular, > you cannot declare an object of the type until after it), from which all the > incomplete references must be resolved to the final type. > > > Not sure how your other examples look like. > > Pure pointer cycles of any length. OK, the code already pre-allocates the vector to be 8 elements. What about simply punting when reaching this depth? I do not think real world program have more than 8 nested pointers often enough for this to matter. They will then get same alias set as void *. Honza > > -- > Eric Botcazou
> OK, the code already pre-allocates the vector to be 8 elements. What about > simply punting when reaching this depth? I do not think real world program > have more than 8 nested pointers often enough for this to matter. > They will then get same alias set as void *. Fine with me, but the len<8 cases need to be dealt with as well.
Index: alias.c =================================================================== --- alias.c (revision 231081) +++ alias.c (working copy) @@ -405,6 +405,10 @@ alias_set_subset_of (alias_set_type set1 { alias_set_entry *ase2; + /* Disable TBAA oracle with !flag_strict_aliasing. */ + if (!flag_strict_aliasing) + return true; + /* Everything is a subset of the "aliases everything" set. */ if (set2 == 0) return true; @@ -466,6 +470,10 @@ alias_sets_conflict_p (alias_set_type se alias_set_entry *ase1; alias_set_entry *ase2; + /* Disable TBAA oracle with !flag_strict_aliasing. */ + if (!flag_strict_aliasing) + return true; + /* The easy case. */ if (alias_sets_must_conflict_p (set1, set2)) return 1; @@ -561,6 +569,9 @@ objects_must_conflict_p (tree t1, tree t { alias_set_type set1, set2; + if (!flag_strict_aliasing) + return 1; + /* If neither has a type specified, we don't know if they'll conflict because we may be using them to store objects of various types, for example the argument and local variables areas of inlined functions. */ @@ -816,10 +827,12 @@ get_alias_set (tree t) { alias_set_type set; - /* If we're not doing any alias analysis, just assume everything - aliases everything else. Also return 0 if this or its type is - an error. */ - if (! flag_strict_aliasing || t == error_mark_node + /* We can not give up with -fno-strict-aliasing because we need to build + proper type representation for possible functions which are build with + -fstirct-aliasing. */ + + /* return 0 if this or its type is an error. */ + if (t == error_mark_node || (! TYPE_P (t) && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node))) return 0; @@ -1085,15 +1098,10 @@ get_alias_set (tree t) alias_set_type new_alias_set (void) { - if (flag_strict_aliasing) - { - if (alias_sets == 0) - vec_safe_push (alias_sets, (alias_set_entry *) NULL); - vec_safe_push (alias_sets, (alias_set_entry *) NULL); - return alias_sets->length () - 1; - } - else - return 0; + if (alias_sets == 0) + vec_safe_push (alias_sets, (alias_set_entry *) NULL); + vec_safe_push (alias_sets, (alias_set_entry *) NULL); + return alias_sets->length () - 1; } /* Indicate that things in SUBSET can alias things in SUPERSET, but that