[RFA] Minor fix to aliasing machinery

Marc pointed out that the handling of various BUILT_IN_MEM* and 
BUILT_IN_STR* functions in tree-ssa-alias.c probably wasn't working as 
intended because the code wasn't prepared for a common return value from 
ao_ref_base, particularly returns of MEM_REFs.

This patch fixes the code to handle the trivial case of returning a 
MEM_REF and adds a simple testcase.  There's probably a lot more that 
could be done here.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu.  Ok for 
the trunk?

Thanks,
Jeff
* tree-ssa-alias.c (stmt_kills_ref_p_1): Handle case where
	ao_ref_base returns a MEM_REF.

	* gcc.dg/tree-ssa/alias-26.c: New test.

On Tue, 29 Oct 2013, Jeff Law wrote:

> Marc pointed out that the handling of various BUILT_IN_MEM* and BUILT_IN_STR* 
> functions in tree-ssa-alias.c probably wasn't working as intended because the 
> code wasn't prepared for a common return value from ao_ref_base, particularly 
> returns of MEM_REFs.

Hmm, ao_ref_base is never a pointer, so I'd say the issue is really with 
trying to use the SSA_NAME directly.

> This patch fixes the code to handle the trivial case of returning a MEM_REF 
> and adds a simple testcase.  There's probably a lot more that could be done 
> here.

Thanks.

I am not sure we want to keep the variable "base" that is either a 
decl/ref (from get_addr_base_and_unit_offset) or a pointer (dest). We know 
which case is which, but then forget it by storing both into base. Maybe 
something like this would be more "type-safe".

 	      bool same = false;
               if (TREE_CODE (dest) == ADDR_EXPR)
                 same = (ref_base == get_addr_base_and_unit_offset
 				      (TREE_OPERAND (dest, 0), &offset));
               else if (TREE_CODE (dest) == SSA_NAME
 		       && TREE_CODE (ref_base) == MEM_REF)
                 same = (TREE_OPERAND (ref_base, 0) == dest);
               if (same)
 		...


By the way, I think the patch is fine as is, I am only discussing possible 
follow-ups.

(see http://gcc.gnu.org/ml/gcc-patches/2013-10/txto0PQEYpiuz.txt for 
another approach using ao_ref_init_from_ptr_and_size)

