[PATCH] Add some REG_POINTER flags during ix86 memcpy/memset expansion (PR target/65504)

[PATCH] Add some REG_POINTER flags during ix86 memcpy/memset expansion (PR target/65504)

[Jakub Jelinek]

Hi!

As expand_set_or_movmem_prologue_epilogue_by_misaligned_moves uses
src = src - (adjusted_dest - dest)
without proper REG_POINTER flags the aliasing code is very easily confused
on what is really a pointer and what is not - as REG_POINTER was used
after forwprop only on dest, but not on anything else, aliasing code thinks
that the memcpy reads dest based memory, when it really reads src based
memory (in the testcase frame pointer based in the end, but CSE did not see
it through).

Fixed by marking pseudos that must hold pointers with REG_POINTER during
the expansion.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok
for trunk?

2015-03-23  Jakub Jelinek  <jakub@redhat.com>

	PR target/65504
	* config/i386/i386.c (ix86_copy_addr_to_reg): Set REG_POINTER
	on the pseudo.
	(expand_set_or_movmem_prologue_epilogue_by_misaligned_moves): Set
	REG_POINTER on *destptr after adjusting it for prologue size.

	* gfortran.dg/pr65504.f90: New test.

--- gcc/config/i386/i386.c.jj	2015-03-23 08:47:53.000000000 +0100
+++ gcc/config/i386/i386.c	2015-03-23 13:38:40.573983052 +0100
@@ -23457,12 +23457,19 @@ counter_mode (rtx count_exp)
 static rtx
 ix86_copy_addr_to_reg (rtx addr)
 {
+  rtx reg;
   if (GET_MODE (addr) == Pmode || GET_MODE (addr) == VOIDmode)
-    return copy_addr_to_reg (addr);
+    {
+      reg = copy_addr_to_reg (addr);
+      REG_POINTER (reg) = 1;
+      return reg;
+    }
   else
     {
       gcc_assert (GET_MODE (addr) == DImode && Pmode == SImode);
-      return gen_rtx_SUBREG (SImode, copy_to_mode_reg (DImode, addr), 0);
+      reg = copy_to_mode_reg (DImode, addr);
+      REG_POINTER (reg) = 1;
+      return gen_rtx_SUBREG (SImode, reg, 0);
     }
 }
 
@@ -24354,6 +24361,8 @@ expand_set_or_movmem_prologue_epilogue_b
       *destptr = expand_simple_binop (GET_MODE (*destptr), PLUS, *destptr,
 				      GEN_INT (prolog_size),
 				      NULL_RTX, 1, OPTAB_DIRECT);
+      if (REG_P (*destptr) && REG_P (saveddest) && REG_POINTER (saveddest))
+	REG_POINTER (*destptr) = 1;
       *destptr = expand_simple_binop (GET_MODE (*destptr), AND, *destptr,
 				      GEN_INT (-desired_align),
 				      *destptr, 1, OPTAB_DIRECT);
@@ -24363,8 +24372,8 @@ expand_set_or_movmem_prologue_epilogue_b
 				       saveddest, 1, OPTAB_DIRECT);
       /* Adjust srcptr and count.  */
       if (!issetmem)
-	*srcptr = expand_simple_binop (GET_MODE (*srcptr), MINUS, *srcptr, saveddest,
-					*srcptr, 1, OPTAB_DIRECT);
+	*srcptr = expand_simple_binop (GET_MODE (*srcptr), MINUS, *srcptr,
+				       saveddest, *srcptr, 1, OPTAB_DIRECT);
       *count = expand_simple_binop (GET_MODE (*count), PLUS, *count,
 				    saveddest, *count, 1, OPTAB_DIRECT);
       /* We copied at most size + prolog_size.  */
--- gcc/testsuite/gfortran.dg/pr65504.f90.jj	2015-03-23 13:39:44.050945860 +0100
+++ gcc/testsuite/gfortran.dg/pr65504.f90	2015-03-23 13:39:57.707722713 +0100
@@ -0,0 +1,28 @@
+! PR target/65504
+! { dg-do run }
+
+program pr65504
+  implicit none
+  type :: T
+    character (len=256) :: a
+    character (len=256) :: b
+  end type T
+  type (T) :: c
+  type (T) :: d
+  c = foo ("test")
+  d = foo ("test")
+  if (trim(c%b) .ne. "foo") call abort
+  contains
+  type (T) function foo (x) result (v)
+    character(len=*), intent(in) :: x
+    select case (x)
+    case ("test")
+      v%b = 'foo'
+    case ("bazx")
+      v%b = 'barx'
+    case default
+      print *, "unknown"
+      stop
+    end select
+  end function foo
+end program pr65504

	Jakub

Re: [PATCH] Add some REG_POINTER flags during ix86 memcpy/memset expansion (PR target/65504)

[Uros Bizjak]

On Mon, Mar 23, 2015 at 4:07 PM, Jakub Jelinek <jakub@redhat.com> wrote:

