Latent bug in update_equiv_regs?

Latent bug in update_equiv_regs?

[Jeff Law]

Somehow I can't help but think I'm missing something here...

Given:

   (set (reg X) (mem Y))

   (...)

   (set (mem Y) (reg Z))

   (...)

   (use  (reg X))



update_equiv_regs can set an equivalence between (reg X) and (mem Y) 
which is clearly wrong as (mem Y) is set to (reg Z).

99.99% of the time this doesn't cause us any problems.  However, imagine 
if (reg X) is not assigned a hard register.  Reload then gleefully 
replaces uses of (reg X) with its equivalent memory location.  Given the 
RTL above that's clearly bad.

It looks like this code is supposed to wipe the problematic equivalence:

           /* We only handle the case of a pseudo register being set
              once, or always to the same value.  */
           /* ??? The mn10200 port breaks if we add equivalences for
              values that need an ADDRESS_REGS register and set them 
equivalent
              to a MEM of a pseudo.  The actual problem is in the 
over-conservative
              handling of INPADDR_ADDRESS / INPUT_ADDRESS / INPUT triples in
              calculate_needs, but we traditionally work around this problem
              here by rejecting equivalences when the destination is in 
a register
              that's likely spilled.  This is fragile, of course, since the
              preferred class of a pseudo depends on all instructions 
that set
              or use it.  */

           if (!REG_P (dest)
               || (regno = REGNO (dest)) < FIRST_PSEUDO_REGISTER
               || reg_equiv[regno].init_insns == const0_rtx
               || (CLASS_LIKELY_SPILLED_P (reg_preferred_class (regno))
&& MEM_P (src) && ! reg_equiv[regno].is_arg_equivalence))
             {
               /* This might be setting a SUBREG of a pseudo, a pseudo 
that is
                  also set somewhere else to a constant.  */
               note_stores (set, no_equiv, NULL);
               continue;
             }

However, if we look at no_equiv we see the following:

