Message ID | 20190613120550.7wtzpieeszazjn7i@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
Series | indirect_ref_may_alias_decl_p fix | expand |
On Thu, 13 Jun 2019, Jan Hubicka wrote: > Hi, > after spending some time on the view converting MEM_REFs, I noticed that > most of reasons we give up on view converts is not actually MEM_REF created > but test > same_type_for_tbaa (TREE_TYPE (dbase2), TREE_TYPE (base2)) > in indirect_ref_may_alias_decl_p > > Here base2 is VAR_DECL while dbase2 is MEM_REF used to access it. > > In the testcase: > struct a {int a1; int a2;}; > struct b:a {}; > > struct b bvar,*bptr2; > int > test(void) > { > struct a *bptr = &bvar; > bptr->a2=0; > bptr2->a1=1; > return bptr->a2; > } > > We have variable of type b, while we access it via its basetype. > This mean that TREE_TYPE (base) is "struct b" while TREE_TYPE (dbase) > is "struct a" which is perfectly normal and we could to the access path > tests at least in the same strength as we would do if bptr=$bvar was > not visible to compiler (in that case we optimize things correctly). > > Of course later in aliasing_component_refs_p we should not assume that > "struct a" is the type of memory slot since the access path may contain > b, but I think we can not assume that about "struct b" either, see below. > > We should not give up on this case and just proceed the same way as > indirect_refs_may_alias_p does. In fact I would like to commonize the > access path oracle of these functions incremetally but first I want to > drop main differences. In particular > > 1) indirect_refs_may_alias_decl_p passing ref2_is_decl as true to > aliasing_component_refs_p. > > This makes aliasing_component_refs_p to assume that all access paths > conflicting with REF2 must start by type of BASE2 or its subtype. > > IMO this is not quite right in gimple memory model where decls are just > untyped memory slots, since I can, for example, I can rewrite decl > of type a by a new data of type struct b {struct a a;}; > which will confuse this logic. The above check you complain about guards against this. > I will try to get rid of this incrementally - I would like to have it > logged how much optimization we lose here. > > 2) indirect_refs_may_alias_decl_p does > > if ((TREE_CODE (base1) != TARGET_MEM_REF > || (!TMR_INDEX (base1) && !TMR_INDEX2 (base1))) > && same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (dbase2)) == 1 > && (TREE_CODE (TREE_TYPE (base1)) != ARRAY_TYPE > || (TYPE_SIZE (TREE_TYPE (base1)) > && TREE_CODE (TYPE_SIZE (TREE_TYPE (base1))) == INTEGER_CST))) > return ranges_maybe_overlap_p (doffset1, max_size1, doffset2, max_size2); > > while indirect_refs_may_alias_p does: > > if ((TREE_CODE (base1) != TARGET_MEM_REF > || (!TMR_INDEX (base1) && !TMR_INDEX2 (base1))) > && (TREE_CODE (base2) != TARGET_MEM_REF > || (!TMR_INDEX (base2) && !TMR_INDEX2 (base2))) > && same_type_for_tbaa (TREE_TYPE (ptrtype1), > TREE_TYPE (ptrtype2)) == 1 > /* But avoid treating arrays as "objects", instead assume they > can overlap by an exact multiple of their element size. > See gcc.dg/torture/alias-2.c. */ > && TREE_CODE (TREE_TYPE (ptrtype1)) != ARRAY_TYPE) > return ranges_maybe_overlap_p (offset1, max_size1, offset2, max_size2); > > Sincce we already checked that TREEE_TYPE (ptrtype) is same as TREE_TYPE (base1) > the same_type_for_tbaa check is equivalent in both. > > Notice however that first tests that array is VLA, while other > supports partial overlaps on all array types. I suppose we want to > choose which way we support that and go with one or another. Let's go with the stricter check for the purpose of unification and work on this issue as followup. > Of course even in that case overlap check is not completely lost, > I attached testcase for that to > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90869 > All we need is to wrap the checks by array size. > > With these differences sorted out I think both functions may dispatch to > common access path oracle after doing the case specific work. > > Bootstrapped/regtested x86_64-linux, makes sense? Yes, the patch below makes sense. Thanks, Richard. > PR tree-optimize/90869 > * tree-ssa-alias.c (indirect_ref_may_alias_decl_p): Watch for view > converts in MEM_REF referencing decl rather than view converts > from decl type to MEM_REF type. > > * g++.dg/tree-ssa/alias-access-path-1.C: New testcase. > Index: tree-ssa-alias.c > =================================================================== > --- tree-ssa-alias.c (revision 272037) > +++ tree-ssa-alias.c (working copy) > @@ -1370,11 +1410,16 @@ indirect_ref_may_alias_decl_p (tree ref1 > poly_offset_int doffset2 = offset2; > if (TREE_CODE (dbase2) == MEM_REF > || TREE_CODE (dbase2) == TARGET_MEM_REF) > - doffset2 -= mem_ref_offset (dbase2) << LOG2_BITS_PER_UNIT; > + { > + doffset2 -= mem_ref_offset (dbase2) << LOG2_BITS_PER_UNIT; > + tree ptrtype2 = TREE_TYPE (TREE_OPERAND (dbase2, 1)); > + /* If second reference is view-converted, give up now. */ > + if (same_type_for_tbaa (TREE_TYPE (dbase2), TREE_TYPE (ptrtype2)) != 1) > + return true; > + } > > - /* If either reference is view-converted, give up now. */ > - if (same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) != 1 > - || same_type_for_tbaa (TREE_TYPE (dbase2), TREE_TYPE (base2)) != 1) > + /* If first reference is view-converted, give up now. */ > + if (same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) != 1) > return true; > > /* If both references are through the same type, they do not alias > @@ -1408,7 +1457,13 @@ indirect_ref_may_alias_decl_p (tree ref1 > offset1, max_size1, > ref2, > ref2_alias_set, base2_alias_set, > - offset2, max_size2, true); > + offset2, max_size2, > + /* Only if the other reference is actual > + decl we can safely check only toplevel > + part of access path 1. */ > + same_type_for_tbaa (TREE_TYPE (dbase2), > + TREE_TYPE (base2)) > + == 1); > > return true; > } > Index: testsuite/g++.dg/tree-ssa/alias-access-path-1.C > =================================================================== > --- testsuite/g++.dg/tree-ssa/alias-access-path-1.C (nonexistent) > +++ testsuite/g++.dg/tree-ssa/alias-access-path-1.C (working copy) > @@ -0,0 +1,24 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O3 -fdump-tree-fre1" } */ > + > +struct a {int a1; int a2;}; > +struct b:a {}; > + > +struct b bvar,*bptr2; > +int > +test(void) > +{ > + struct a *bptr = &bvar; > + bptr->a2=0; > + bptr2->a1=1; > + return bptr->a2; > +} > +int > +test2(void) > +{ > + struct b *bptr = &bvar; > + bptr->a2=0; > + bptr2->a1=1; > + return bptr->a2; > +} > +/* { dg-final { scan-tree-dump-times "return 0" 2 "fre1" } } */ >
> > 1) indirect_refs_may_alias_decl_p passing ref2_is_decl as true to > > aliasing_component_refs_p. > > > > This makes aliasing_component_refs_p to assume that all access paths > > conflicting with REF2 must start by type of BASE2 or its subtype. > > > > IMO this is not quite right in gimple memory model where decls are just > > untyped memory slots, since I can, for example, I can rewrite decl > > of type a by a new data of type struct b {struct a a;}; > > which will confuse this logic. > > The above check you complain about guards against this. Not sufficinly. ref1 can be store of type "struct b". With ref2 of base type "struct a" (which is in conflict) the oracle will currently disambiguate because it will not consider the path "struct b"..."struct a" due to that ref2_is_decl check. > > Sincce we already checked that TREEE_TYPE (ptrtype) is same as TREE_TYPE (base1) > > the same_type_for_tbaa check is equivalent in both. > > > > Notice however that first tests that array is VLA, while other > > supports partial overlaps on all array types. I suppose we want to > > choose which way we support that and go with one or another. > > Let's go with the stricter check for the purpose of unification and work > on this issue as followup. Yep, that is my plan. W/o that this that alias-2.c array overlap testcase would break (and it breaks if one of accesses is through decl). Concerning the unification, I am still confinced we want to test base2_alias_set to be non-zero in indirect_ref_may_alias_decl_p (same way as we do in indirect_refs_may_alias_p) as discussed in thread https://gcc.gnu.org/ml/gcc-patches/2019-06/msg00087.html Because we can not assume that there are no dynamic type changes on decls in gimple memory model. I also need to dig into the divergence between nonoverlapping_component_refs_of_decl_p and nonoverlapping_component_refs_p. It seems to me that having the decl in hand does not buy us much here either. Notice that one tries to match structures only, while other handles unions too. For LTO as well for code merging even w/o LTO it would be nice to keep alias between union {int a; int b;}; and ptr->a an ptr->b (which we currently do), but it seems it is guaranteed only by the second function. Honza
On Thu, 13 Jun 2019 at 14:29, Jan Hubicka <hubicka@ucw.cz> wrote: > > > > 1) indirect_refs_may_alias_decl_p passing ref2_is_decl as true to > > > aliasing_component_refs_p. > > > > > > This makes aliasing_component_refs_p to assume that all access paths > > > conflicting with REF2 must start by type of BASE2 or its subtype. > > > > > > IMO this is not quite right in gimple memory model where decls are just > > > untyped memory slots, since I can, for example, I can rewrite decl > > > of type a by a new data of type struct b {struct a a;}; > > > which will confuse this logic. > > > > The above check you complain about guards against this. > > Not sufficinly. ref1 can be store of type "struct b". With ref2 of > base type "struct a" (which is in conflict) the oracle will currently > disambiguate because it will not consider the path > "struct b"..."struct a" due to that ref2_is_decl check. > > > > Sincce we already checked that TREEE_TYPE (ptrtype) is same as TREE_TYPE (base1) > > > the same_type_for_tbaa check is equivalent in both. > > > > > > Notice however that first tests that array is VLA, while other > > > supports partial overlaps on all array types. I suppose we want to > > > choose which way we support that and go with one or another. > > > > Let's go with the stricter check for the purpose of unification and work > > on this issue as followup. > > Yep, that is my plan. W/o that this that alias-2.c array overlap > testcase would break (and it breaks if one of accesses is through decl). > > Concerning the unification, I am still confinced we want to test > base2_alias_set to be non-zero in indirect_ref_may_alias_decl_p > (same way as we do in indirect_refs_may_alias_p) as discussed in thread > https://gcc.gnu.org/ml/gcc-patches/2019-06/msg00087.html > Because we can not assume that there are no dynamic type changes on > decls in gimple memory model. > > I also need to dig into the divergence between > nonoverlapping_component_refs_of_decl_p and > nonoverlapping_component_refs_p. It seems to me that having the decl in > hand does not buy us much here either. > > Notice that one tries to match structures only, while other handles > unions too. For LTO as well for code merging even w/o LTO it would be > nice to keep alias between > union {int a; int b;}; > and ptr->a an ptr->b (which we currently do), but it seems it is > guaranteed only by the second function. > Hi! Since this was committed (r272247), I've noticed a failure to build glibc-2.29 for aarch64: #'target_mem_ref' not supported by expression#'pmap_rmt.c: In function 'clnt_broadcast': pmap_rmt.c:298:19: error: may be used uninitialized in this function [-Werror=maybe-uninitialized] 298 | baddr.sin_addr = addrs[i]; | ~~~~~~~~~~~~~~~^~~~~~~~~~ while compiling sunrpc/pmap_rmt.os Christophe > Honza
> Hi! > > Since this was committed (r272247), I've noticed a failure to build > glibc-2.29 for aarch64: > #'target_mem_ref' not supported by expression#'pmap_rmt.c: In function > 'clnt_broadcast': > pmap_rmt.c:298:19: error: may be used uninitialized in this function > [-Werror=maybe-uninitialized] > 298 | baddr.sin_addr = addrs[i]; > | ~~~~~~~~~~~~~~~^~~~~~~~~~ > > while compiling sunrpc/pmap_rmt.os It is hard to tell w/o preprocessed source, but looking at https://github.com/lattera/glibc/blob/master/sunrpc/pmap_rmt.c addrs is initialized by getbroadcastnests which has early exit when getifaddrs fails leaving adds uninitialized. So this may be just an usual false positive of -Wmaybe-uninitialized caused by better optimization :) Honza > > Christophe > > > > > Honza
Hi Christophe, > Since this was committed (r272247), I've noticed a failure to build > glibc-2.29 for aarch64: > #'target_mem_ref' not supported by expression#'pmap_rmt.c: In function > 'clnt_broadcast': > pmap_rmt.c:298:19: error: may be used uninitialized in this function > [-Werror=maybe-uninitialized] > 298 | baddr.sin_addr = addrs[i]; > | ~~~~~~~~~~~~~~~^~~~~~~~~~ > > while compiling sunrpc/pmap_rmt.os indeed: I just came to the same conclusion via a reghunt. Even worse, it breaks i386-pc-solaris2.11, sparc*-sun-solaris2.11, and i686-pc-solaris2.11 bootstrap. This is PR bootstrap/90873. Rainer
Hi, actually since Rainer's testcase has TARGET_MEM_REF I think the problem may be due to fact that we now can get TARGET_MEM_REF ofsetting the array housed in decl while previously we gave up on types mismatch. Does the following help? Index: tree-ssa-alias.c =================================================================== --- tree-ssa-alias.c (revision 272265) +++ tree-ssa-alias.c (working copy) @@ -1393,8 +1393,10 @@ But avoid treating variable length arrays as "objects", instead assume they can overlap by an exact multiple of their element size. See gcc.dg/torture/alias-2.c. */ - if ((TREE_CODE (base1) != TARGET_MEM_REF - || (!TMR_INDEX (base1) && !TMR_INDEX2 (base1))) + if (((TREE_CODE (base1) != TARGET_MEM_REF + || (!TMR_INDEX (base1) && !TMR_INDEX2 (base1))) + && (TREE_CODE (dbase2) != TARGET_MEM_REF + || (!TMR_INDEX (dbase2) && !TMR_INDEX2 (dbase2)))) && same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (dbase2)) == 1 && (TREE_CODE (TREE_TYPE (base1)) != ARRAY_TYPE || (TYPE_SIZE (TREE_TYPE (base1))
Hi, I am testing the following patch which solves the bogus warning in tree-ssa-forwprop.c and -m32 and plan to commit it as obvoius to unbreak bootstrap once testing converges. Previously we did not get here wince we got mismatch between TMR type and decl type but same code is present in indirect_ref_may_alias_p. Honza PR bootstrap/90873 * tree-ssa-alias.c (indirect_ref_may_alias_decl_p): Also check that dbase is not TARGET_MEM_REF. Index: tree-ssa-alias.c =================================================================== --- tree-ssa-alias.c (revision 272247) +++ tree-ssa-alias.c (working copy) @@ -1393,8 +1393,10 @@ indirect_ref_may_alias_decl_p (tree ref1 But avoid treating variable length arrays as "objects", instead assume they can overlap by an exact multiple of their element size. See gcc.dg/torture/alias-2.c. */ - if ((TREE_CODE (base1) != TARGET_MEM_REF + if (((TREE_CODE (base1) != TARGET_MEM_REF || (!TMR_INDEX (base1) && !TMR_INDEX2 (base1))) + && (TREE_CODE (dbase2) != TARGET_MEM_REF + || (!TMR_INDEX (dbase2) && !TMR_INDEX2 (base2)))) && same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (dbase2)) == 1 && (TREE_CODE (TREE_TYPE (base1)) != ARRAY_TYPE || (TYPE_SIZE (TREE_TYPE (base1))
Hi Jan, > I am testing the following patch which solves the bogus warning in > tree-ssa-forwprop.c and -m32 and plan to commit it as obvoius to unbreak > bootstrap once testing converges. Previously we did not get here wince > we got mismatch between TMR type and decl type but same code is present > in indirect_ref_may_alias_p. > > Honza > > PR bootstrap/90873 > * tree-ssa-alias.c (indirect_ref_may_alias_decl_p): Also check that > dbase is not TARGET_MEM_REF. I've included the patch in nightly bootstraps on i386-pc-solaris2.11, sparc-sun-solaris2.11, and i686-pc-linux-gnu. All completed without regressions, thanks. One last issue, though. The error messages totally feel like line noise to me: /vol/gcc/src/hg/trunk/local/gcc/tree-ssa-forwprop.c: In function 'bool simplify_rotate(gimple_stmt_iterator*)': /vol/gcc/src/hg/trunk/local/gcc/tree-ssa-forwprop.c:1650:40: error: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Werror=maybe-uninitialized] 1650 | if (cdef_arg2[i] == def_arg2[1 - i] | ~~~~~~~~~~~~~~^ Is this really something we mean to expose to users? Rainer
> Hi Jan, > > > I am testing the following patch which solves the bogus warning in > > tree-ssa-forwprop.c and -m32 and plan to commit it as obvoius to unbreak > > bootstrap once testing converges. Previously we did not get here wince > > we got mismatch between TMR type and decl type but same code is present > > in indirect_ref_may_alias_p. > > > > Honza > > > > PR bootstrap/90873 > > * tree-ssa-alias.c (indirect_ref_may_alias_decl_p): Also check that > > dbase is not TARGET_MEM_REF. > > I've included the patch in nightly bootstraps on i386-pc-solaris2.11, > sparc-sun-solaris2.11, and i686-pc-linux-gnu. All completed without > regressions, thanks. > > One last issue, though. The error messages totally feel like line noise > to me: > > /vol/gcc/src/hg/trunk/local/gcc/tree-ssa-forwprop.c: In function 'bool simplify_rotate(gimple_stmt_iterator*)': > /vol/gcc/src/hg/trunk/local/gcc/tree-ssa-forwprop.c:1650:40: error: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Werror=maybe-uninitialized] > 1650 | if (cdef_arg2[i] == def_arg2[1 - i] > | ~~~~~~~~~~~~~~^ > > Is this really something we mean to expose to users? No, I think we should have PR on this. I am not sure why late uninitialized warnings runs after ivopts which is the place we introduce TMR. Honza > > Rainer > > -- > ----------------------------------------------------------------------------- > Rainer Orth, Center for Biotechnology, Bielefeld University
Index: tree-ssa-alias.c =================================================================== --- tree-ssa-alias.c (revision 272037) +++ tree-ssa-alias.c (working copy) @@ -1370,11 +1410,16 @@ indirect_ref_may_alias_decl_p (tree ref1 poly_offset_int doffset2 = offset2; if (TREE_CODE (dbase2) == MEM_REF || TREE_CODE (dbase2) == TARGET_MEM_REF) - doffset2 -= mem_ref_offset (dbase2) << LOG2_BITS_PER_UNIT; + { + doffset2 -= mem_ref_offset (dbase2) << LOG2_BITS_PER_UNIT; + tree ptrtype2 = TREE_TYPE (TREE_OPERAND (dbase2, 1)); + /* If second reference is view-converted, give up now. */ + if (same_type_for_tbaa (TREE_TYPE (dbase2), TREE_TYPE (ptrtype2)) != 1) + return true; + } - /* If either reference is view-converted, give up now. */ - if (same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) != 1 - || same_type_for_tbaa (TREE_TYPE (dbase2), TREE_TYPE (base2)) != 1) + /* If first reference is view-converted, give up now. */ + if (same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) != 1) return true; /* If both references are through the same type, they do not alias @@ -1408,7 +1457,13 @@ indirect_ref_may_alias_decl_p (tree ref1 offset1, max_size1, ref2, ref2_alias_set, base2_alias_set, - offset2, max_size2, true); + offset2, max_size2, + /* Only if the other reference is actual + decl we can safely check only toplevel + part of access path 1. */ + same_type_for_tbaa (TREE_TYPE (dbase2), + TREE_TYPE (base2)) + == 1); return true; } Index: testsuite/g++.dg/tree-ssa/alias-access-path-1.C =================================================================== --- testsuite/g++.dg/tree-ssa/alias-access-path-1.C (nonexistent) +++ testsuite/g++.dg/tree-ssa/alias-access-path-1.C (working copy) @@ -0,0 +1,24 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -fdump-tree-fre1" } */ + +struct a {int a1; int a2;}; +struct b:a {}; + +struct b bvar,*bptr2; +int +test(void) +{ + struct a *bptr = &bvar; + bptr->a2=0; + bptr2->a1=1; + return bptr->a2; +} +int +test2(void) +{ + struct b *bptr = &bvar; + bptr->a2=0; + bptr2->a1=1; + return bptr->a2; +} +/* { dg-final { scan-tree-dump-times "return 0" 2 "fre1" } } */