diff mbox series

tree-optimization/101280 - revise interchange fix for PR101173

Message ID n946306n-o821-668q-66q-96p1qs8533@fhfr.qr
State New
Headers show
Series tree-optimization/101280 - revise interchange fix for PR101173 | expand

Commit Message

Richard Biener July 1, 2021, 11:38 a.m. UTC 
The following revises the original fix for PR101173 to correctly
check for a reversed dependence rather than disallowing a zero
distance.  It also adds a check from TSVC which asks for this
kind of interchange (but with a valid dependence).

Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.

2021-07-01  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/101280
	PR tree-optimization/101173
	* gimple-loop-interchange.cc
	(tree_loop_interchange::valid_data_dependences): Revert
	previous change and instead correctly handle DDR_REVERSED_P
	dependence.

	* gcc.dg/tree-ssa/loop-interchange-16.c: New testcase.
---
 gcc/gimple-loop-interchange.cc                |  7 ++++--
 .../gcc.dg/tree-ssa/loop-interchange-16.c     | 22 +++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-16.c

Comments

Michael Matz July 1, 2021, 2:49 p.m. UTC | #1 
Hello,

On Thu, 1 Jul 2021, Richard Biener wrote:

> diff --git a/gcc/gimple-loop-interchange.cc b/gcc/gimple-loop-interchange.cc
> index 43045c5455e..43ef112a2d0 100644
> --- a/gcc/gimple-loop-interchange.cc
> +++ b/gcc/gimple-loop-interchange.cc
> @@ -1043,8 +1043,11 @@ tree_loop_interchange::valid_data_dependences (unsigned i_idx, unsigned o_idx,
>  	    continue;
>  
>  	  /* Be conservative, skip case if either direction at i_idx/o_idx
> -	     levels is not '=' (for the inner loop) or '<'.  */
> -	  if (dist_vect[i_idx] < 0 || dist_vect[o_idx] <= 0)
> +	     levels is not '=' or '<'.  */
> +	  if (dist_vect[i_idx] < 0
> +	      || (DDR_REVERSED_P (ddr) && dist_vect[i_idx] > 0)
> +	      || dist_vect[o_idx] < 0
> +	      || (DDR_REVERSED_P (ddr) && dist_vect[o_idx] > 0))

Hmm, if DDR_REVERSED_P matters here, then it should matter for all arms.  
IOW: < 0 should be tested only when !DDR_REVERSED_P, not always:

       if ((!DDR_REVERSED_P (ddr) && dist_vect[i_idx] < 0)
           || (DDR_REVERSED_P (ddr) && dist_vect[i_idx] > 0)
           || (!DDR_REVERSED_P (ddr) && dist_vect[o_idx] < 0)
           || (DDR_REVERSED_P (ddr) && dist_vect[o_idx] > 0))

(what you have effectively written is a condition that allows only 0 when 
DDR_REVERSED_P)


Ciao,
Michael.
Richard Biener July 2, 2021, 6:03 a.m. UTC | #2 
On Thu, 1 Jul 2021, Michael Matz wrote:

> Hello,
> 
> On Thu, 1 Jul 2021, Richard Biener wrote:
> 
> > diff --git a/gcc/gimple-loop-interchange.cc b/gcc/gimple-loop-interchange.cc
> > index 43045c5455e..43ef112a2d0 100644
> > --- a/gcc/gimple-loop-interchange.cc
> > +++ b/gcc/gimple-loop-interchange.cc
> > @@ -1043,8 +1043,11 @@ tree_loop_interchange::valid_data_dependences (unsigned i_idx, unsigned o_idx,
> >  	    continue;
> >  
> >  	  /* Be conservative, skip case if either direction at i_idx/o_idx
> > -	     levels is not '=' (for the inner loop) or '<'.  */
> > -	  if (dist_vect[i_idx] < 0 || dist_vect[o_idx] <= 0)
> > +	     levels is not '=' or '<'.  */
> > +	  if (dist_vect[i_idx] < 0
> > +	      || (DDR_REVERSED_P (ddr) && dist_vect[i_idx] > 0)
> > +	      || dist_vect[o_idx] < 0
> > +	      || (DDR_REVERSED_P (ddr) && dist_vect[o_idx] > 0))
> 
> Hmm, if DDR_REVERSED_P matters here, then it should matter for all arms.  
> IOW: < 0 should be tested only when !DDR_REVERSED_P, not always:
> 
>        if ((!DDR_REVERSED_P (ddr) && dist_vect[i_idx] < 0)
>            || (DDR_REVERSED_P (ddr) && dist_vect[i_idx] > 0)
>            || (!DDR_REVERSED_P (ddr) && dist_vect[o_idx] < 0)
>            || (DDR_REVERSED_P (ddr) && dist_vect[o_idx] > 0))
> 
> (what you have effectively written is a condition that allows only 0 when 
> DDR_REVERSED_P)

True - been fixing bugs too fast I guess.  Maybe the 3rd time's the charm 
then ;)

Richard.
Richard Biener July 2, 2021, 6:54 a.m. UTC | #3 
On Fri, 2 Jul 2021, Richard Biener wrote:

> On Thu, 1 Jul 2021, Michael Matz wrote:
> 
> > Hello,
> > 
> > On Thu, 1 Jul 2021, Richard Biener wrote:
> > 
> > > diff --git a/gcc/gimple-loop-interchange.cc b/gcc/gimple-loop-interchange.cc
> > > index 43045c5455e..43ef112a2d0 100644
> > > --- a/gcc/gimple-loop-interchange.cc
> > > +++ b/gcc/gimple-loop-interchange.cc
> > > @@ -1043,8 +1043,11 @@ tree_loop_interchange::valid_data_dependences (unsigned i_idx, unsigned o_idx,
> > >  	    continue;
> > >  
> > >  	  /* Be conservative, skip case if either direction at i_idx/o_idx
> > > -	     levels is not '=' (for the inner loop) or '<'.  */
> > > -	  if (dist_vect[i_idx] < 0 || dist_vect[o_idx] <= 0)
> > > +	     levels is not '=' or '<'.  */
> > > +	  if (dist_vect[i_idx] < 0
> > > +	      || (DDR_REVERSED_P (ddr) && dist_vect[i_idx] > 0)
> > > +	      || dist_vect[o_idx] < 0
> > > +	      || (DDR_REVERSED_P (ddr) && dist_vect[o_idx] > 0))
> > 
> > Hmm, if DDR_REVERSED_P matters here, then it should matter for all arms.  
> > IOW: < 0 should be tested only when !DDR_REVERSED_P, not always:
> > 
> >        if ((!DDR_REVERSED_P (ddr) && dist_vect[i_idx] < 0)
> >            || (DDR_REVERSED_P (ddr) && dist_vect[i_idx] > 0)
> >            || (!DDR_REVERSED_P (ddr) && dist_vect[o_idx] < 0)
> >            || (DDR_REVERSED_P (ddr) && dist_vect[o_idx] > 0))
> > 
> > (what you have effectively written is a condition that allows only 0 when 
> > DDR_REVERSED_P)
> 
> True - been fixing bugs too fast I guess.  Maybe the 3rd time's the charm 
> then ;)

Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.

Richard.

From 094af28137de5c55a21123c3b4e06e7d7a08bb5a Mon Sep 17 00:00:00 2001
From: Richard Biener <rguenther@suse.de>
Date: Fri, 2 Jul 2021 08:51:43 +0200
Subject: [PATCH] tree-optimization/101280 - re-revise interchange fix for
 PR101173
To: gcc-patches@gcc.gnu.org

The following fixes up the revision of the original fix for PR101173
to properly guard all dependence checks with DDR_REVERSED_P or its
inverse.

2021-07-01  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/101280
	PR tree-optimization/101173
	* gimple-loop-interchange.cc
	(tree_loop_interchange::valid_data_dependences): Properly
	guard all dependence checks with DDR_REVERSED_P or its
	inverse.
---
 gcc/gimple-loop-interchange.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/gimple-loop-interchange.cc b/gcc/gimple-loop-interchange.cc
index 43ef112a2d0..7a88faa2c07 100644
--- a/gcc/gimple-loop-interchange.cc
+++ b/gcc/gimple-loop-interchange.cc
@@ -1044,9 +1044,9 @@ tree_loop_interchange::valid_data_dependences (unsigned i_idx, unsigned o_idx,
 
 	  /* Be conservative, skip case if either direction at i_idx/o_idx
 	     levels is not '=' or '<'.  */
-	  if (dist_vect[i_idx] < 0
+	  if ((!DDR_REVERSED_P (ddr) && dist_vect[i_idx] < 0)
 	      || (DDR_REVERSED_P (ddr) && dist_vect[i_idx] > 0)
-	      || dist_vect[o_idx] < 0
+	      || (!DDR_REVERSED_P (ddr) && dist_vect[o_idx] < 0)
 	      || (DDR_REVERSED_P (ddr) && dist_vect[o_idx] > 0))
 	    return false;
 	}
diff mbox series

Patch

diff --git a/gcc/gimple-loop-interchange.cc b/gcc/gimple-loop-interchange.cc
index 43045c5455e..43ef112a2d0 100644
--- a/gcc/gimple-loop-interchange.cc
+++ b/gcc/gimple-loop-interchange.cc
@@ -1043,8 +1043,11 @@  tree_loop_interchange::valid_data_dependences (unsigned i_idx, unsigned o_idx,
 	    continue;
 
 	  /* Be conservative, skip case if either direction at i_idx/o_idx
-	     levels is not '=' (for the inner loop) or '<'.  */
-	  if (dist_vect[i_idx] < 0 || dist_vect[o_idx] <= 0)
+	     levels is not '=' or '<'.  */
+	  if (dist_vect[i_idx] < 0
+	      || (DDR_REVERSED_P (ddr) && dist_vect[i_idx] > 0)
+	      || dist_vect[o_idx] < 0
+	      || (DDR_REVERSED_P (ddr) && dist_vect[o_idx] > 0))
 	    return false;
 	}
     }
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-16.c b/gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-16.c
new file mode 100644
index 00000000000..781555e085d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/loop-interchange-16.c
@@ -0,0 +1,22 @@ 
+/* PR/101280 */
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-tree-linterchange-details" } */
+
+void dummy (double *, double *);
+#define LEN_2D 32
+double aa[LEN_2D][LEN_2D], bb[LEN_2D][LEN_2D];
+double s231(int iterations)
+{
+//    loop interchange
+//    loop with data dependency
+    for (int nl = 0; nl < 100*(iterations/LEN_2D); nl++) {
+        for (int i = 0; i < LEN_2D; ++i) {
+            for (int j = 1; j < LEN_2D; j++) {
+                aa[j][i] = aa[j - 1][i] + bb[j][i];
+            }
+        }
+        dummy(aa[0],bb[0]);
+    }
+}
+
+/* { dg-final { scan-tree-dump "loops interchanged" "linterchange" } } */