Re: [RFA][PATCH] Minor fix to aliasing machinery

[Richard Biener <richard dot guenther at gmail dot com>]

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