Patchwork [fortran] Fix PR 44693

login
register
mail settings
Submitter Thomas Koenig
Date July 4, 2010, 9:46 a.m.
Message ID <1278236813.4645.7.camel@linux-fd1f.site>
Download mbox | patch
Permalink /patch/57834/
State New
Headers show

Comments

Thomas Koenig - July 4, 2010, 9:46 a.m.
Hello world,

the attached patch fixes PR 44693.

During regression-testing, there was a failure of
dynamic_dispatch_6.f03 at higher optimization levels.  I *think* that
this is not related to this patch, but I would appreciate if the
reviewer would check for that.  Of course, if this turns out to really
cause a regression or expose a latent bug, I don't want to commit this.

OK for trunk?

	Thomas

2010-07-04  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/PR44693
	* check.c (dim_rank_check):  Also check intrinsic functions.
	Adjust permissible rank for functions which reduce the rank of
	their argument.

	PR fortran/PR44693
	* dim_range_1.f90:  New test.
Tobias Burnus - July 4, 2010, 10:27 a.m.
Thomas Koenig wrote:
> During regression-testing, there was a failure of
> dynamic_dispatch_6.f03 at higher optimization levels.
See: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44596
(Paul has a draft patch for this.)


Regarding the patch: Glancing at it, I do not understand the patch. My
impression is that it won't work, but your example proofs me wrong. If
no one reviews it, I will do so later.


Anyway, I was puzzled about:

a) For

  esss = sum(Ix * Iyz, 0) ! { dg-error "is not a valid dimension index" }

when does dim_range_check actually get called? dim=0 is wrong, but
gfc_check_product_sum does not call dim_rank_check.

b) I do not understand the:

+      switch(array->value.function.isym->id)
+	{
+	case GFC_ISYM_SUM:
+	  rank = array->rank + 1;
+	  break;


If I have count(array=maxval(arrayvar), dim=1), why can't I use
"array->rank" but have to use "array->rank + 1"? Why doesn't
maxval(arrayvar) return the correct rank?

Tobias
Thomas Koenig - July 4, 2010, 10:50 a.m.
Am Sonntag, den 04.07.2010, 12:27 +0200 schrieb Tobias Burnus:

> Anyway, I was puzzled about:
> 
> a) For
> 
>   esss = sum(Ix * Iyz, 0) ! { dg-error "is not a valid dimension index" }
> 
> when does dim_range_check actually get called? dim=0 is wrong, but
> gfc_check_product_sum does not call dim_rank_check.

gfc_check_product_sum calls check_reduction, which in turn calls
dim_rank_check.

> 
> b) I do not understand the:
> 
> +      switch(array->value.function.isym->id)
> +	{
> +	case GFC_ISYM_SUM:
> +	  rank = array->rank + 1;
> +	  break;
> 
> 
> If I have count(array=maxval(arrayvar), dim=1), why can't I use
> "array->rank" but have to use "array->rank + 1"? Why doesn't
> maxval(arrayvar) return the correct rank?

Consider

real a(10:10)

then "sum(a,dim=2)" is correct, and the sum has rank 1.

If you think a more descriptive comment is in order, I will be more than
happy to oblige ;-)

	Thomas
Tobias Burnus - July 4, 2010, 8:06 p.m.
Thomas Koenig wrote:
> OK for trunk?
>   

Not OK - for the reason I was expecting.

-  if (dim->expr_type != EXPR_CONSTANT
-      || (array->expr_type != EXPR_VARIABLE
-	  && array->expr_type != EXPR_ARRAY))
+  if (dim->expr_type != EXPR_CONSTANT)
     return SUCCESS;


This part is OK. While the following does not make sense at all:

-  rank = array->rank;
+  if (array->expr_type == EXPR_FUNCTION && array->value.function.isym)
+    {
+
+      /* Functions which reduce the rank of their arguments.  */
+      switch(array->value.function.isym->id)
+	{
+	case GFC_ISYM_SUM:
+	case GFC_ISYM_PRODUCT:
+	case GFC_ISYM_ANY:
+	case GFC_ISYM_ALL:
+	case GFC_ISYM_COUNT:
+	case GFC_ISYM_MINVAL:
+	case GFC_ISYM_MAXVAL:
+	  rank = array->rank + 1;
+	  break;
+
+	default:
+	  rank = array->rank;
+	}
+    }
+  else
+    {
+      rank = array->rank;
+    }
+


Example:

real :: a(3,3,3)
a = 5.0
print *, sum(sum(a,dim=1), dim=3) 
end

That code is invalid as "sum" only returns a rank 2 array, but due to
the increment of the rank in the second part of your patch, this is not
rejected.

How about using only the first part of your patch - and keep the old and
simple
  rank = array->rank;

OK if you do this and add the add the new example to the test file.

Tobias
Thomas Koenig - July 4, 2010, 8:20 p.m.
Hi Tobias,

> How about using only the first part of your patch - and keep the old
> and
> simple
>   rank = array->rank;

Actually, you are correct.  I was led down the garden path by a failure
in minmaxloc_4.f90, which contains


PROGRAM TST
  IMPLICIT NONE
  REAL :: A(1,3)
  REAL :: B(3,1)

[...]

  if (minloc(sum(b(1:3,:),2),2) .ne. 1) call abort()
  if (maxloc(sum(b(1:3,:),2),2) .ne. 3) call abort()

which is wrong.

Thanks for catching the error!

I will commit a correct version shortly.

	Thomas

Patch

Index: check.c
===================================================================
--- check.c	(Revision 161784)
+++ check.c	(Arbeitskopie)
@@ -473,12 +473,34 @@  dim_rank_check (gfc_expr *dim, gfc_expr *array, in
   if (dim == NULL)
     return SUCCESS;
 
-  if (dim->expr_type != EXPR_CONSTANT
-      || (array->expr_type != EXPR_VARIABLE
-	  && array->expr_type != EXPR_ARRAY))
+  if (dim->expr_type != EXPR_CONSTANT)
     return SUCCESS;
 
-  rank = array->rank;
+  if (array->expr_type == EXPR_FUNCTION && array->value.function.isym)
+    {
+
+      /* Functions which reduce the rank of their arguments.  */
+      switch(array->value.function.isym->id)
+	{
+	case GFC_ISYM_SUM:
+	case GFC_ISYM_PRODUCT:
+	case GFC_ISYM_ANY:
+	case GFC_ISYM_ALL:
+	case GFC_ISYM_COUNT:
+	case GFC_ISYM_MINVAL:
+	case GFC_ISYM_MAXVAL:
+	  rank = array->rank + 1;
+	  break;
+
+	default:
+	  rank = array->rank;
+	}
+    }
+  else
+    {
+      rank = array->rank;
+    }
+
   if (array->expr_type == EXPR_VARIABLE)
     {
       ar = gfc_find_array_ref (array);