RFA: Fix PR53688

Hi,

On Tue, 19 Jun 2012, Richard Guenther wrote:

> The MEM_REF is acceptable to the tree oracle and it can extract 
> points-to information from it.
> 
> Thus for simplicity unconditionally building the above is the best.

But it doesn't work, as refs_may_alias_p_1 only accepts certain operands 
in MEM_REFs.  So, I opted to check the operand for is_gimple_mem_ref_addr 
after it's built, and if not acceptable at least build a mem-ref for the 
base object, if possible.  In order not to loose info we had before the 
patch I had to improve get_base_address a little to not give up on 
MEM_REFs like "MEM[&p.c]".

Regstrapped on x86_64-linux, no regressions.  Okay for trunk?


Ciao,
Michael.
	PR middle-end/53688
	* gimple.c (get_base_address): Strip components also from inner
	arguments to MEM_REFs.
	* builtins.c (get_memory_rtx): Always build an all-aliasing MEM_REF
	with correct size.

testsuite/
	* gcc.c-torture/execute/pr53688.c: New test.

On Wed, Jun 20, 2012 at 4:57 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Tue, 19 Jun 2012, Richard Guenther wrote:
>
>> The MEM_REF is acceptable to the tree oracle and it can extract
>> points-to information from it.
>>
>> Thus for simplicity unconditionally building the above is the best.
>
> But it doesn't work, as refs_may_alias_p_1 only accepts certain operands
> in MEM_REFs.  So, I opted to check the operand for is_gimple_mem_ref_addr
> after it's built, and if not acceptable at least build a mem-ref for the
> base object, if possible.  In order not to loose info we had before the
> patch I had to improve get_base_address a little to not give up on
> MEM_REFs like "MEM[&p.c]".
>
> Regstrapped on x86_64-linux, no regressions.  Okay for trunk?
>

Hrm ...

> Ciao,
> Michael.
>        PR middle-end/53688
>        * gimple.c (get_base_address): Strip components also from inner
>        arguments to MEM_REFs.
>        * builtins.c (get_memory_rtx): Always build an all-aliasing MEM_REF
>        with correct size.
>
> testsuite/
>        * gcc.c-torture/execute/pr53688.c: New test.
> Index: gimple.c
> ===================================================================
> --- gimple.c    (revision 188772)
> +++ gimple.c    (working copy)
> @@ -2911,7 +2911,11 @@ get_base_address (tree t)
>   if ((TREE_CODE (t) == MEM_REF
>        || TREE_CODE (t) == TARGET_MEM_REF)
>       && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR)
> -    t = TREE_OPERAND (TREE_OPERAND (t, 0), 0);
> +    {
> +      t = TREE_OPERAND (TREE_OPERAND (t, 0), 0);
> +      while (handled_component_p (t))
> +       t = TREE_OPERAND (t, 0);
> +    }
>
>   if (TREE_CODE (t) == SSA_NAME
>       || DECL_P (t)
> Index: builtins.c
> ===================================================================
> --- builtins.c  (revision 188772)
> +++ builtins.c  (working copy)
> @@ -1252,7 +1252,6 @@ get_memory_rtx (tree exp, tree len)
>  {
>   tree orig_exp = exp;
>   rtx addr, mem;
> -  HOST_WIDE_INT off;
>
>   /* When EXP is not resolved SAVE_EXPR, MEM_ATTRS can be still derived
>      from its expression, for expr->a.b only <variable>.a.b is recorded.  */
> @@ -1269,114 +1268,30 @@ get_memory_rtx (tree exp, tree len)
>         && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (exp, 0))))
>     exp = TREE_OPERAND (exp, 0);
>
> -  off = 0;
> -  if (TREE_CODE (exp) == POINTER_PLUS_EXPR
> -      && TREE_CODE (TREE_OPERAND (exp, 0)) == ADDR_EXPR
> -      && host_integerp (TREE_OPERAND (exp, 1), 0)
> -      && (off = tree_low_cst (TREE_OPERAND (exp, 1), 0)) > 0)
> -    exp = TREE_OPERAND (TREE_OPERAND (exp, 0), 0);
> -  else if (TREE_CODE (exp) == ADDR_EXPR)
> -    exp = TREE_OPERAND (exp, 0);
> -  else if (POINTER_TYPE_P (TREE_TYPE (exp)))
> -    exp = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (exp)), exp);
> -  else
> -    exp = NULL;
> -
> -  /* Honor attributes derived from exp, except for the alias set
> -     (as builtin stringops may alias with anything) and the size
> -     (as stringops may access multiple array elements).  */
> -  if (exp)
> +  /* Build a MEM_REF representing the whole accessed area as a byte blob,
> +     (as builtin stringops may alias with anything).  */
> +  exp = fold_build2 (MEM_REF,
> +                    build_array_type (char_type_node,
> +                                      build_range_type (sizetype,
> +                                                        size_one_node, len)),
> +                    exp, build_int_cst (ptr_type_node, 0));
> +
> +  /* If the MEM_REF has no acceptable address, try to get the base object,
> +     and build an all-aliasing unknown-sized access to that one.  */
> +  if (!is_gimple_mem_ref_addr (TREE_OPERAND (exp, 0))
> +      && (exp = get_base_address (exp)))

