Message ID | 20150323150738.GX1746@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
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.
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
> 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 >
> 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.
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
--- 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