Re: PR 58958: wrong aliasing info

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

On Mon, Nov 4, 2013 at 11:55 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Fri, Nov 1, 2013 at 11:39 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> Hello,
>>
>> the issue was described in the PR and the message linked from there.
>> ao_ref_init_from_ptr_and_size calls get_ref_base_and_extent, which may
>> detect an array_ref of variable index, but ao_ref_init_from_ptr_and_size
>> never learns of it and uses the offset+size as if they were meaningful.
>
> Well...  it certainly learns of it, but it chooses to ignore...
>
>    if (TREE_CODE (ptr) == ADDR_EXPR)
> -    ref->base = get_ref_base_and_extent (TREE_OPERAND (ptr, 0),
> -                                        &ref->offset, &t1, &t2);
> +    {
> +      ref->base = get_addr_base_and_unit_offset_1 (TREE_OPERAND (ptr, 0),
> +                                                  &t, 0, &safe);
> +      ref->offset = BITS_PER_UNIT * t;
> +    }
>
> safe == (t1 != -1 && t1 == t2)
>
> note that ao_ref_init_from_ptr_and_size gets the size fed in as argument
> so I fail to see why it matters at all ...?  That is, if you feed in a wrong
> size then it's the callers error.

I think one issue is that the above code uses get_ref_base_and_extent
on an address.  It should have used get_addr_base_and_unit_offset.

Richard.