The get_base_address massaging should be not necessary if you'd
use the original exp here, not the built MEM_REF.

Otherwise looks ok.

Thanks,
Richard.

>     {
> -      set_mem_attributes (mem, exp, 0);
> -
> -      if (off)
> -       mem = adjust_automodify_address_nv (mem, BLKmode, NULL, off);
> -
> -      /* Allow the string and memory builtins to overflow from one
> -        field into another, see http://gcc.gnu.org/PR23561.
> -        Thus avoid COMPONENT_REFs in MEM_EXPR unless we know the whole
> -        memory accessed by the string or memory builtin will fit
> -        within the field.  */
> -      if (MEM_EXPR (mem) && TREE_CODE (MEM_EXPR (mem)) == COMPONENT_REF)
> -       {
> -         tree mem_expr = MEM_EXPR (mem);
> -         HOST_WIDE_INT offset = -1, length = -1;
> -         tree inner = exp;
> -
> -         while (TREE_CODE (inner) == ARRAY_REF
> -                || CONVERT_EXPR_P (inner)
> -                || TREE_CODE (inner) == VIEW_CONVERT_EXPR
> -                || TREE_CODE (inner) == SAVE_EXPR)
> -           inner = TREE_OPERAND (inner, 0);
> -
> -         gcc_assert (TREE_CODE (inner) == COMPONENT_REF);
> -
> -         if (MEM_OFFSET_KNOWN_P (mem))
> -           offset = MEM_OFFSET (mem);
> -
> -         if (offset >= 0 && len && host_integerp (len, 0))
> -           length = tree_low_cst (len, 0);
> -
> -         while (TREE_CODE (inner) == COMPONENT_REF)
> -           {
> -             tree field = TREE_OPERAND (inner, 1);
> -             gcc_assert (TREE_CODE (mem_expr) == COMPONENT_REF);
> -             gcc_assert (field == TREE_OPERAND (mem_expr, 1));
> -
> -             /* Bitfields are generally not byte-addressable.  */
> -             gcc_assert (!DECL_BIT_FIELD (field)
> -                         || ((tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
> -                              % BITS_PER_UNIT) == 0
> -                             && host_integerp (DECL_SIZE (field), 0)
> -                             && (TREE_INT_CST_LOW (DECL_SIZE (field))
> -                                 % BITS_PER_UNIT) == 0));
> -
> -             /* If we can prove that the memory starting at XEXP (mem, 0) and
> -                ending at XEXP (mem, 0) + LENGTH will fit into this field, we
> -                can keep the COMPONENT_REF in MEM_EXPR.  But be careful with
> -                fields without DECL_SIZE_UNIT like flexible array members.  */
> -             if (length >= 0
> -                 && DECL_SIZE_UNIT (field)
> -                 && host_integerp (DECL_SIZE_UNIT (field), 0))
> -               {
> -                 HOST_WIDE_INT size
> -                   = TREE_INT_CST_LOW (DECL_SIZE_UNIT (field));
> -                 if (offset <= size
> -                     && length <= size
> -                     && offset + length <= size)
> -                   break;
> -               }
> -
> -             if (offset >= 0
> -                 && host_integerp (DECL_FIELD_OFFSET (field), 0))
> -               offset += TREE_INT_CST_LOW (DECL_FIELD_OFFSET (field))
> -                         + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
> -                           / BITS_PER_UNIT;
> -             else
> -               {
> -                 offset = -1;
> -                 length = -1;
> -               }
> -
> -             mem_expr = TREE_OPERAND (mem_expr, 0);
> -             inner = TREE_OPERAND (inner, 0);
> -           }
> -
> -         if (mem_expr == NULL)
> -           offset = -1;
> -         if (mem_expr != MEM_EXPR (mem))
> -           {
> -             set_mem_expr (mem, mem_expr);
> -             if (offset >= 0)
> -               set_mem_offset (mem, offset);
> -             else
> -               clear_mem_offset (mem);
> -           }
> -       }
> -      set_mem_alias_set (mem, 0);
> -      clear_mem_size (mem);
> +      exp = build_fold_addr_expr (exp);
> +      exp = fold_build2 (MEM_REF,
> +                        build_array_type (char_type_node,
> +                                          build_range_type (sizetype,
> +                                                            size_zero_node,
> +                                                            NULL)),
> +                        exp, build_int_cst (ptr_type_node, 0));
>     }
> -
> +  if (exp)
> +    set_mem_attributes (mem, exp, 0);
> +  set_mem_alias_set (mem, 0);
>   return mem;
>  }
>
> Index: testsuite/gcc.c-torture/execute/pr53688.c
> ===================================================================
> --- testsuite/gcc.c-torture/execute/pr53688.c   (revision 0)
> +++ testsuite/gcc.c-torture/execute/pr53688.c   (revision 0)
> @@ -0,0 +1,32 @@
> +char headline[256];
> +struct hdr {
> +  char part1[9];
> +  char part2[8];
> +} p;
> +
> +void __attribute__((noinline,noclone))
> +init()
> +{
> +  __builtin_memcpy (p.part1, "FOOBARFOO", sizeof (p.part1));
> +  __builtin_memcpy (p.part2, "SPEC CPU", sizeof (p.part2));
> +}
> +
> +int main()
> +{
> +  char *x;
> +  int c;
> +  init();
> +  __builtin_memcpy (&headline[0], p.part1, 9);
> +  c = 9;
> +  x = &headline[0];
> +  x = x + c;
> +  __builtin_memset (x, ' ', 245);
> +  __builtin_memcpy (&headline[10], p.part2, 8);
> +  c = 18;
> +  x = &headline[0];
> +  x = x + c;
> +  __builtin_memset (x, ' ', 238);
> +  if (headline[10] != 'S')
> +    __builtin_abort ();
> +  return 0;
> +}

