Re: [PATCH] always create a new MEM for adjust_automodify_address_* (PR38868)

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

On Tue, Jan 20, 2009 at 11:49 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
> PR38668 is a wrong-code bug in the scheduler, in turn caused by a wrong
> MEM_EXPR emitted by memset expansion.  The bug manifests on i686 only,
> but is in target independent code (and in a very used function, though
> it requires a very particular set of arguments to surface).
>
> After emitting a rep stos, the ix86_expand_setmem function keeps the
> destination MEM up-to-date with
>
>    dst = adjust_automodify_address_nv (dst, BLKmode, destreg,
>                                        (count / size_needed) *
>                                        size_needed);
>
> This in turn executes this in adjust_address_1 (MEMREF being dst, and
> ADDR being DESTREG):
>
>  addr = copy_rtx (addr);
>  new = change_address_1 (memref, mode, addr, validate);
>
> While meant primarily to prevent shared RTL, the copy also has the
> effect of not triggering these two lines in change_address_1:
>
>  if (rtx_equal_p (addr, XEXP (memref, 0)) && mode == GET_MODE (memref))
>    return memref;
>
> But in our case the copy is a no-op, because the address is simply a
> register (and not for example a POST_MODIFY).  So adjust_address_1 goes
> on modifying the MEM_EXPR of dst, which is the memory reference that was
> already put in the RTL stream.  As a result, the RTL stream will have a
> MEM whose MEM_EXPR points at the *end* of the data instead of at the
> beginning.
>
> This is fixed simply by adding another copy, this time of the MEM
> itself, is necessary.
>
> Bootstrapped/regtested i686-pc-linux-gnu and (by Dominique)
> i686-pc-darwin9, ok?

Ok.

Thanks,
Richard.

> Paolo
>
> 2008-01-19  Paolo Bonzini  <bonzini@gnu.org>
>
>        PR target/38868
>        * emit-rtl.c (adjust_address_1): Make sure memref is never overwritten.
>
> 2008-01-19  Paolo Bonzini  <bonzini@gnu.org>
>
>        PR target/38868
>        * gfortran.dg/pr38868.f: New testcase.
>
> Index: emit-rtl.c
> ===================================================================
> --- emit-rtl.c  (revision 143495)
> +++ emit-rtl.c  (working copy)
> @@ -2035,6 +2035,11 @@ adjust_address_1 (rtx memref, enum machi
>
>   new_rtx = change_address_1 (memref, mode, addr, validate);
>
> +  /* If the address is a REG, change_address_1 rightfully returns memref,
> +     but this would destroy memref's MEM_ATTRS.  */
> +  if (new_rtx == memref && offset != 0)
> +    new_rtx = copy_rtx (new_rtx);
> +
>   /* Compute the new values of the memory attributes due to this adjustment.
>      We add the offsets and update the alignment.  */
>   if (memoffset)
> Index: testsuite/gfortran.dg/pr38868.f
> ===================================================================
> --- testsuite/gfortran.dg/pr38868.f     (revision 0)
> +++ testsuite/gfortran.dg/pr38868.f     (revision 0)
> @@ -0,0 +1,17 @@
> +! { dg-do compile }
> +! { dg-options "-O2 -fdump-rtl-expand" }
> +      PROGRAM testcase
> +      IMPLICIT NONE
> +
> +      CHARACTER*4 ANER(18)
> +      CHARACTER*80 LINE
> +      aner = ''
> +      ANER(1)='A   '
> +      ANER(2)='    '
> +      LINE=' '
> +      LINE(78:80)='xyz'
> +      WRITE(*,'(A82)') "'"//LINE//"'"
> +      END
> +
> +! { dg-final { scan-rtl-dump-times "line\\\+80" 0 "expand" } }
> +! { dg-final { cleanup-rtl-dump "expand" } } */
>
>