| Message ID | alpine.DEB.2.02.1310250707430.14734@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On 10/24/13 23:23, Marc Glisse wrote: > Hello, > > I noticed that in some cases we were failing to find aliasing > information because we were only looking at an SSA_NAME variable, > missing the fact that it was really an ADDR_EXPR. The attached patch > passes bootstrap+testsuite, does it make sense? (I am a bit afraid of > losing some type information for instance) > > I didn't investigate the 2 tests where I had to remove dg-bogus, because > removing dg-bogus sounds like a bonus... > > 2013-10-25 Marc Glisse <marc.glisse@inria.fr> > > gcc/ > * tree-ssa-alias.c (ao_ref_init_from_ptr_and_size): Look for an > ADDR_EXPR in the defining statement. Shouldn't the ADDR_EXPR have been propagated into the use? Jeff
On Thu, 24 Oct 2013, Jeff Law wrote: > On 10/24/13 23:23, Marc Glisse wrote: >> Hello, >> >> I noticed that in some cases we were failing to find aliasing >> information because we were only looking at an SSA_NAME variable, >> missing the fact that it was really an ADDR_EXPR. The attached patch >> passes bootstrap+testsuite, does it make sense? (I am a bit afraid of >> losing some type information for instance) >> >> I didn't investigate the 2 tests where I had to remove dg-bogus, because >> removing dg-bogus sounds like a bonus... >> >> 2013-10-25 Marc Glisse <marc.glisse@inria.fr> >> >> gcc/ >> * tree-ssa-alias.c (ao_ref_init_from_ptr_and_size): Look for an >> ADDR_EXPR in the defining statement. > Shouldn't the ADDR_EXPR have been propagated into the use? Maybe when the address is a constant, but here it comes from malloc.
On Fri, Oct 25, 2013 at 8:11 AM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Thu, 24 Oct 2013, Jeff Law wrote: > >> On 10/24/13 23:23, Marc Glisse wrote: >>> >>> Hello, >>> >>> I noticed that in some cases we were failing to find aliasing >>> information because we were only looking at an SSA_NAME variable, >>> missing the fact that it was really an ADDR_EXPR. The attached patch >>> passes bootstrap+testsuite, does it make sense? (I am a bit afraid of >>> losing some type information for instance) >>> >>> I didn't investigate the 2 tests where I had to remove dg-bogus, because >>> removing dg-bogus sounds like a bonus... >>> >>> 2013-10-25 Marc Glisse <marc.glisse@inria.fr> >>> >>> gcc/ >>> * tree-ssa-alias.c (ao_ref_init_from_ptr_and_size): Look for an >>> ADDR_EXPR in the defining statement. >> >> Shouldn't the ADDR_EXPR have been propagated into the use? > > > Maybe when the address is a constant, but here it comes from malloc. points-to should have "propagated" the alias info, so no, looking at def-stmts in random places like this isn't ok. Where does alias info get lost? Thanks, Richard. > -- > Marc Glisse
On Fri, Oct 25, 2013 at 10:59 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, Oct 25, 2013 at 8:11 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >> On Thu, 24 Oct 2013, Jeff Law wrote: >> >>> On 10/24/13 23:23, Marc Glisse wrote: >>>> >>>> Hello, >>>> >>>> I noticed that in some cases we were failing to find aliasing >>>> information because we were only looking at an SSA_NAME variable, >>>> missing the fact that it was really an ADDR_EXPR. The attached patch >>>> passes bootstrap+testsuite, does it make sense? (I am a bit afraid of >>>> losing some type information for instance) >>>> >>>> I didn't investigate the 2 tests where I had to remove dg-bogus, because >>>> removing dg-bogus sounds like a bonus... >>>> >>>> 2013-10-25 Marc Glisse <marc.glisse@inria.fr> >>>> >>>> gcc/ >>>> * tree-ssa-alias.c (ao_ref_init_from_ptr_and_size): Look for an >>>> ADDR_EXPR in the defining statement. >>> >>> Shouldn't the ADDR_EXPR have been propagated into the use? >> >> >> Maybe when the address is a constant, but here it comes from malloc. > > points-to should have "propagated" the alias info, so no, looking at > def-stmts in random places like this isn't ok. Where does alias info > get lost? It doesn't get lost, but this is the missed optimization that malloc results still alias with global pointers (here a function argument). Taking into account the offset shouldn't change anything. I suppose you are looking for the memcpy to be folded into an assignment? Which ao_ref_from_ptr_and_size is critical for the transform - it doesn't seem to be the one in memory op folding. Richard. > Thanks, > Richard. > >> -- >> Marc Glisse
On Fri, 25 Oct 2013, Richard Biener wrote: > On Fri, Oct 25, 2013 at 10:59 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Fri, Oct 25, 2013 at 8:11 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >>> On Thu, 24 Oct 2013, Jeff Law wrote: >>> >>>> On 10/24/13 23:23, Marc Glisse wrote: >>>>> >>>>> Hello, >>>>> >>>>> I noticed that in some cases we were failing to find aliasing >>>>> information because we were only looking at an SSA_NAME variable, >>>>> missing the fact that it was really an ADDR_EXPR. The attached patch >>>>> passes bootstrap+testsuite, does it make sense? (I am a bit afraid of >>>>> losing some type information for instance) >>>>> >>>>> I didn't investigate the 2 tests where I had to remove dg-bogus, because >>>>> removing dg-bogus sounds like a bonus... >>>>> >>>>> 2013-10-25 Marc Glisse <marc.glisse@inria.fr> >>>>> >>>>> gcc/ >>>>> * tree-ssa-alias.c (ao_ref_init_from_ptr_and_size): Look for an >>>>> ADDR_EXPR in the defining statement. >>>> >>>> Shouldn't the ADDR_EXPR have been propagated into the use? >>> >>> >>> Maybe when the address is a constant, but here it comes from malloc. >> >> points-to should have "propagated" the alias info, so no, looking at >> def-stmts in random places like this isn't ok. Where does alias info >> get lost? By "points-to", do you mean in pt_solution? That's very rough information, and in particular here it can't tell me that 2 fields of the same struct don't alias, can it? > It doesn't get lost, but this is the missed optimization that malloc > results still alias with global pointers (here a function argument). > Taking into account the offset shouldn't change anything. I think this is a different issue (but if you say improving malloc would help, maybe). > I suppose you are looking for the memcpy to be folded into an > assignment? Which ao_ref_from_ptr_and_size is critical for > the transform - it doesn't seem to be the one in memory op folding. No, I only used memcpy as an example, my goal is for the compiler to notice that the call (memcpy here, possibly other functions with some new attribute later) doesn't clobber the counter (i) and thus the counter value is the same after the call as it was before. If memcpy gets turned to an assignment (which alignment should prevent), my testcase becomes useless.
On Fri, Oct 25, 2013 at 11:29 AM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Fri, 25 Oct 2013, Richard Biener wrote: > >> On Fri, Oct 25, 2013 at 10:59 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> >>> On Fri, Oct 25, 2013 at 8:11 AM, Marc Glisse <marc.glisse@inria.fr> >>> wrote: >>>> >>>> On Thu, 24 Oct 2013, Jeff Law wrote: >>>> >>>>> On 10/24/13 23:23, Marc Glisse wrote: >>>>>> >>>>>> >>>>>> Hello, >>>>>> >>>>>> I noticed that in some cases we were failing to find aliasing >>>>>> information because we were only looking at an SSA_NAME variable, >>>>>> missing the fact that it was really an ADDR_EXPR. The attached patch >>>>>> passes bootstrap+testsuite, does it make sense? (I am a bit afraid of >>>>>> losing some type information for instance) >>>>>> >>>>>> I didn't investigate the 2 tests where I had to remove dg-bogus, >>>>>> because >>>>>> removing dg-bogus sounds like a bonus... >>>>>> >>>>>> 2013-10-25 Marc Glisse <marc.glisse@inria.fr> >>>>>> >>>>>> gcc/ >>>>>> * tree-ssa-alias.c (ao_ref_init_from_ptr_and_size): Look for an >>>>>> ADDR_EXPR in the defining statement. >>>>> >>>>> >>>>> Shouldn't the ADDR_EXPR have been propagated into the use? >>>> >>>> >>>> >>>> Maybe when the address is a constant, but here it comes from malloc. >>> >>> >>> points-to should have "propagated" the alias info, so no, looking at >>> def-stmts in random places like this isn't ok. Where does alias info >>> get lost? > > > By "points-to", do you mean in pt_solution? That's very rough information, > and in particular here it can't tell me that 2 fields of the same struct > don't alias, can it? No, it cannot. >> It doesn't get lost, but this is the missed optimization that malloc >> results still alias with global pointers (here a function argument). >> Taking into account the offset shouldn't change anything. > > > I think this is a different issue (but if you say improving malloc would > help, maybe). > > >> I suppose you are looking for the memcpy to be folded into an >> assignment? Which ao_ref_from_ptr_and_size is critical for >> the transform - it doesn't seem to be the one in memory op folding. > > > No, I only used memcpy as an example, my goal is for the compiler to notice > that the call (memcpy here, possibly other functions with some new attribute > later) doesn't clobber the counter (i) and thus the counter value is the > same after the call as it was before. Ah, so you are looking at call_may_clobber_ref_p_1 and its pointer handling when special-casing builtins? Note that fields can only be disambiguated if the size of the access is known (not sure what fancy attribute you are going to invent here ...). Generally the simple alias machinery is written to be cheap, walking use-def chains isn't. Users do not have to expect use-def chains to be walked (we can change that rule of course, but for example the RTL alias machinery also shares the core of the gimple alias machinery. But I can see that in this particular case the patch makes sense. Still, + if (TREE_CODE (ptr) == SSA_NAME) + { + gimple stmt = SSA_NAME_DEF_STMT (ptr); + if (gimple_assign_single_p (stmt) + && !gimple_has_volatile_ops (stmt) + && gimple_assign_rhs_code (stmt) == ADDR_EXPR) + ptr = gimple_assign_rhs1 (stmt); + } no need to look at gimple_has_volatile_ops (stmt). Also you want to handle p_2 = p_1 + CST; foo (p_2); which has a related canonical form, p_2 = &MEM[p_1, CST]; The patch is ok with the volatile check removed, you can followup with handling POINTER_PLUS_EXPR if you like. Thanks, Richard. > If memcpy gets turned to an assignment (which alignment should prevent), my > testcase becomes useless. > > -- > Marc Glisse
On Fri, 25 Oct 2013, Richard Biener wrote: > Ah, so you are looking at call_may_clobber_ref_p_1 and its pointer handling > when special-casing builtins? Yes. > Note that fields can only be disambiguated > if the size of the access is known TBAA could also help sometimes. > (not sure what fancy attribute you are going to invent here ...). That will certainly require quite a bit of discussion... > Generally the simple alias machinery is written to be cheap, I wouldn't mind an expensive version ;-) > walking use-def chains isn't. Peeking at the defining statement shouldn't be very costly, as long as you don't do it recursively. > no need to look at gimple_has_volatile_ops (stmt). Ok. > Also you want to handle > > p_2 = p_1 + CST; > foo (p_2); > > which has a related canonical form, > > p_2 = &MEM[p_1, CST]; This testcase seems relevant, I'll see if I can handle it: void f (const char *c, int *i) { *i = 42; __builtin_memcpy (i + 1, c, sizeof (int)); if (*i != 42) __builtin_abort(); } > The patch is ok with the volatile check removed, you can followup > with handling POINTER_PLUS_EXPR if you like. Thanks.
Marc Glisse wrote: > I noticed that in some cases we were failing to find aliasing > information because we were only looking at an SSA_NAME variable, > missing the fact that it was really an ADDR_EXPR. The attached patch > passes bootstrap+testsuite, does it make sense? (I am a bit afraid of > losing some type information for instance) > > I didn't investigate the 2 tests where I had to remove dg-bogus, because > removing dg-bogus sounds like a bonus... I wonder why you are seeing failures with those test cases. I tired your patch (w/o modifying the test cases) and they passed. The idea of those test cases is to ensure that there is no output like "note: loop versioned for vectorization because of possible aliasing" Because that output would be a hint that "#pragma GCC ivdep" doesn't work. What kind of output do you see? Seemingly not the one above as you kept the "version" dg-bogus. Tobias
On Fri, 25 Oct 2013, Tobias Burnus wrote: > Marc Glisse wrote: >> I noticed that in some cases we were failing to find aliasing >> information because we were only looking at an SSA_NAME variable, >> missing the fact that it was really an ADDR_EXPR. The attached patch >> passes bootstrap+testsuite, does it make sense? (I am a bit afraid of >> losing some type information for instance) >> >> I didn't investigate the 2 tests where I had to remove dg-bogus, because >> removing dg-bogus sounds like a bonus... > > I wonder why you are seeing failures with those test cases. I tired your > patch (w/o modifying the test cases) and they passed. > > The idea of those test cases is to ensure that there is no output like > "note: loop versioned for vectorization because of possible aliasing" > Because that output would be a hint that "#pragma GCC ivdep" doesn't work. > > What kind of output do you see? Seemingly not the one above as you kept the > "version" dg-bogus. <beats self on the head> My source directory includes the word "alias" in it. So I'll leave those 2 dg-bogus alone, but please tighten the regexp a bit, include at least a space or something...
On 10/25/13 03:29, Marc Glisse wrote: > >> It doesn't get lost, but this is the missed optimization that malloc >> results still alias with global pointers (here a function argument). >> Taking into account the offset shouldn't change anything. > > I think this is a different issue (but if you say improving malloc would > help, maybe). Has the patent on Steensgaard's techniques expired yet? IIRC it would handle this kind of stuff quite well. Jeff
Index: gcc/testsuite/gcc.dg/tree-ssa/alias-23.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/alias-23.c (revision 0) +++ gcc/testsuite/gcc.dg/tree-ssa/alias-23.c (working copy) @@ -0,0 +1,18 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-optimized" } */ + +typedef struct A { int i; double d; } A; + +void f1 (const char *c) +{ + A *s = (A*) __builtin_malloc (sizeof (A)); + double *p = &s->d; + s->i = 42; + __builtin_memcpy (p, c, sizeof (double)); + int j = s->i; + if (j != 42) __builtin_abort(); +} + +/* { dg-final { scan-tree-dump-not "abort" "optimized" } } */ +/* { dg-final { cleanup-tree-dump "optimized" } } */ + Property changes on: gcc/testsuite/gcc.dg/tree-ssa/alias-23.c ___________________________________________________________________ Added: svn:eol-style ## -0,0 +1 ## +native \ No newline at end of property Added: svn:keywords ## -0,0 +1 ## +Author Date Id Revision URL \ No newline at end of property Index: gcc/testsuite/gcc.dg/vect/vect-ivdep-1.c =================================================================== --- gcc/testsuite/gcc.dg/vect/vect-ivdep-1.c (revision 204044) +++ gcc/testsuite/gcc.dg/vect/vect-ivdep-1.c (working copy) @@ -8,12 +8,11 @@ void foo(int n, int *a, int *b, int *c, int *d, int *e) { int i, j; #pragma GCC ivdep for (i = 0; i < n; ++i) { a[i] = b[i] + c[i]; } } /* { dg-message "loop vectorized" "" { target *-*-* } 0 } */ /* { dg-bogus "version" "" { target *-*-* } 0 } */ -/* { dg-bogus "alias" "" { target *-*-* } 0 } */ /* { dg-final { cleanup-tree-dump "vect" } } */ Index: gcc/testsuite/gfortran.dg/vect/vect-do-concurrent-1.f90 =================================================================== --- gcc/testsuite/gfortran.dg/vect/vect-do-concurrent-1.f90 (revision 204044) +++ gcc/testsuite/gfortran.dg/vect/vect-do-concurrent-1.f90 (working copy) @@ -6,12 +6,11 @@ subroutine test(n, a, b, c) integer, value :: n real, contiguous, pointer :: a(:), b(:), c(:) integer :: i do concurrent (i = 1:n) a(i) = b(i) + c(i) end do end subroutine test ! { dg-message "loop vectorized" "" { target *-*-* } 0 } ! { dg-bogus "version" "" { target *-*-* } 0 } -! { dg-bogus "alias" "" { target *-*-* } 0 } ! { dg-final { cleanup-tree-dump "vect" } } Index: gcc/tree-ssa-alias.c =================================================================== --- gcc/tree-ssa-alias.c (revision 204044) +++ gcc/tree-ssa-alias.c (working copy) @@ -561,20 +561,29 @@ ao_ref_alias_set (ao_ref *ref) /* Init an alias-oracle reference representation from a gimple pointer PTR and a gimple size SIZE in bytes. If SIZE is NULL_TREE the the size is assumed to be unknown. The access is assumed to be only to or after of the pointer target, not before it. */ void ao_ref_init_from_ptr_and_size (ao_ref *ref, tree ptr, tree size) { HOST_WIDE_INT t1, t2; ref->ref = NULL_TREE; + if (TREE_CODE (ptr) == SSA_NAME) + { + gimple stmt = SSA_NAME_DEF_STMT (ptr); + if (gimple_assign_single_p (stmt) + && !gimple_has_volatile_ops (stmt) + && gimple_assign_rhs_code (stmt) == ADDR_EXPR) + ptr = gimple_assign_rhs1 (stmt); + } + if (TREE_CODE (ptr) == ADDR_EXPR) ref->base = get_ref_base_and_extent (TREE_OPERAND (ptr, 0), &ref->offset, &t1, &t2); else { ref->base = build2 (MEM_REF, char_type_node, ptr, null_pointer_node); ref->offset = 0; } if (size