On Tue, Oct 29, 2013 at 10:36 PM, Jeff Law <law@redhat.com> wrote:
>
> Marc pointed out that the handling of various BUILT_IN_MEM* and
> BUILT_IN_STR* functions in tree-ssa-alias.c probably wasn't working as
> intended because the code wasn't prepared for a common return value from
> ao_ref_base, particularly returns of MEM_REFs.
>
> This patch fixes the code to handle the trivial case of returning a MEM_REF
> and adds a simple testcase.  There's probably a lot more that could be done
> here.
>
> Bootstrapped and regression tested on x86_64-unknown-linux-gnu.  Ok for the
> trunk?
>
> Thanks,
> Jeff
>
>         * tree-ssa-alias.c (stmt_kills_ref_p_1): Handle case where
>         ao_ref_base returns a MEM_REF.
>
>         * gcc.dg/tree-ssa/alias-26.c: New test.
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/alias-26.c
> b/gcc/testsuite/gcc.dg/tree-ssa/alias-26.c
> new file mode 100644
> index 0000000..b5625b8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/alias-26.c
> @@ -0,0 +1,12 @@
> +/* { 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" } } */
> +
> diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
> index 4db83bd..5120e72 100644
> --- a/gcc/tree-ssa-alias.c
> +++ b/gcc/tree-ssa-alias.c
> @@ -2079,6 +2079,7 @@ stmt_kills_ref_p_1 (gimple stmt, ao_ref *ref)
>               tree dest = gimple_call_arg (stmt, 0);
>               tree len = gimple_call_arg (stmt, 2);
>               tree base = NULL_TREE;
> +             tree ref_base;
>               HOST_WIDE_INT offset = 0;
>               if (!host_integerp (len, 0))
>                 return false;
> @@ -2087,8 +2088,11 @@ stmt_kills_ref_p_1 (gimple stmt, ao_ref *ref)
>                                                       &offset);
>               else if (TREE_CODE (dest) == SSA_NAME)
>                 base = dest;
> +             ref_base = ao_ref_base (ref);
>               if (base
> -                 && base == ao_ref_base (ref))
> +                 && ((TREE_CODE (ref_base) == MEM_REF
> +                      && base == TREE_OPERAND (ref_base, 0))

That's not sufficient - ref_base may have an offset, so for correctness
you have to check that integer_zerop (TREE_OPERAND (ref_base, 0)).
But this now looks convoluted and somewhat backward, and still
does not catch all cases (including the def-stmt lookup recently
added to ao_ref_from_ptr_and_size).

Richard.

> +                     || ref_base == base))
>                 {
>                   HOST_WIDE_INT size = TREE_INT_CST_LOW (len);
>                   if (offset <= ref->offset / BITS_PER_UNIT
>

On Tue, Oct 29, 2013 at 11:23 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Tue, 29 Oct 2013, Jeff Law wrote:
>
>> Marc pointed out that the handling of various BUILT_IN_MEM* and
>> BUILT_IN_STR* functions in tree-ssa-alias.c probably wasn't working as
>> intended because the code wasn't prepared for a common return value from
>> ao_ref_base, particularly returns of MEM_REFs.
>
>
> Hmm, ao_ref_base is never a pointer, so I'd say the issue is really with
> trying to use the SSA_NAME directly.

Yes.  Note that the code tries to relate two pointers but one is a
memory-reference (ao_ref_base) and one is either a SSA name pointer
or the result of get_addr_base_and_unit_offset (a memory reference as well).

>> This patch fixes the code to handle the trivial case of returning a
>> MEM_REF and adds a simple testcase.  There's probably a lot more that could
>> be done here.
>
>
> Thanks.
>
> I am not sure we want to keep the variable "base" that is either a decl/ref
> (from get_addr_base_and_unit_offset) or a pointer (dest). We know which case
> is which, but then forget it by storing both into base. Maybe something like
> this would be more "type-safe".
>
>               bool same = false;
>               if (TREE_CODE (dest) == ADDR_EXPR)
>                 same = (ref_base == get_addr_base_and_unit_offset
>                                       (TREE_OPERAND (dest, 0), &offset));
>               else if (TREE_CODE (dest) == SSA_NAME
>                        && TREE_CODE (ref_base) == MEM_REF)

   && integer_zerop (TREE_OPERAND (ref_base, 1))

>                 same = (TREE_OPERAND (ref_base, 0) == dest);
>               if (same)
>                 ...

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.

> By the way, I think the patch is fine as is, I am only discussing possible
> follow-ups.

Only slightly wrong-codish ;)

Richard.

> (see http://gcc.gnu.org/ml/gcc-patches/2013-10/txto0PQEYpiuz.txt for another
> approach using ao_ref_init_from_ptr_and_size)
>
> --
> Marc Glisse

On 10/30/13 03:34, Richard Biener wrote:
>>
>>          * tree-ssa-alias.c (stmt_kills_ref_p_1): Handle case where
>>          ao_ref_base returns a MEM_REF.
>>
>>          * gcc.dg/tree-ssa/alias-26.c: New test.
>>
>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/alias-26.c
>> b/gcc/testsuite/gcc.dg/tree-ssa/alias-26.c
>> new file mode 100644
>> index 0000000..b5625b8
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/alias-26.c
>> @@ -0,0 +1,12 @@
>> +/* { 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" } } */
>> +
>> diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
>> index 4db83bd..5120e72 100644
>> --- a/gcc/tree-ssa-alias.c
>> +++ b/gcc/tree-ssa-alias.c
>> @@ -2079,6 +2079,7 @@ stmt_kills_ref_p_1 (gimple stmt, ao_ref *ref)
>>                tree dest = gimple_call_arg (stmt, 0);
>>                tree len = gimple_call_arg (stmt, 2);
>>                tree base = NULL_TREE;
>> +             tree ref_base;
>>                HOST_WIDE_INT offset = 0;
>>                if (!host_integerp (len, 0))
>>                  return false;
>> @@ -2087,8 +2088,11 @@ stmt_kills_ref_p_1 (gimple stmt, ao_ref *ref)
>>                                                        &offset);
>>                else if (TREE_CODE (dest) == SSA_NAME)
>>                  base = dest;
>> +             ref_base = ao_ref_base (ref);
>>                if (base
>> -                 && base == ao_ref_base (ref))
>> +                 && ((TREE_CODE (ref_base) == MEM_REF
>> +                      && base == TREE_OPERAND (ref_base, 0))
>
> That's not sufficient - ref_base may have an offset, so for correctness
> you have to check that integer_zerop (TREE_OPERAND (ref_base, 0)).
> But this now looks convoluted and somewhat backward, and still
> does not catch all cases (including the def-stmt lookup recently
> added to ao_ref_from_ptr_and_size).
So how do you want to proceed?  I'm not really up for burning through 
this code right now and trying to sort out how it ought to work.

Perhaps checkin the test (xfailed) and wait for someone with the 
interest and time to push this through to completion?

jeff