> Richard.
>
>> Bootstrap+testsuite on x86_64-unknown-linux-gnu.
>>
>> 2013-11-04  Marc Glisse  <marc.glisse@inria.fr>
>>
>>         PR tree-optimization/
>> gcc/
>>         * tree-dfa.h (get_addr_base_and_unit_offset_1): Add error reporting.
>>         * tree-ssa-alias.c (ao_ref_init_from_ptr_and_size): Use
>>         get_addr_base_and_unit_offset_1 instead of get_ref_base_and_extent.
>>
>> gcc/testsuite/
>>         * gcc.dg/tree-ssa/pr58958.c: New file.
>>
>> --
>> Marc Glisse
>> Index: gcc/testsuite/gcc.dg/tree-ssa/pr58958.c
>> ===================================================================
>> --- gcc/testsuite/gcc.dg/tree-ssa/pr58958.c     (revision 0)
>> +++ gcc/testsuite/gcc.dg/tree-ssa/pr58958.c     (working copy)
>> @@ -0,0 +1,12 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fdump-tree-optimized" } */
>> +
>> +double a[10];
>> +int f(int n){
>> +  a[3]=9;
>> +  __builtin_memset(&a[n],3,sizeof(double));
>> +  return a[3]==9;
>> +}
>> +
>> +/* { dg-final { scan-tree-dump " == 9" "optimized" } } */
>> +/* { dg-final { cleanup-tree-dump "optimized" } } */
>>
>> Property changes on: gcc/testsuite/gcc.dg/tree-ssa/pr58958.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-dfa.h
>> ===================================================================
>> --- gcc/tree-dfa.h      (revision 204302)
>> +++ gcc/tree-dfa.h      (working copy)
>> @@ -30,65 +30,70 @@ extern tree ssa_default_def (struct func
>>  extern void set_ssa_default_def (struct function *, tree, tree);
>>  extern tree get_or_create_ssa_default_def (struct function *, tree);
>>  extern tree get_ref_base_and_extent (tree, HOST_WIDE_INT *,
>>                                      HOST_WIDE_INT *, HOST_WIDE_INT *);
>>  extern tree get_addr_base_and_unit_offset (tree, HOST_WIDE_INT *);
>>  extern bool stmt_references_abnormal_ssa_name (gimple);
>>  extern void dump_enumerated_decls (FILE *, int);
>>
>>  /* Returns the base object and a constant BITS_PER_UNIT offset in *POFFSET
>> that
>>     denotes the starting address of the memory access EXP.
>> -   Returns NULL_TREE if the offset is not constant or any component
>> -   is not BITS_PER_UNIT-aligned.
>> +   If the offset is not constant or any component is not
>> BITS_PER_UNIT-aligned,
>> +   sets *SAFE to false or returns NULL_TREE if SAFE is NULL.
>>     VALUEIZE if non-NULL is used to valueize SSA names.  It should return
>>     its argument or a constant if the argument is known to be constant.  */
>>  /* ??? This is a static inline here to avoid the overhead of the indirect
>> calls
>>     to VALUEIZE.  But is this overhead really that significant?  And should
>> we
>>     perhaps just rely on WHOPR to specialize the function?  */
>>
>>  static inline tree
>>  get_addr_base_and_unit_offset_1 (tree exp, HOST_WIDE_INT *poffset,
>> -                                tree (*valueize) (tree))
>> +                                tree (*valueize) (tree), bool *safe = NULL)
>>  {
>>    HOST_WIDE_INT byte_offset = 0;
>> +  bool issafe = true;
>>
>>    /* Compute cumulative byte-offset for nested component-refs and
>> array-refs,
>>       and find the ultimate containing object.  */
>>    while (1)
>>      {
>>        switch (TREE_CODE (exp))
>>         {
>>         case BIT_FIELD_REF:
>>           {
>>             HOST_WIDE_INT this_off = TREE_INT_CST_LOW (TREE_OPERAND (exp,
>> 2));
>>             if (this_off % BITS_PER_UNIT)
>> -             return NULL_TREE;
>> -           byte_offset += this_off / BITS_PER_UNIT;
>> +             issafe = false;
>> +           else
>> +             byte_offset += this_off / BITS_PER_UNIT;
>>           }
>>           break;
>>
>>         case COMPONENT_REF:
>>           {
>>             tree field = TREE_OPERAND (exp, 1);
>>             tree this_offset = component_ref_field_offset (exp);
>>             HOST_WIDE_INT hthis_offset;
>>
>>             if (!this_offset
>>                 || TREE_CODE (this_offset) != INTEGER_CST
>>                 || (TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field))
>>                     % BITS_PER_UNIT))
>> -             return NULL_TREE;
>> -
>> -           hthis_offset = TREE_INT_CST_LOW (this_offset);
>> -           hthis_offset += (TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET
>> (field))
>> -                            / BITS_PER_UNIT);
>> -           byte_offset += hthis_offset;
>> +             issafe = false;
>> +           else
>> +             {
>> +               hthis_offset = TREE_INT_CST_LOW (this_offset);
>> +               hthis_offset +=
>> +                 (TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field))
>> +                  / BITS_PER_UNIT);
>> +               byte_offset += hthis_offset;
>> +             }
>>           }
>>           break;
>>
>>         case ARRAY_REF:
>>         case ARRAY_RANGE_REF:
>>           {
>>             tree index = TREE_OPERAND (exp, 1);
>>             tree low_bound, unit_size;
>>
>>             if (valueize
>> @@ -102,21 +107,21 @@ get_addr_base_and_unit_offset_1 (tree ex
>>                 && (unit_size = array_ref_element_size (exp),
>>                     TREE_CODE (unit_size) == INTEGER_CST))
>>               {
>>                 HOST_WIDE_INT hindex = TREE_INT_CST_LOW (index);
>>
>>                 hindex -= TREE_INT_CST_LOW (low_bound);
>>                 hindex *= TREE_INT_CST_LOW (unit_size);
>>                 byte_offset += hindex;
>>               }
>>             else
>> -             return NULL_TREE;
>> +             issafe = false;
>>           }
>>           break;
>>
>>         case REALPART_EXPR:
>>           break;
>>
>>         case IMAGPART_EXPR:
>>           byte_offset += TREE_INT_CST_LOW (TYPE_SIZE_UNIT (TREE_TYPE
>> (exp)));
>>           break;
>>
>> @@ -148,37 +153,48 @@ get_addr_base_and_unit_offset_1 (tree ex
>>           {
>>             tree base = TREE_OPERAND (exp, 0);
>>             if (valueize
>>                 && TREE_CODE (base) == SSA_NAME)
>>               base = (*valueize) (base);
>>
>>             /* Hand back the decl for MEM[&decl, off].  */
>>             if (TREE_CODE (base) == ADDR_EXPR)
>>               {
>>                 if (TMR_INDEX (exp) || TMR_INDEX2 (exp))
>> -                 return NULL_TREE;
>> -               if (!integer_zerop (TMR_OFFSET (exp)))
>> +                 issafe = false;
>> +               else if (!integer_zerop (TMR_OFFSET (exp)))
>>                   {
>>                     double_int off = mem_ref_offset (exp);
>>                     gcc_assert (off.high == -1 || off.high == 0);
>>                     byte_offset += off.to_shwi ();
>>                   }
>>                 exp = TREE_OPERAND (base, 0);
>>               }
>>             goto done;
>>           }
>>
>>         default:
>>           goto done;
>>         }
>>
>>        exp = TREE_OPERAND (exp, 0);
>>      }
>>  done:
>>
>> -  *poffset = byte_offset;
>> -  return exp;
>> +  if (issafe)
>> +    {
>> +      *poffset = byte_offset;
>> +      return exp;
>> +    }
>> +  else if (safe)
>> +    {
>> +      *safe = false;
>> +      *poffset = 0;
>> +      return exp;
>> +    }
>> +  else
>> +    return NULL_TREE;
>>  }
>>
>>
>>
>>  #endif /* GCC_TREE_DFA_H */
>> Index: gcc/tree-ssa-alias.c
>> ===================================================================
>> --- gcc/tree-ssa-alias.c        (revision 204302)
>> +++ gcc/tree-ssa-alias.c        (working copy)
>> @@ -559,49 +559,54 @@ 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, extra_offset = 0;
>> +  HOST_WIDE_INT t, extra_offset = 0;
>> +  bool safe = true;
>>    ref->ref = NULL_TREE;
>>    if (TREE_CODE (ptr) == SSA_NAME)
>>      {
>>        gimple stmt = SSA_NAME_DEF_STMT (ptr);
>>        if (gimple_assign_single_p (stmt)
>>           && gimple_assign_rhs_code (stmt) == ADDR_EXPR)
>>         ptr = gimple_assign_rhs1 (stmt);
>>        else if (is_gimple_assign (stmt)
>>                && gimple_assign_rhs_code (stmt) == POINTER_PLUS_EXPR
>>                && host_integerp (gimple_assign_rhs2 (stmt), 0)
>> -              && (t1 = int_cst_value (gimple_assign_rhs2 (stmt))) >= 0)
>> +              && (t = int_cst_value (gimple_assign_rhs2 (stmt))) >= 0)
>>         {
>>           ptr = gimple_assign_rhs1 (stmt);
>> -         extra_offset = BITS_PER_UNIT * t1;
>> +         extra_offset = BITS_PER_UNIT * t;
>>         }
>>      }
>>
>>    if (TREE_CODE (ptr) == ADDR_EXPR)
>> -    ref->base = get_ref_base_and_extent (TREE_OPERAND (ptr, 0),
>> -                                        &ref->offset, &t1, &t2);
>> +    {
>> +      ref->base = get_addr_base_and_unit_offset_1 (TREE_OPERAND (ptr, 0),
>> +                                                  &t, 0, &safe);
>> +      ref->offset = BITS_PER_UNIT * t;
>> +    }
>>    else
>>      {
>>        ref->base = build2 (MEM_REF, char_type_node,
>>                           ptr, null_pointer_node);
>>        ref->offset = 0;
>>      }
>>    ref->offset += extra_offset;
>> -  if (size
>> +  if (safe
>> +      && size
>>        && host_integerp (size, 0)
>>        && TREE_INT_CST_LOW (size) * BITS_PER_UNIT / BITS_PER_UNIT
>>          == TREE_INT_CST_LOW (size))
>>      ref->max_size = ref->size = TREE_INT_CST_LOW (size) * BITS_PER_UNIT;
>>    else
>>      ref->max_size = ref->size = -1;
>>    ref->ref_alias_set = 0;
>>    ref->base_alias_set = 0;
>>    ref->volatile_p = false;
>>  }
>>