| Message ID | alpine.DEB.2.02.1311061308080.20046@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 6, 2013 at 1:19 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > [Discussion started in > http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02472.html ] > > > On Wed, 30 Oct 2013, Marc Glisse wrote: > >> On Wed, 30 Oct 2013, Richard Biener wrote: >> >>> Btw, get_addr_base_and_unit_offset may also return an offsetted >>> MEM_REF (from &MEM [p_3, 17] for example). As we are interested in >>> pointers this could be handled by not requiring a memory reference >>> but extracting the base address and offset, covering more cases. >> >> >> I tried the attached patch, and it almost worked, except for one fortran >> testcase (widechar_intrinsics_10.f90): > > > Now that ao_ref_init_from_ptr_and_size has been fixed, the following patch > passes bootstrap+testsuite (default languages) on x86_64-unknown-linux-gnu. > > 2013-11-06 Marc Glisse <marc.glisse@inria.fr> > Jeff Law <law@redhat.com> > > gcc/ > * tree-ssa-alias.c (stmt_kills_ref_p_1): Use > ao_ref_init_from_ptr_and_size for builtins. > > gcc/testsuite/ > * gcc.dg/tree-ssa/alias-27.c: New testcase. > > -- > Marc Glisse > Index: gcc/testsuite/gcc.dg/tree-ssa/alias-27.c > =================================================================== > --- gcc/testsuite/gcc.dg/tree-ssa/alias-27.c (revision 0) > +++ gcc/testsuite/gcc.dg/tree-ssa/alias-27.c (working copy) > @@ -0,0 +1,11 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O1 -fdump-tree-optimized" } */ > + > +void f (long *p) { > + *p = 42; > + p[4] = 42; > + __builtin_memset (p, 0, 100); > +} > + > +/* { dg-final { scan-tree-dump-not "= 42" "optimized" } } */ > +/* { dg-final { cleanup-tree-dump "optimized" } } */ > > Property changes on: gcc/testsuite/gcc.dg/tree-ssa/alias-27.c > ___________________________________________________________________ > Added: svn:keywords > ## -0,0 +1 ## > +Author Date Id Revision URL > \ No newline at end of property > Added: svn:eol-style > ## -0,0 +1 ## > +native > \ No newline at end of property > Index: gcc/tree-ssa-alias.c > =================================================================== > --- gcc/tree-ssa-alias.c (revision 204453) > +++ gcc/tree-ssa-alias.c (working copy) > @@ -2001,23 +2001,24 @@ stmt_may_clobber_ref_p (gimple stmt, tre > return stmt_may_clobber_ref_p_1 (stmt, &r); > } > > /* If STMT kills the memory reference REF return true, otherwise > return false. */ > > static bool > stmt_kills_ref_p_1 (gimple stmt, ao_ref *ref) > { > /* For a must-alias check we need to be able to constrain > - the access properly. */ > - ao_ref_base (ref); > - if (ref->max_size == -1) > + the access properly. > + FIXME: except for BUILTIN_FREE. */ > + if (!ao_ref_base (ref) > + || ref->max_size == -1) > return false; > > if (gimple_has_lhs (stmt) > && TREE_CODE (gimple_get_lhs (stmt)) != SSA_NAME > /* The assignment is not necessarily carried out if it can throw > and we can catch it in the current function where we could inspect > the previous value. > ??? We only need to care about the RHS throwing. For aggregate > assignments or similar calls and non-call exceptions the LHS > might throw as well. */ > @@ -2090,37 +2091,47 @@ stmt_kills_ref_p_1 (gimple stmt, ao_ref > case BUILT_IN_MEMPCPY: > case BUILT_IN_MEMMOVE: > case BUILT_IN_MEMSET: > case BUILT_IN_MEMCPY_CHK: > case BUILT_IN_MEMPCPY_CHK: > case BUILT_IN_MEMMOVE_CHK: > case BUILT_IN_MEMSET_CHK: > { > tree dest = gimple_call_arg (stmt, 0); > tree len = gimple_call_arg (stmt, 2); > - tree base = NULL_TREE; > - HOST_WIDE_INT offset = 0; > + tree rbase = ref->base; > + HOST_WIDE_INT roffset = ref->offset; > if (!host_integerp (len, 0)) > return false; > - if (TREE_CODE (dest) == ADDR_EXPR) > - base = get_addr_base_and_unit_offset (TREE_OPERAND (dest, > 0), > - &offset); > - else if (TREE_CODE (dest) == SSA_NAME) > - base = dest; > - if (base > - && base == ao_ref_base (ref)) > + ao_ref dref; > + ao_ref_init_from_ptr_and_size (&dref, dest, len); What I dislike about this is that it can end up building a new tree node. Not sure if that should block the patch though as it clearly allows to simplify the code ... > + tree base = ao_ref_base (&dref); > + HOST_WIDE_INT offset = dref.offset; > + if (!base || dref.size == -1) > + return false; > + if (TREE_CODE (base) == MEM_REF) > + { > + if (TREE_CODE (rbase) != MEM_REF) why's that? I think that just processing both bases separately would work as well. > + return false; > + // Compare pointers. > + offset += BITS_PER_UNIT > + * TREE_INT_CST_LOW (TREE_OPERAND (base, 1)); Use mem_ref_offset (base). > + roffset += BITS_PER_UNIT > + * TREE_INT_CST_LOW (TREE_OPERAND (rbase, 1)); Likewise. > + base = TREE_OPERAND (base, 0); > + rbase = TREE_OPERAND (rbase, 0); Both could be &decl here, so you want if (TREE_CODE (base) == ADDR_EXPR) base = TREE_OPERAND (base, 0); Thanks, Richard. > + } > + if (base == rbase) > { > - HOST_WIDE_INT size = TREE_INT_CST_LOW (len); > - if (offset <= ref->offset / BITS_PER_UNIT > - && (offset + size > - >= ((ref->offset + ref->max_size + BITS_PER_UNIT - > 1) > - / BITS_PER_UNIT))) > + HOST_WIDE_INT size = BITS_PER_UNIT * TREE_INT_CST_LOW > (len); > + if (offset <= roffset > + && offset + size >= (roffset + ref->max_size)) > return true; > } > break; > } > > case BUILT_IN_VA_END: > { > tree ptr = gimple_call_arg (stmt, 0); > if (TREE_CODE (ptr) == ADDR_EXPR) > { >
On Wed, 6 Nov 2013, Richard Biener wrote: > On Wed, Nov 6, 2013 at 1:19 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >> [Discussion started in >> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02472.html ] >> >> >> On Wed, 30 Oct 2013, Marc Glisse wrote: >> >>> On Wed, 30 Oct 2013, Richard Biener wrote: >>> >>>> Btw, get_addr_base_and_unit_offset may also return an offsetted >>>> MEM_REF (from &MEM [p_3, 17] for example). As we are interested in >>>> pointers this could be handled by not requiring a memory reference >>>> but extracting the base address and offset, covering more cases. >>> >>> >>> I tried the attached patch, and it almost worked, except for one fortran >>> testcase (widechar_intrinsics_10.f90): >> >> >> Now that ao_ref_init_from_ptr_and_size has been fixed, the following patch >> passes bootstrap+testsuite (default languages) on x86_64-unknown-linux-gnu. >> >> 2013-11-06 Marc Glisse <marc.glisse@inria.fr> >> Jeff Law <law@redhat.com> >> >> gcc/ >> * tree-ssa-alias.c (stmt_kills_ref_p_1): Use >> ao_ref_init_from_ptr_and_size for builtins. >> >> gcc/testsuite/ >> * gcc.dg/tree-ssa/alias-27.c: New testcase. >> >> -- >> Marc Glisse >> Index: gcc/testsuite/gcc.dg/tree-ssa/alias-27.c >> =================================================================== >> --- gcc/testsuite/gcc.dg/tree-ssa/alias-27.c (revision 0) >> +++ gcc/testsuite/gcc.dg/tree-ssa/alias-27.c (working copy) >> @@ -0,0 +1,11 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-O1 -fdump-tree-optimized" } */ >> + >> +void f (long *p) { >> + *p = 42; >> + p[4] = 42; >> + __builtin_memset (p, 0, 100); >> +} >> + >> +/* { dg-final { scan-tree-dump-not "= 42" "optimized" } } */ >> +/* { dg-final { cleanup-tree-dump "optimized" } } */ >> >> Property changes on: gcc/testsuite/gcc.dg/tree-ssa/alias-27.c >> ___________________________________________________________________ >> Added: svn:keywords >> ## -0,0 +1 ## >> +Author Date Id Revision URL >> \ No newline at end of property >> Added: svn:eol-style >> ## -0,0 +1 ## >> +native >> \ No newline at end of property >> Index: gcc/tree-ssa-alias.c >> =================================================================== >> --- gcc/tree-ssa-alias.c (revision 204453) >> +++ gcc/tree-ssa-alias.c (working copy) >> @@ -2001,23 +2001,24 @@ stmt_may_clobber_ref_p (gimple stmt, tre >> return stmt_may_clobber_ref_p_1 (stmt, &r); >> } >> >> /* If STMT kills the memory reference REF return true, otherwise >> return false. */ >> >> static bool >> stmt_kills_ref_p_1 (gimple stmt, ao_ref *ref) >> { >> /* For a must-alias check we need to be able to constrain >> - the access properly. */ >> - ao_ref_base (ref); >> - if (ref->max_size == -1) >> + the access properly. >> + FIXME: except for BUILTIN_FREE. */ >> + if (!ao_ref_base (ref) >> + || ref->max_size == -1) >> return false; >> >> if (gimple_has_lhs (stmt) >> && TREE_CODE (gimple_get_lhs (stmt)) != SSA_NAME >> /* The assignment is not necessarily carried out if it can throw >> and we can catch it in the current function where we could inspect >> the previous value. >> ??? We only need to care about the RHS throwing. For aggregate >> assignments or similar calls and non-call exceptions the LHS >> might throw as well. */ >> @@ -2090,37 +2091,47 @@ stmt_kills_ref_p_1 (gimple stmt, ao_ref >> case BUILT_IN_MEMPCPY: >> case BUILT_IN_MEMMOVE: >> case BUILT_IN_MEMSET: >> case BUILT_IN_MEMCPY_CHK: >> case BUILT_IN_MEMPCPY_CHK: >> case BUILT_IN_MEMMOVE_CHK: >> case BUILT_IN_MEMSET_CHK: >> { >> tree dest = gimple_call_arg (stmt, 0); >> tree len = gimple_call_arg (stmt, 2); >> - tree base = NULL_TREE; >> - HOST_WIDE_INT offset = 0; >> + tree rbase = ref->base; >> + HOST_WIDE_INT roffset = ref->offset; >> if (!host_integerp (len, 0)) >> return false; >> - if (TREE_CODE (dest) == ADDR_EXPR) >> - base = get_addr_base_and_unit_offset (TREE_OPERAND (dest, >> 0), >> - &offset); >> - else if (TREE_CODE (dest) == SSA_NAME) >> - base = dest; >> - if (base >> - && base == ao_ref_base (ref)) >> + ao_ref dref; >> + ao_ref_init_from_ptr_and_size (&dref, dest, len); > > What I dislike about this is that it can end up building a new tree node. > Not sure if that should block the patch though as it clearly allows to > simplify the code ... > >> + tree base = ao_ref_base (&dref); >> + HOST_WIDE_INT offset = dref.offset; >> + if (!base || dref.size == -1) >> + return false; >> + if (TREE_CODE (base) == MEM_REF) >> + { >> + if (TREE_CODE (rbase) != MEM_REF) > > why's that? I think that just processing both bases separately > would work as well. If they differ, there is no point going on, we might as well break early. And this way we maintain the property that either base and rbase are both refs, or they are both pointers, not some weird mix. >> + return false; >> + // Compare pointers. >> + offset += BITS_PER_UNIT >> + * TREE_INT_CST_LOW (TREE_OPERAND (base, 1)); > > Use mem_ref_offset (base). offset += BITS_PER_UNIT * mem_ref_offset(base).to_shwi(); or did you mean the computations should use double_int? >> + roffset += BITS_PER_UNIT >> + * TREE_INT_CST_LOW (TREE_OPERAND (rbase, 1)); > > Likewise. > >> + base = TREE_OPERAND (base, 0); >> + rbase = TREE_OPERAND (rbase, 0); > > Both could be &decl here, so you want > > if (TREE_CODE (base) == ADDR_EXPR) > base = TREE_OPERAND (base, 0); I rely on the ao_ref* functions to set base to decl and not MEM_REF[&decl], is that a wrong assumption?
On Wed, Nov 6, 2013 at 3:08 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Wed, 6 Nov 2013, Richard Biener wrote: > >> On Wed, Nov 6, 2013 at 1:19 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >>> >>> [Discussion started in >>> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02472.html ] >>> >>> >>> On Wed, 30 Oct 2013, Marc Glisse wrote: >>> >>>> On Wed, 30 Oct 2013, Richard Biener wrote: >>>> >>>>> Btw, get_addr_base_and_unit_offset may also return an offsetted >>>>> MEM_REF (from &MEM [p_3, 17] for example). As we are interested in >>>>> pointers this could be handled by not requiring a memory reference >>>>> but extracting the base address and offset, covering more cases. >>>> >>>> >>>> >>>> I tried the attached patch, and it almost worked, except for one fortran >>>> testcase (widechar_intrinsics_10.f90): >>> >>> >>> >>> Now that ao_ref_init_from_ptr_and_size has been fixed, the following >>> patch >>> passes bootstrap+testsuite (default languages) on >>> x86_64-unknown-linux-gnu. >>> >>> 2013-11-06 Marc Glisse <marc.glisse@inria.fr> >>> Jeff Law <law@redhat.com> >>> >>> gcc/ >>> * tree-ssa-alias.c (stmt_kills_ref_p_1): Use >>> ao_ref_init_from_ptr_and_size for builtins. >>> >>> gcc/testsuite/ >>> * gcc.dg/tree-ssa/alias-27.c: New testcase. >>> >>> -- >>> Marc Glisse >>> Index: gcc/testsuite/gcc.dg/tree-ssa/alias-27.c >>> =================================================================== >>> --- gcc/testsuite/gcc.dg/tree-ssa/alias-27.c (revision 0) >>> +++ gcc/testsuite/gcc.dg/tree-ssa/alias-27.c (working copy) >>> @@ -0,0 +1,11 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-options "-O1 -fdump-tree-optimized" } */ >>> + >>> +void f (long *p) { >>> + *p = 42; >>> + p[4] = 42; >>> + __builtin_memset (p, 0, 100); >>> +} >>> + >>> +/* { dg-final { scan-tree-dump-not "= 42" "optimized" } } */ >>> +/* { dg-final { cleanup-tree-dump "optimized" } } */ >>> >>> Property changes on: gcc/testsuite/gcc.dg/tree-ssa/alias-27.c >>> ___________________________________________________________________ >>> Added: svn:keywords >>> ## -0,0 +1 ## >>> +Author Date Id Revision URL >>> \ No newline at end of property >>> Added: svn:eol-style >>> ## -0,0 +1 ## >>> +native >>> \ No newline at end of property >>> Index: gcc/tree-ssa-alias.c >>> =================================================================== >>> --- gcc/tree-ssa-alias.c (revision 204453) >>> +++ gcc/tree-ssa-alias.c (working copy) >>> @@ -2001,23 +2001,24 @@ stmt_may_clobber_ref_p (gimple stmt, tre >>> return stmt_may_clobber_ref_p_1 (stmt, &r); >>> } >>> >>> /* If STMT kills the memory reference REF return true, otherwise >>> return false. */ >>> >>> static bool >>> stmt_kills_ref_p_1 (gimple stmt, ao_ref *ref) >>> { >>> /* For a must-alias check we need to be able to constrain >>> - the access properly. */ >>> - ao_ref_base (ref); >>> - if (ref->max_size == -1) >>> + the access properly. >>> + FIXME: except for BUILTIN_FREE. */ >>> + if (!ao_ref_base (ref) >>> + || ref->max_size == -1) >>> return false; >>> >>> if (gimple_has_lhs (stmt) >>> && TREE_CODE (gimple_get_lhs (stmt)) != SSA_NAME >>> /* The assignment is not necessarily carried out if it can throw >>> and we can catch it in the current function where we could >>> inspect >>> the previous value. >>> ??? We only need to care about the RHS throwing. For aggregate >>> assignments or similar calls and non-call exceptions the LHS >>> might throw as well. */ >>> @@ -2090,37 +2091,47 @@ stmt_kills_ref_p_1 (gimple stmt, ao_ref >>> case BUILT_IN_MEMPCPY: >>> case BUILT_IN_MEMMOVE: >>> case BUILT_IN_MEMSET: >>> case BUILT_IN_MEMCPY_CHK: >>> case BUILT_IN_MEMPCPY_CHK: >>> case BUILT_IN_MEMMOVE_CHK: >>> case BUILT_IN_MEMSET_CHK: >>> { >>> tree dest = gimple_call_arg (stmt, 0); >>> tree len = gimple_call_arg (stmt, 2); >>> - tree base = NULL_TREE; >>> - HOST_WIDE_INT offset = 0; >>> + tree rbase = ref->base; >>> + HOST_WIDE_INT roffset = ref->offset; >>> if (!host_integerp (len, 0)) >>> return false; >>> - if (TREE_CODE (dest) == ADDR_EXPR) >>> - base = get_addr_base_and_unit_offset (TREE_OPERAND (dest, >>> 0), >>> - &offset); >>> - else if (TREE_CODE (dest) == SSA_NAME) >>> - base = dest; >>> - if (base >>> - && base == ao_ref_base (ref)) >>> + ao_ref dref; >>> + ao_ref_init_from_ptr_and_size (&dref, dest, len); >> >> >> What I dislike about this is that it can end up building a new tree node. >> Not sure if that should block the patch though as it clearly allows to >> simplify the code ... >> >>> + tree base = ao_ref_base (&dref); >>> + HOST_WIDE_INT offset = dref.offset; >>> + if (!base || dref.size == -1) >>> + return false; >>> + if (TREE_CODE (base) == MEM_REF) >>> + { >>> + if (TREE_CODE (rbase) != MEM_REF) >> >> >> why's that? I think that just processing both bases separately >> would work as well. > > > If they differ, there is no point going on, we might as well break early. > And this way we maintain the property that either base and rbase are both > refs, or they are both pointers, not some weird mix. One can be plain 'b' while one MEM[&b, 5], so yes, they differ, but only in offset. >>> + return false; >>> + // Compare pointers. >>> + offset += BITS_PER_UNIT >>> + * TREE_INT_CST_LOW (TREE_OPERAND (base, 1)); >> >> >> Use mem_ref_offset (base). > > > offset += BITS_PER_UNIT * mem_ref_offset(base).to_shwi(); Yes. > or did you mean the computations should use double_int? That would certainly be more correct ... (with a test at the end whether the result fits_shwi ()). >>> + roffset += BITS_PER_UNIT >>> + * TREE_INT_CST_LOW (TREE_OPERAND (rbase, >>> 1)); >> >> >> Likewise. >> >>> + base = TREE_OPERAND (base, 0); >>> + rbase = TREE_OPERAND (rbase, 0); >> >> >> Both could be &decl here, so you want >> >> if (TREE_CODE (base) == ADDR_EXPR) >> base = TREE_OPERAND (base, 0); > > > I rely on the ao_ref* functions to set base to decl and not MEM_REF[&decl], > is that a wrong assumption? Ah, I missed that, yes, that's a correct assumption which also makes my first comment moot. It's been some time since I wrote all this code ... ;) So the only thing that remains is the mem_ref_offset thing and yes, I guess I'd prefer to use double-ints because we deal with bit offsets in the end. Thanks, Richard. > -- > Marc Glisse
Index: gcc/testsuite/gcc.dg/tree-ssa/alias-27.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/alias-27.c (revision 0) +++ gcc/testsuite/gcc.dg/tree-ssa/alias-27.c (working copy) @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-O1 -fdump-tree-optimized" } */ + +void f (long *p) { + *p = 42; + p[4] = 42; + __builtin_memset (p, 0, 100); +} + +/* { dg-final { scan-tree-dump-not "= 42" "optimized" } } */ +/* { dg-final { cleanup-tree-dump "optimized" } } */ Property changes on: gcc/testsuite/gcc.dg/tree-ssa/alias-27.c ___________________________________________________________________ Added: svn:keywords ## -0,0 +1 ## +Author Date Id Revision URL \ No newline at end of property Added: svn:eol-style ## -0,0 +1 ## +native \ No newline at end of property Index: gcc/tree-ssa-alias.c =================================================================== --- gcc/tree-ssa-alias.c (revision 204453) +++ gcc/tree-ssa-alias.c (working copy) @@ -2001,23 +2001,24 @@ stmt_may_clobber_ref_p (gimple stmt, tre return stmt_may_clobber_ref_p_1 (stmt, &r); } /* If STMT kills the memory reference REF return true, otherwise return false. */ static bool stmt_kills_ref_p_1 (gimple stmt, ao_ref *ref) { /* For a must-alias check we need to be able to constrain - the access properly. */ - ao_ref_base (ref); - if (ref->max_size == -1) + the access properly. + FIXME: except for BUILTIN_FREE. */ + if (!ao_ref_base (ref) + || ref->max_size == -1) return false; if (gimple_has_lhs (stmt) && TREE_CODE (gimple_get_lhs (stmt)) != SSA_NAME /* The assignment is not necessarily carried out if it can throw and we can catch it in the current function where we could inspect the previous value. ??? We only need to care about the RHS throwing. For aggregate assignments or similar calls and non-call exceptions the LHS might throw as well. */ @@ -2090,37 +2091,47 @@ stmt_kills_ref_p_1 (gimple stmt, ao_ref case BUILT_IN_MEMPCPY: case BUILT_IN_MEMMOVE: case BUILT_IN_MEMSET: case BUILT_IN_MEMCPY_CHK: case BUILT_IN_MEMPCPY_CHK: case BUILT_IN_MEMMOVE_CHK: case BUILT_IN_MEMSET_CHK: { tree dest = gimple_call_arg (stmt, 0); tree len = gimple_call_arg (stmt, 2); - tree base = NULL_TREE; - HOST_WIDE_INT offset = 0; + tree rbase = ref->base; + HOST_WIDE_INT roffset = ref->offset; if (!host_integerp (len, 0)) return false; - if (TREE_CODE (dest) == ADDR_EXPR) - base = get_addr_base_and_unit_offset (TREE_OPERAND (dest, 0), - &offset); - else if (TREE_CODE (dest) == SSA_NAME) - base = dest; - if (base - && base == ao_ref_base (ref)) + ao_ref dref; + ao_ref_init_from_ptr_and_size (&dref, dest, len); + tree base = ao_ref_base (&dref); + HOST_WIDE_INT offset = dref.offset; + if (!base || dref.size == -1) + return false; + if (TREE_CODE (base) == MEM_REF) + { + if (TREE_CODE (rbase) != MEM_REF) + return false; + // Compare pointers. + offset += BITS_PER_UNIT + * TREE_INT_CST_LOW (TREE_OPERAND (base, 1)); + roffset += BITS_PER_UNIT + * TREE_INT_CST_LOW (TREE_OPERAND (rbase, 1)); + base = TREE_OPERAND (base, 0); + rbase = TREE_OPERAND (rbase, 0); + } + if (base == rbase) { - HOST_WIDE_INT size = TREE_INT_CST_LOW (len); - if (offset <= ref->offset / BITS_PER_UNIT - && (offset + size - >= ((ref->offset + ref->max_size + BITS_PER_UNIT - 1) - / BITS_PER_UNIT))) + HOST_WIDE_INT size = BITS_PER_UNIT * TREE_INT_CST_LOW (len); + if (offset <= roffset + && offset + size >= (roffset + ref->max_size)) return true; } break; } case BUILT_IN_VA_END: { tree ptr = gimple_call_arg (stmt, 0); if (TREE_CODE (ptr) == ADDR_EXPR) {