Re: Fix PR90796

2019-10-28 Thread Michael Matz
Hello,

On Mon, 28 Oct 2019, Rainer Orth wrote:

> >> > +FAIL: gcc.dg/unroll-and-jam.c scan-tree-dump-times unrolljam "applying
> >> > unroll and jam" 6
> >> 
> >> Hrmpf, I'll have a look :-/  Thanks for noticing.
> >
> > A strange interaction with LIM, which only does something on 32bit, but 
> > not on 64bit (which is a problem to investigate on its own, but outside of 
> > scope).  Disabling it requires changing the testcase so that LIM isn't 
> > necessary for other reasons, as below.  Can you check if that fixes the 
> > problem on Solaris as well?  (It does for linux 32bit).
> 
> I just checked: the test now PASSes on both sparc-sun-solaris2.11 and
> i386-pc-solaris2.11 (32 and 64-bit each).

Thank you.  Committed as r277508 now.


Ciao,
Michael.


Re: Fix PR90796

2019-10-28 Thread Rainer Orth
Hi Michael,

>> On Tue, 22 Oct 2019, Rainer Orth wrote:
>> 
>> > > testsuite/
>> > >  * gcc.dg/unroll-and-jam.c: Add three invalid and one valid case.
>> > 
>> > this testcase now FAILs on 32-bit targets (seen on i386-pc-solaris2.11
>> > and sparc-sun-solaris2.11, also reports for i686-pc-linux-gnu and
>> > i586-unknown-freebsd11.2):
>> > 
>> > +FAIL: gcc.dg/unroll-and-jam.c scan-tree-dump-times unrolljam "applying
>> > unroll and jam" 6
>> 
>> Hrmpf, I'll have a look :-/  Thanks for noticing.
>
> A strange interaction with LIM, which only does something on 32bit, but 
> not on 64bit (which is a problem to investigate on its own, but outside of 
> scope).  Disabling it requires changing the testcase so that LIM isn't 
> necessary for other reasons, as below.  Can you check if that fixes the 
> problem on Solaris as well?  (It does for linux 32bit).

I just checked: the test now PASSes on both sparc-sun-solaris2.11 and
i386-pc-solaris2.11 (32 and 64-bit each).

Thanks.
Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University


Re: Fix PR90796

2019-10-28 Thread Michael Matz
Hi,

On Wed, 23 Oct 2019, Michael Matz wrote:

> On Tue, 22 Oct 2019, Rainer Orth wrote:
> 
> > > testsuite/
> > >   * gcc.dg/unroll-and-jam.c: Add three invalid and one valid case.
> > 
> > this testcase now FAILs on 32-bit targets (seen on i386-pc-solaris2.11
> > and sparc-sun-solaris2.11, also reports for i686-pc-linux-gnu and
> > i586-unknown-freebsd11.2):
> > 
> > +FAIL: gcc.dg/unroll-and-jam.c scan-tree-dump-times unrolljam "applying 
> > unroll and jam" 6
> 
> Hrmpf, I'll have a look :-/  Thanks for noticing.

A strange interaction with LIM, which only does something on 32bit, but 
not on 64bit (which is a problem to investigate on its own, but outside of 
scope).  Disabling it requires changing the testcase so that LIM isn't 
necessary for other reasons, as below.  Can you check if that fixes the 
problem on Solaris as well?  (It does for linux 32bit).


Ciao,
Michael.

testsuite/
PR middle-end/90796
* gcc.dg/unroll-and-jam.c: Disable loop-invariant motion.

--- a/gcc/testsuite/gcc.dg/unroll-and-jam.c
+++ b/gcc/testsuite/gcc.dg/unroll-and-jam.c
@@ -1,5 +1,5 @@
 /* { dg-do run } */