> As expand_set_or_movmem_prologue_epilogue_by_misaligned_moves uses
> src = src - (adjusted_dest - dest)
> without proper REG_POINTER flags the aliasing code is very easily confused
> on what is really a pointer and what is not - as REG_POINTER was used
> after forwprop only on dest, but not on anything else, aliasing code thinks
> that the memcpy reads dest based memory, when it really reads src based
> memory (in the testcase frame pointer based in the end, but CSE did not see
> it through).
>
> Fixed by marking pseudos that must hold pointers with REG_POINTER during
> the expansion.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok
> for trunk?
>
> 2015-03-23  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/65504
>         * config/i386/i386.c (ix86_copy_addr_to_reg): Set REG_POINTER
>         on the pseudo.
>         (expand_set_or_movmem_prologue_epilogue_by_misaligned_moves): Set
>         REG_POINTER on *destptr after adjusting it for prologue size.
>
>         * gfortran.dg/pr65504.f90: New test.

OK.

Thanks,
Uros.

Re: [PATCH] Add some REG_POINTER flags during ix86 memcpy/memset expansion (PR target/65504)

[Jeff Law]

On 03/23/2015 09:07 AM, Jakub Jelinek wrote:
> Hi!
>
> As expand_set_or_movmem_prologue_epilogue_by_misaligned_moves uses
> src = src - (adjusted_dest - dest)
> without proper REG_POINTER flags the aliasing code is very easily confused
> on what is really a pointer and what is not - as REG_POINTER was used
> after forwprop only on dest, but not on anything else, aliasing code thinks
> that the memcpy reads dest based memory, when it really reads src based
> memory (in the testcase frame pointer based in the end, but CSE did not see
> it through).
>
> Fixed by marking pseudos that must hold pointers with REG_POINTER during
> the expansion.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok
> for trunk?
FWIW, you have to be very careful depending on REG_POINTER.  I believe 
Ada can still set REG_POINTER on things that are not pointers (via 
virtual origins) and cross jumping can cause problems too where one arm 
has x + y with X as the pointer and the other arm has x + y with Y as 
the pointer.

But yes, in general, if we're marking things that must be pointers with 
REG_POINTER, that is a good thing.

jeff

Re: [PATCH] Add some REG_POINTER flags during ix86 memcpy/memset expansion (PR target/65504)

[Jan Hubicka]

> On 03/23/2015 09:07 AM, Jakub Jelinek wrote:
> >Hi!
> >
> >As expand_set_or_movmem_prologue_epilogue_by_misaligned_moves uses
> >src = src - (adjusted_dest - dest)
> >without proper REG_POINTER flags the aliasing code is very easily confused
> >on what is really a pointer and what is not - as REG_POINTER was used
> >after forwprop only on dest, but not on anything else, aliasing code thinks
> >that the memcpy reads dest based memory, when it really reads src based
> >memory (in the testcase frame pointer based in the end, but CSE did not see
> >it through).
> >
> >Fixed by marking pseudos that must hold pointers with REG_POINTER during
> >the expansion.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok
> >for trunk?
> FWIW, you have to be very careful depending on REG_POINTER.  I
> believe Ada can still set REG_POINTER on things that are not
> pointers (via virtual origins) and cross jumping can cause problems
> too where one arm has x + y with X as the pointer and the other arm
> has x + y with Y as the pointer.
> 
> But yes, in general, if we're marking things that must be pointers
> with REG_POINTER, that is a good thing.

Yep, the patch is OK.  Even if Ada gets things partly wrong it seems independent
issue.

Honza
> 
> jeff
> 

Re: [PATCH] Add some REG_POINTER flags during ix86 memcpy/memset expansion (PR target/65504)

[Eric Botcazou]

> FWIW, you have to be very careful depending on REG_POINTER.  I believe
> Ada can still set REG_POINTER on things that are not pointers (via
> virtual origins) and cross jumping can cause problems too where one arm
> has x + y with X as the pointer and the other arm has x + y with Y as
> the pointer.

Can you elaborate a bit about Ada here?  Front-ends don't fiddle directly with 
RTL for years so I'm a little at a loss here.

-- 
Eric Botcazou

Re: [PATCH] Add some REG_POINTER flags during ix86 memcpy/memset expansion (PR target/65504)

[Jeff Law]

On 03/23/2015 03:33 PM, Eric Botcazou wrote:
>> FWIW, you have to be very careful depending on REG_POINTER.  I believe
>> Ada can still set REG_POINTER on things that are not pointers (via
>> virtual origins) and cross jumping can cause problems too where one arm
>> has x + y with X as the pointer and the other arm has x + y with Y as
>> the pointer.
>
> Can you elaborate a bit about Ada here?  Front-ends don't fiddle directly with
> RTL for years so I'm a little at a loss here.
It's been years since I looked at this, but my recollection was that the 
Ada front end could set REG_POINTER for virtual origins.  From a 
correctness standpoint it only matters on a few obscure targets and only 
causes a failure when the flag is set on a pseudo with a value on a 
different, unmapped, page than the full effective address.

Jeff