diff mbox series

[fortran] Improve dependency checking

Message ID a4fe61bf-8a6e-1f29-889b-a0304015c74d@netcologne.de
State New
Headers show
Series [fortran] Improve dependency checking | expand

Commit Message

Thomas Koenig July 25, 2019, 12:57 p.m. UTC 
Hello world,

the attached pach does some more work in gfc_check_dependency for
the case where an identity between arguments would also lead
to problems.

It does not lead to removal of the warning with -Warray-temporaries,
because that is still generated by the call to library function.
Instead, I checked for the names of the variables which used to
be introduced by the matmul inlining code.

Regression-tested. OK for trunk?

Regards

	Thomas

2019-07-25  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/65819
	* dependency.h (gfc_dep_resovler): Add optional argument identical.
	* dependency.c (gfc_check_dependency): Do not alway return 1 if
	the symbol is the same. Pass on identical to gfc_dep_resolver.
	(gfc_check_element_vs_element): Whitespace fix.
	(gfc_dep_resolver): Adjust comment for function.  If identical is
	true, return 1 if any overlap has been found.

2019-07-25  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/65819
	* gfortran.dg/dependency_54.f90: New test.

Comments

Steve Kargl July 25, 2019, 2:24 p.m. UTC | #1 
On Thu, Jul 25, 2019 at 02:57:35PM +0200, Thomas Koenig wrote:
> Index: dependency.h
> ===================================================================
> --- dependency.h	(Revision 273733)
> +++ dependency.h	(Arbeitskopie)
> @@ -37,7 +37,7 @@ int gfc_check_fncall_dependency (gfc_expr *, sym_i
>  int gfc_check_dependency (gfc_expr *, gfc_expr *, bool);
>  int gfc_expr_is_one (gfc_expr *, int);
>  
> -int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *);
> +int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *, bool identical = false);

This is changing the prototype.  I would expect to see


int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *, bool);
Thomas Koenig July 25, 2019, 2:42 p.m. UTC | #2 
Hi Steve,

>> -int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *);
>> +int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *, bool identical = false);
> This is changing the prototype.  I would expect to see
> 
> 
> int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *, bool);

Usig C++'s optional arguments is actually quite useful, it's used
already used in a few places in the front end.

The idea is that you don't need to touch the other callers, just the
ones where the new argument matters.

However, in this particular case, it would also be possible to ajust
all other callers (exactly one), if you prefer.

Regards

	Thomas
Steve Kargl July 25, 2019, 3:59 p.m. UTC | #3 
On Thu, Jul 25, 2019 at 04:42:44PM +0200, Thomas Koenig wrote:
> Hi Steve,
> 
> >> -int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *);
> >> +int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *, bool identical = false);
> > This is changing the prototype.  I would expect to see
> > 
> > 
> > int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *, bool);
> 
> Usig C++'s optional arguments is actually quite useful, it's used
> already used in a few places in the front end.
> 
> The idea is that you don't need to touch the other callers, just the
> ones where the new argument matters.
> 
> However, in this particular case, it would also be possible to ajust
> all other callers (exactly one), if you prefer.
> 

Ah, I don't speak C++, and didn't know one could corrupt a
C prototype in this manner.  A quick glance of gfortran.h
indeed shows a few more occurences of "bool xxx = false".
I suppose the patch is then OK.

PS: watch for long lines.
Thomas Koenig July 25, 2019, 4:26 p.m. UTC | #4 
Hi Steve,

> Ah, I don't speak C++, and didn't know one could corrupt a
> C prototype in this manner.  A quick glance of gfortran.h
> indeed shows a few more occurences of "bool xxx = false".
> I suppose the patch is then OK.

I don't use many C++ features, but I use this one because I feel
it should really be in C :-)

> PS: watch for long lines.

Corrected and committed as r273807.

Thanks for the review!

Regards

	Thomas
diff mbox series

Patch

Index: dependency.c
===================================================================
--- dependency.c	(Revision 273733)
+++ dependency.c	(Arbeitskopie)
@@ -1351,13 +1351,10 @@  gfc_check_dependency (gfc_expr *expr1, gfc_expr *e
 	  return 0;
 	}
 