Hi,

On Wed, 20 Jun 2012, Richard Guenther wrote:

> > +  exp = fold_build2 (MEM_REF,
> > +                    build_array_type (char_type_node,
> > +                                      build_range_type (sizetype,
> > +                                                        size_one_node, len)),
> > +                    exp, build_int_cst (ptr_type_node, 0));
> > +
> > +  /* If the MEM_REF has no acceptable address, try to get the base object,
> > +     and build an all-aliasing unknown-sized access to that one.  */
> > +  if (!is_gimple_mem_ref_addr (TREE_OPERAND (exp, 0))
> > +      && (exp = get_base_address (exp)))
> 
> The get_base_address massaging should be not necessary if you'd
> use the original exp here, not the built MEM_REF.

Hmm?  The original expression is an address, I have to build a MEM_REF out 
of that, and the is_gimple_mem_ref_addr() just checked that that very 
address (after going through fold) is not acceptable as MEM_REF operand.  
So how could I avoid the massaging of the address to make it an acceptable 
operand?


Ciao,
Michael.

On Wed, Jun 20, 2012 at 5:09 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Wed, 20 Jun 2012, Richard Guenther wrote:
>
>> > +  exp = fold_build2 (MEM_REF,
>> > +                    build_array_type (char_type_node,
>> > +                                      build_range_type (sizetype,
>> > +                                                        size_one_node, len)),
>> > +                    exp, build_int_cst (ptr_type_node, 0));
>> > +
>> > +  /* If the MEM_REF has no acceptable address, try to get the base object,
>> > +     and build an all-aliasing unknown-sized access to that one.  */
>> > +  if (!is_gimple_mem_ref_addr (TREE_OPERAND (exp, 0))
>> > +      && (exp = get_base_address (exp)))
>>
>> The get_base_address massaging should be not necessary if you'd
>> use the original exp here, not the built MEM_REF.
>
> Hmm?  The original expression is an address, I have to build a MEM_REF out
> of that, and the is_gimple_mem_ref_addr() just checked that that very
> address (after going through fold) is not acceptable as MEM_REF operand.
> So how could I avoid the massaging of the address to make it an acceptable
> operand?

Not change get_base_addres and use

  if (!is_gimple_mem_ref_addr (TREE_OPERAND (exp, 0))
     && (exp = get_base_address (TREE_OPERAND (orig_exp, 0))))

Richard.