/* Mark REG as having no known equivalence.
    Some instructions might have been processed before and furnished
    with REG_EQUIV notes for this register; these notes will have to be
    removed.
    STORE is the piece of RTL that does the non-constant / conflicting
    assignment - a SET, CLOBBER or REG_INC note.  It is currently not used,
    but needs to be there because this function is called from 
note_stores.  */
static void
no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSED, void *data 
ATTRIBUTE_UNUSED)
{
   int regno;
   rtx list;

   if (!REG_P (reg))
     return;

[ ... ]

Thus no_equiv doesn't do anything when passed a MEM.

I haven't tried triggering this problem with the mainline sources...

What am I missing?

jeff

Re: Latent bug in update_equiv_regs?

[Ian Lance Taylor]

Jeff Law <law@redhat.com> writes:

> Somehow I can't help but think I'm missing something here...
>
> Given:
>
>   (set (reg X) (mem Y))
>
>   (...)
>
>   (set (mem Y) (reg Z))
>
>   (...)
>
>   (use  (reg X))
>
>
>
> update_equiv_regs can set an equivalence between (reg X) and (mem Y)
> which is clearly wrong as (mem Y) is set to (reg Z).

My understanding is that that scenario is supposed to not happen because
update_equiv_regs is only supposed to equate a register and a memory
location in the specific cases where that is OK.  It's not no_equiv that
is supposed to fix this, the equivalence should only be created when it
will always be OK.

So I think you need to explain more about why the equivalence was
created.

Ian

Re: Latent bug in update_equiv_regs?

[Jeff Law]

On 08/19/09 17:46, Ian Lance Taylor wrote:
>
> My understanding is that that scenario is supposed to not happen because
> update_equiv_regs is only supposed to equate a register and a memory
> location in the specific cases where that is OK.  It's not no_equiv that
> is supposed to fix this, the equivalence should only be created when it
> will always be OK.
>
> So I think you need to explain more about why the equivalence was
> created.
>
> Ian
>    
You're right.  This should have been rejected by validate_equiv_mem, but 
isn't because the two memory references are in different alias sets.

You can see this in the mainline sources configured for 
i686-pc-linux-gnu by compiling 
libgomp/testsuite/libgomp.fortran/reduction1.f90 with -O3 -fopenmp

In the .expand dump we have:

(insn 242 241 243 47 j.f90:138 (set (reg:SF 74 [ D.3137 ])
         (mem/s:SF (plus:SI (reg/f:SI 247 [ .omp_data_i ])
                 (const_int 32 [0x20])) [2 .omp_data_i_55(D)->c+0 S4 
A64])) -1 (nil))
[ ... ]

(insn 247 246 248 47 j.f90:138 (set (mem/s:SF (plus:SI (reg/f:SI 247 [ 
.omp_data_i ])
                 (const_int 32 [0x20])) [13 S4 A64])
         (reg:SF 351)) -1 (nil))

As you can see we've got different alias sets on the two MEMs.   This 
could be an expansion bug, f95 bug, or a bug in one of the SSA 
optimizers.   Ugh.

Thanks,
jeff

Re: Latent bug in update_equiv_regs?

[Ian Lance Taylor]

Jeff Law <law@redhat.com> writes:

> You're right.  This should have been rejected by validate_equiv_mem,
> but isn't because the two memory references are in different alias
> sets.
>
> You can see this in the mainline sources configured for
> i686-pc-linux-gnu by compiling
> libgomp/testsuite/libgomp.fortran/reduction1.f90 with -O3 -fopenmp
>
> In the .expand dump we have:
>
> (insn 242 241 243 47 j.f90:138 (set (reg:SF 74 [ D.3137 ])
>         (mem/s:SF (plus:SI (reg/f:SI 247 [ .omp_data_i ])
>                 (const_int 32 [0x20])) [2 .omp_data_i_55(D)->c+0 S4
> A64])) -1 (nil))
> [ ... ]
>
> (insn 247 246 248 47 j.f90:138 (set (mem/s:SF (plus:SI (reg/f:SI 247 [
> .omp_data_i ])
>                 (const_int 32 [0x20])) [13 S4 A64])
>         (reg:SF 351)) -1 (nil))
>
> As you can see we've got different alias sets on the two MEMs.   This
> could be an expansion bug, f95 bug, or a bug in one of the SSA
> optimizers.   Ugh.

My first guess would be our current ooglie booglie: invalid stack slot
sharing in cfgexpand.c.

Ian

Re: Latent bug in update_equiv_regs?

[Richard Guenther]

On Thu, Aug 20, 2009 at 2:33 AM, Jeff Law<law@redhat.com> wrote:
> On 08/19/09 17:46, Ian Lance Taylor wrote:
>>
>> My understanding is that that scenario is supposed to not happen because
>> update_equiv_regs is only supposed to equate a register and a memory
>> location in the specific cases where that is OK.  It's not no_equiv that
>> is supposed to fix this, the equivalence should only be created when it
>> will always be OK.
>>
>> So I think you need to explain more about why the equivalence was
>> created.
>>
>> Ian
>>
>
> You're right.  This should have been rejected by validate_equiv_mem, but
> isn't because the two memory references are in different alias sets.
>
> You can see this in the mainline sources configured for i686-pc-linux-gnu by
> compiling libgomp/testsuite/libgomp.fortran/reduction1.f90 with -O3 -fopenmp
>
> In the .expand dump we have:
>
> (insn 242 241 243 47 j.f90:138 (set (reg:SF 74 [ D.3137 ])
>        (mem/s:SF (plus:SI (reg/f:SI 247 [ .omp_data_i ])
>                (const_int 32 [0x20])) [2 .omp_data_i_55(D)->c+0 S4 A64])) -1
> (nil))
> [ ... ]
>
> (insn 247 246 248 47 j.f90:138 (set (mem/s:SF (plus:SI (reg/f:SI 247 [
> .omp_data_i ])
>                (const_int 32 [0x20])) [13 S4 A64])
>        (reg:SF 351)) -1 (nil))
>
> As you can see we've got different alias sets on the two MEMs.   This could
> be an expansion bug, f95 bug, or a bug in one of the SSA optimizers.   Ugh.

It looks indeed bogus.  Do you have a testcase at hand?

Richard.

> Thanks,
> jeff
>
>
>
>

Re: Latent bug in update_equiv_regs?

[Jeff Law]

On 08/19/09 18:48, Ian Lance Taylor wrote:
> Jeff Law<law@redhat.com>  writes:
>
>    
>> You're right.  This should have been rejected by validate_equiv_mem,
>> but isn't because the two memory references are in different alias
>> sets.
>>
>> You can see this in the mainline sources configured for
>> i686-pc-linux-gnu by compiling
>> libgomp/testsuite/libgomp.fortran/reduction1.f90 with -O3 -fopenmp
>>
>> In the .expand dump we have:
>>
>> (insn 242 241 243 47 j.f90:138 (set (reg:SF 74 [ D.3137 ])
>>          (mem/s:SF (plus:SI (reg/f:SI 247 [ .omp_data_i ])
>>                  (const_int 32 [0x20])) [2 .omp_data_i_55(D)->c+0 S4
>> A64])) -1 (nil))
>> [ ... ]
>>
>> (insn 247 246 248 47 j.f90:138 (set (mem/s:SF (plus:SI (reg/f:SI 247 [
>> .omp_data_i ])
>>                  (const_int 32 [0x20])) [13 S4 A64])
>>          (reg:SF 351)) -1 (nil))
>>
>> As you can see we've got different alias sets on the two MEMs.   This
>> could be an expansion bug, f95 bug, or a bug in one of the SSA
>> optimizers.   Ugh.
>>      
> My first guess would be our current ooglie booglie: invalid stack slot
> sharing in cfgexpand.c.
>    
I don't think that's the case.  I hacked up partition_stack_vars to not 
share anything and still get the same failure.

Jeff

Re: Latent bug in update_equiv_regs?

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On 08/20/09 02:45, Richard Guenther wrote:
>
> It looks indeed bogus.  Do you have a testcase at hand?
>    
Compile the attached testcase with -O3 -mopenmp on i686-pc-linux-gnu.  
Find MAIN__.omp_fn.2 in the .expand dump.

Within that function, you're looking for this sequence of insns:



;; D.3137_29 = REALPART_EXPR <.omp_data_i_55(D)->c>;

(insn 242 241 0 j.f90:138 (set (reg:SF 74 [ D.3137 ])
         (mem/s:SF (plus:SI (reg/f:SI 247 [ .omp_data_i ])
                 (const_int 32 [0x20])) [2 .omp_data_i_55(D)->c+0 S4 
A64])) -1 (nil))

[ ... ]

(insn 247 246 0 j.f90:138 (set (mem/s:SF (plus:SI (reg/f:SI 247 [ 
.omp_data_i ])
                 (const_int 32 [0x20])) [13 S4 A64])
         (reg:SF 351)) -1 (nil))


This doesn't cause a libgomp testsuite failure in the mainline because 
(reg:SF 74) gets a hard reg and thus reload doesn't utilize the bogus 
REG_EQUIV note that eventually gets added to insn 242.

Jeff

[-- Attachment #2: reduction1.f90 --]
[-- Type: text/plain, Size: 4309 bytes --]

! { dg-do run }
!$ use omp_lib

  integer :: i, ia (6), n, cnt
  real :: r, ra (4)
  double precision :: d, da (5)
  complex :: c, ca (3)
  logical :: v

  i = 1
  ia = 2
  r = 3
  ra = 4
  d = 5.5
  da = 6.5
  c = cmplx (7.5, 1.5)
  ca = cmplx (8.5, -3.0)
  v = .false.
  cnt = -1

!$omp parallel num_threads (3) private (n) reduction (.or.:v) &
!$omp & reduction (+:i, ia, r, ra, d, da, c, ca)
!$ if (i .ne. 0 .or. any (ia .ne. 0)) v = .true.
!$ if (r .ne. 0 .or. any (ra .ne. 0)) v = .true.
!$ if (d .ne. 0 .or. any (da .ne. 0)) v = .true.
!$ if (c .ne. cmplx (0) .or. any (ca .ne. cmplx (0))) v = .true.
  n = omp_get_thread_num ()
  if (n .eq. 0) then
    cnt = omp_get_num_threads ()
    i = 4
    ia(3:5) = -2
    r = 5
    ra(1:2) = 6.5
    d = -2.5
    da(2:4) = 8.5
    c = cmplx (2.5, -3.5)
    ca(1) = cmplx (4.5, 5)
  else if (n .eq. 1) then
    i = 2
    ia(4:6) = 5
    r = 1
    ra(2:4) = -1.5
    d = 8.5
    da(1:3) = 2.5
    c = cmplx (0.5, -3)
    ca(2:3) = cmplx (-1, 6)
  else
    i = 1
    ia = 1
    r = -1
    ra = -1
    d = 1
    da = -1
    c = 1
    ca = cmplx (-1, 0)
  end if
!$omp end parallel
  if (v) call abort
  if (cnt .eq. 3) then
    if (i .ne. 8 .or. any (ia .ne. (/3, 3, 1, 6, 6, 8/))) call abort
    if (r .ne. 8 .or. any (ra .ne. (/9.5, 8.0, 1.5, 1.5/))) call abort
    if (d .ne. 12.5 .or. any (da .ne. (/8.0, 16.5, 16.5, 14.0, 5.5/))) call abort
    if (c .ne. cmplx (11.5, -5)) call abort
    if (ca(1) .ne. cmplx (12, 2)) call abort
    if (ca(2) .ne. cmplx (6.5, 3) .or. ca(2) .ne. ca(3)) call abort
  end if

  i = 1
  ia = 2
  r = 3
  ra = 4
  d = 5.5
  da = 6.5
  c = cmplx (7.5, 1.5)
  ca = cmplx (8.5, -3.0)
  v = .false.
  cnt = -1

!$omp parallel num_threads (3) private (n) reduction (.or.:v) &
!$omp & reduction (-:i, ia, r, ra, d, da, c, ca)
!$ if (i .ne. 0 .or. any (ia .ne. 0)) v = .true.
!$ if (r .ne. 0 .or. any (ra .ne. 0)) v = .true.
!$ if (d .ne. 0 .or. any (da .ne. 0)) v = .true.
!$ if (c .ne. cmplx (0) .or. any (ca .ne. cmplx (0))) v = .true.
  n = omp_get_thread_num ()
  if (n .eq. 0) then
    cnt = omp_get_num_threads ()
    i = 4
    ia(3:5) = -2
    r = 5
    ra(1:2) = 6.5
    d = -2.5
    da(2:4) = 8.5
    c = cmplx (2.5, -3.5)
    ca(1) = cmplx (4.5, 5)
  else if (n .eq. 1) then
    i = 2
    ia(4:6) = 5
    r = 1
    ra(2:4) = -1.5
    d = 8.5
    da(1:3) = 2.5
    c = cmplx (0.5, -3)
    ca(2:3) = cmplx (-1, 6)
  else
    i = 1
    ia = 1
    r = -1
    ra = -1
    d = 1
    da = -1
    c = 1
    ca = cmplx (-1, 0)
  end if
!$omp end parallel
  if (v) call abort
  if (cnt .eq. 3) then
    if (i .ne. 8 .or. any (ia .ne. (/3, 3, 1, 6, 6, 8/))) call abort
    if (r .ne. 8 .or. any (ra .ne. (/9.5, 8.0, 1.5, 1.5/))) call abort
    if (d .ne. 12.5 .or. any (da .ne. (/8.0, 16.5, 16.5, 14.0, 5.5/))) call abort
    if (c .ne. cmplx (11.5, -5)) call abort
    if (ca(1) .ne. cmplx (12, 2)) call abort
    if (ca(2) .ne. cmplx (6.5, 3) .or. ca(2) .ne. ca(3)) call abort
  end if

  i = 1
  ia = 2
  r = 4
  ra = 8
  d = 16
  da = 32
  c = 2
  ca = cmplx (0, 2)
  v = .false.
  cnt = -1

!$omp parallel num_threads (3) private (n) reduction (.or.:v) &
!$omp & reduction (*:i, ia, r, ra, d, da, c, ca)
!$ if (i .ne. 1 .or. any (ia .ne. 1)) v = .true.
!$ if (r .ne. 1 .or. any (ra .ne. 1)) v = .true.
!$ if (d .ne. 1 .or. any (da .ne. 1)) v = .true.
!$ if (c .ne. cmplx (1) .or. any (ca .ne. cmplx (1))) v = .true.
  n = omp_get_thread_num ()
  if (n .eq. 0) then
    cnt = omp_get_num_threads ()
    i = 3
    ia(3:5) = 2
    r = 0.5
    ra(1:2) = 2
    d = -1
    da(2:4) = -2
    c = 2.5
    ca(1) = cmplx (-5, 0)
  else if (n .eq. 1) then
    i = 2
    ia(4:6) = -2
    r = 8
    ra(2:4) = -0.5
    da(1:3) = -1
    c = -3
    ca(2:3) = cmplx (0, -1)
  else
    ia = 2
    r = 0.5
    ra = 0.25
    d = 2.5
    da = -1
    c = cmplx (0, -1)
    ca = cmplx (-1, 0)
  end if
!$omp end parallel
  if (v) call abort
  if (cnt .eq. 3) then
    if (i .ne. 6 .or. any (ia .ne. (/4, 4, 8, -16, -16, -8/))) call abort
    if (r .ne. 8 .or. any (ra .ne. (/4., -2., -1., -1./))) call abort
    if (d .ne. -40 .or. any (da .ne. (/32., -64., -64., 64., -32./))) call abort
    if (c .ne. cmplx (0, 15)) call abort
    if (ca(1) .ne. cmplx (0, 10)) call abort
    if (ca(2) .ne. cmplx (-2, 0) .or. ca(2) .ne. ca(3)) call abort
  end if
end

Re: Latent bug in update_equiv_regs?

[Richard Guenther]

On Thu, Aug 20, 2009 at 3:37 PM, Jeff Law<law@redhat.com> wrote:
> On 08/20/09 02:45, Richard Guenther wrote:
>>
>> It looks indeed bogus.  Do you have a testcase at hand?
>>
>
> Compile the attached testcase with -O3 -mopenmp on i686-pc-linux-gnu.  Find
> MAIN__.omp_fn.2 in the .expand dump.
>
> Within that function, you're looking for this sequence of insns:
>
>
>
> ;; D.3137_29 = REALPART_EXPR <.omp_data_i_55(D)->c>;
>
> (insn 242 241 0 j.f90:138 (set (reg:SF 74 [ D.3137 ])
>        (mem/s:SF (plus:SI (reg/f:SI 247 [ .omp_data_i ])
>                (const_int 32 [0x20])) [2 .omp_data_i_55(D)->c+0 S4 A64])) -1
> (nil))
>
> [ ... ]
>
> (insn 247 246 0 j.f90:138 (set (mem/s:SF (plus:SI (reg/f:SI 247 [
> .omp_data_i ])
>                (const_int 32 [0x20])) [13 S4 A64])
>        (reg:SF 351)) -1 (nil))
>
>
> This doesn't cause a libgomp testsuite failure in the mainline because
> (reg:SF 74) gets a hard reg and thus reload doesn't utilize the bogus
> REG_EQUIV note that eventually gets added to insn 242.

I suppose we are using the complex type alias-set at one place and
the component at another.  My suggestion would be to handle
complex types like we do array and vector tyoes in get_alias_set,
thus assign the component alias set to the complex types.

Richard.

> Jeff
>