-      if (identical)
-	return 1;
-
       /* Identical and disjoint ranges return 0,
 	 overlapping ranges return 1.  */
       if (expr1->ref && expr2->ref)
-	return gfc_dep_resolver (expr1->ref, expr2->ref, NULL);
+	return gfc_dep_resolver (expr1->ref, expr2->ref, NULL, identical);
 
       return 1;
 
@@ -1884,6 +1881,7 @@  gfc_check_element_vs_element (gfc_ref *lref, gfc_r
 
   if (i > -2)
     return GFC_DEP_NODEP;
+ 
   return GFC_DEP_EQUAL;
 }
 
@@ -2084,13 +2082,15 @@  ref_same_as_full_array (gfc_ref *full_ref, gfc_ref
 
 /* Finds if two array references are overlapping or not.
    Return value
-   	2 : array references are overlapping but reversal of one or
+	2 : array references are overlapping but reversal of one or
 	    more dimensions will clear the dependency.
-   	1 : array references are overlapping.
-   	0 : array references are identical or not overlapping.  */
+	1 : array references are overlapping, or identical is true and
+	    there is some kind of overlap.
+	0 : array references are identical or not overlapping.  */
 
 int
-gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse)
+gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_reverse *reverse,
+		  bool identical)
 {
   int n;
   int m;
@@ -2124,11 +2124,15 @@  int
 
 	case REF_ARRAY:
 
+	  /* For now, treat all coarrays as dangerous.  */
+	  if (lref->u.ar.codimen || rref->u.ar.codimen)
+	    return 1;
+
 	  if (ref_same_as_full_array (lref, rref))
-	    return 0;
+	    return identical;
 
 	  if (ref_same_as_full_array (rref, lref))
-	    return 0;
+	    return identical;
 
 	  if (lref->u.ar.dimen != rref->u.ar.dimen)
 	    {
@@ -2180,6 +2184,8 @@  int
 		  gcc_assert (rref->u.ar.dimen_type[n] == DIMEN_ELEMENT
 			      && lref->u.ar.dimen_type[n] == DIMEN_ELEMENT);
 		  this_dep = gfc_check_element_vs_element (rref, lref, n);
+		  if (identical && this_dep == GFC_DEP_EQUAL)
+		    this_dep = GFC_DEP_OVERLAP;
 		}
 
 	      /* If any dimension doesn't overlap, we have no dependency.  */
@@ -2240,6 +2246,9 @@  int
 		 know the worst one.*/
 
 	    update_fin_dep:
+	      if (identical && this_dep == GFC_DEP_EQUAL)
+		this_dep = GFC_DEP_OVERLAP;
+
 	      if (this_dep > fin_dep)
 		fin_dep = this_dep;
 	    }
@@ -2253,7 +2262,7 @@  int
 
 	  /* Exactly matching and forward overlapping ranges don't cause a
 	     dependency.  */
-	  if (fin_dep < GFC_DEP_BACKWARD)
+	  if (fin_dep < GFC_DEP_BACKWARD && !identical)
 	    return 0;
 
 	  /* Keep checking.  We only have a dependency if
@@ -2267,11 +2276,14 @@  int
       rref = rref->next;
     }
 
+  /* Assume the worst if we nest to different depths.  */
+  if (lref || rref)
+    return 1;
+
   /* If we haven't seen any array refs then something went wrong.  */
   gcc_assert (fin_dep != GFC_DEP_ERROR);
 
-  /* Assume the worst if we nest to different depths.  */
-  if (lref || rref)
+  if (identical && fin_dep != GFC_DEP_NODEP)
     return 1;
 
   return fin_dep == GFC_DEP_OVERLAP;
Index: dependency.h
===================================================================
--- dependency.h	(Revision 273733)
+++ dependency.h	(Arbeitskopie)
@@ -37,7 +37,7 @@  int gfc_check_fncall_dependency (gfc_expr *, sym_i
 int gfc_check_dependency (gfc_expr *, gfc_expr *, bool);
 int gfc_expr_is_one (gfc_expr *, int);
 
-int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *);
+int gfc_dep_resolver(gfc_ref *, gfc_ref *, gfc_reverse *, bool identical = false);
 int gfc_are_equivalenced_arrays (gfc_expr *, gfc_expr *);
 
 gfc_expr * gfc_discard_nops (gfc_expr *);