-/* { dg-options "-O3 -floop-unroll-and-jam --param unroll-jam-min-percent=0 
-fdump-tree-unrolljam-details" } */
+/* { dg-options "-O3 -floop-unroll-and-jam -fno-tree-loop-im --param 
unroll-jam-min-percent=0 -fdump-tree-unrolljam-details" } */
 /* { dg-require-effective-target int32plus } */
 
 #include 
@@ -31,10 +31,10 @@ void checkb(void)
   //printf("  %d\n", sum);
 }
 
-unsigned i, j;
 #define TEST(name, body, test) \
 static void __attribute__((noinline,noclone)) name (unsigned long n, unsigned 
long m) \
 { \
+  unsigned i, j; \
   for (i = 1; i < m; i++) { \
   for (j = 1; j < n; j++) { \
  body; \


Re: Fix PR90796

2019-10-23 Thread Michael Matz
Hello,

On Tue, 22 Oct 2019, Rainer Orth wrote:

> > testsuite/
> > * gcc.dg/unroll-and-jam.c: Add three invalid and one valid case.
> 
> this testcase now FAILs on 32-bit targets (seen on i386-pc-solaris2.11
> and sparc-sun-solaris2.11, also reports for i686-pc-linux-gnu and
> i586-unknown-freebsd11.2):
> 
> +FAIL: gcc.dg/unroll-and-jam.c scan-tree-dump-times unrolljam "applying 
> unroll and jam" 6

Hrmpf, I'll have a look :-/  Thanks for noticing.


Ciao,
Michael.


Re: Fix PR90796

2019-10-22 Thread Rainer Orth
Hi Michael,

> this was still collecting dust on my disk, so here it is.  See the 
> extensive comment in the patch for what happens, in short invariant IVs 
> are affine but still have to be handled more conservative than other 
> affine IVs in transformations that reorder instructions.  Making our 
> dependence analysis more conservative instead would be too much, we 
> wouldn't be able to handle cases that we should handle anymore.
>
> Regstrapped on x86-64-linux without regressions (all languages+Ada).
>
>
> Ciao,
> Michael.
>
>   PR middle-end/90796
>   * gimple-loop-jam.c (any_access_function_variant_p): New function.
>   (adjust_unroll_factor): Use it to constrain safety, new parameter.
>   (tree_loop_unroll_and_jam): Adjust call and profitable unroll factor.
>
> testsuite/
>   * gcc.dg/unroll-and-jam.c: Add three invalid and one valid case.

this testcase now FAILs on 32-bit targets (seen on i386-pc-solaris2.11
and sparc-sun-solaris2.11, also reports for i686-pc-linux-gnu and
i586-unknown-freebsd11.2):

+FAIL: gcc.dg/unroll-and-jam.c scan-tree-dump-times unrolljam "applying unroll 
and jam" 6

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University


Re: Fix PR90796

2019-10-22 Thread Richard Biener
On Tue, Oct 22, 2019 at 11:36 AM Michael Matz  wrote:
>
> Hi,
>
> this was still collecting dust on my disk, so here it is.  See the
> extensive comment in the patch for what happens, in short invariant IVs
> are affine but still have to be handled more conservative than other
> affine IVs in transformations that reorder instructions.  Making our
> dependence analysis more conservative instead would be too much, we
> wouldn't be able to handle cases that we should handle anymore.
>
> Regstrapped on x86-64-linux without regressions (all languages+Ada).

OK for trunk and affected branches.

Thanks,
Richard.

>
> Ciao,
> Michael.
>
> PR middle-end/90796
> * gimple-loop-jam.c (any_access_function_variant_p): New function.
> (adjust_unroll_factor): Use it to constrain safety, new parameter.
> (tree_loop_unroll_and_jam): Adjust call and profitable unroll factor.
>
> testsuite/
> * gcc.dg/unroll-and-jam.c: Add three invalid and one valid case.
>
> diff --git a/gcc/gimple-loop-jam.c b/gcc/gimple-loop-jam.c
> index 11153f5..899653b 100644
> --- a/gcc/gimple-loop-jam.c
> +++ b/gcc/gimple-loop-jam.c
> @@ -360,16 +360,33 @@ fuse_loops (class loop *loop)
>rewrite_into_loop_closed_ssa_1 (NULL, 0, SSA_OP_USE, loop);
>  }
>
> +/* Return true if any of the access functions for dataref A
> +   isn't invariant with respect to loop LOOP_NEST.  */
> +static bool
> +any_access_function_variant_p (const struct data_reference *a,
> +  const class loop *loop_nest)
> +{
> +  unsigned int i;
> +  vec fns = DR_ACCESS_FNS (a);
> +  tree t;
> +
> +  FOR_EACH_VEC_ELT (fns, i, t)
> +if (!evolution_function_is_invariant_p (t, loop_nest->num))
> +  return true;
> +
> +  return false;
> +}
> +
>  /* Returns true if the distance in DDR can be determined and adjusts
> the unroll factor in *UNROLL to make unrolling valid for that distance.
> -   Otherwise return false.
> +   Otherwise return false.  DDR is with respect to the outer loop of INNER.
>
> If this data dep can lead to a removed memory reference, increment
> *REMOVED and adjust *PROFIT_UNROLL to be the necessary unroll factor
> for this to happen.  */
>
>  static bool
> -adjust_unroll_factor (struct data_dependence_relation *ddr,
> +adjust_unroll_factor (class loop *inner, struct data_dependence_relation 
> *ddr,
>   unsigned *unroll, unsigned *profit_unroll,
>   unsigned *removed)
>  {
> @@ -392,9 +409,59 @@ adjust_unroll_factor (struct data_dependence_relation 
> *ddr,
> gcc_unreachable ();
>   else if ((unsigned)dist >= *unroll)
> ;
> - else if (lambda_vector_lexico_pos (dist_v + 1, DDR_NB_LOOPS (ddr) - 
> 1)
> -  || (lambda_vector_zerop (dist_v + 1, DDR_NB_LOOPS (ddr) - 
> 1)
> -  && dist > 0))
> + else if (lambda_vector_zerop (dist_v + 1, DDR_NB_LOOPS (ddr) - 1))
> +   {
> + /* We have (a,0) with a < N, so this will be transformed into
> +(0,0) after unrolling by N.  This might potentially be a
> +problem, if it's not a read-read dependency.  */
> + if (DR_IS_READ (DDR_A (ddr)) && DR_IS_READ (DDR_B (ddr)))
> +   ;
> + else
> +   {
> + /* So, at least one is a write, and we might reduce the
> +distance vector to (0,0).  This is still no problem
> +if both data-refs are affine with respect to the inner
> +loops.  But if one of them is invariant with respect
> +to an inner loop our reordering implicit in loop fusion
> +corrupts the program, as our data dependences don't
> +capture this.  E.g. for:
> +  for (0 <= i < n)
> +for (0 <= j < m)
> +  a[i][0] = a[i+1][0] + 2;// (1)
> +  b[i][j] = b[i+1][j] + 2;// (2)
> +the distance vector for both statements is (-1,0),
> +but exchanging the order for (2) is okay, while
> +for (1) it is not.  To see this, write out the original
> +accesses (assume m is 2):
> +  a i j original
> +  0 0 0 r a[1][0] b[1][0]
> +  1 0 0 w a[0][0] b[0][0]
> +  2 0 1 r a[1][0] b[1][1]
> +  3 0 1 w a[0][0] b[0][1]
> +  4 1 0 r a[2][0] b[2][0]
> +  5 1 0 w a[1][0] b[1][0]
> +after unroll-by-2 and fusion the accesses are done in
> +this order (from column a): 0,1, 4,5, 2,3, i.e. this:
> +  a i j transformed
> +  0 0 0 r a[1][0] b[1][0]
> +  1 0 0 w a[0][0] b[0][0]
> +  4 1 0 r a[2][0] b[