GCC Bugzilla has been upgraded from version 4.4.9 to 5.0rc3. If you see any problem, please report it to bug 64968.
Bug 31726 - minloc/maxloc: wrong results with empty array (F2003 only)
Summary: minloc/maxloc: wrong results with empty array (F2003 only)
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libfortran (show other bugs)
Version: 4.3.0
: P3 normal
Target Milestone: ---
Assignee: Paul Thomas
URL:
Keywords: wrong-code
Depends on:
Blocks: 20585 30694
  Show dependency treegraph
 
Reported: 2007-04-27 11:12 UTC by Daniel Franke
Modified: 2007-06-24 11:05 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.1.1 4.2.0 4.3.0
Last reconfirmed: 2007-06-21 18:14:41


Attachments
Test case (630 bytes, text/plain)
2007-04-27 17:42 UTC, Tobias Burnus
Details
Test case for the library functions (668 bytes, text/plain)
2007-04-27 21:00 UTC, Thomas Koenig
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Franke 2007-04-27 11:12:04 UTC
$> cat minloc.f90
PRINT *, MINLOC((/ 42, 23 /), DIM=1, MASK=.FALSE.)
PRINT *, MINVAL((/ 42, 23 /), DIM=1, MASK=.FALSE.)
END

$> gfotran-4.2 -g -Wall minloc.f90 && ./a.out
           1
  2147483647

F2003, 13.7.78, MINLOC, Result Value:
"Case (ii) The result of MINLOC (ARRAY, MASK = MASK) is a rank-one array
[...]
If ARRAY has size zero or every element of MASK has the value false, all
elements of the result are zero."

"Case (iii) If ARRAY has rank one, MINLOC (ARRAY, DIM = DIM [, MASK =
MASK]) is a scalar whose value is equal to that of the first element of
MINLOC (ARRAY [, MASK = MASK])."


Thus, the result for MINLOC in above code should be zero. I'm not sure about MINVAL. The same problem exists with MAXLOC/MAXVAL.
Comment 1 Daniel Franke 2007-04-27 11:22:51 UTC
MINVAL/MAXVAL are ok.

F2003, 13.7.79, MINVAL, Result Value:
Case (i): [...] If ARRAY has size zero and type integer or real, the result 
          has the value of the positive number of the largest magnitude
          supported by the processor for numbers of the type and kind type
          parameter of ARRAY. If ARRAY has size zero and type character,
Comment 2 Tobias Burnus 2007-04-27 13:16:34 UTC
The problem is:
  if(0) { /* find index */
  else
        {
          pos.1 = 0;
        }
      D.1356 = (int4) ((<unnamed-unsigned:32>) pos.1 + 1);

As C and the internal tree have array indexes starting at one, the final index has to be incremented by one. Thus "pos.1 = 0" is ok, but "+1" is only correct if one has an index. The simplest would be to have "pos.1 = -1". Then one needs to change trans-intrinsic.c's gfc_conv_intrinsic_minmaxloc:

  /* Initialize the position to zero, following Fortran 2003.  We are free
     to do this because Fortran 95 allows the result of an entirely false
     mask to be processor dependent.  */
  gfc_add_modify_expr (&loop.pre, pos, gfc_index_zero_node);

One should also check the library version of MIN/MAXLOC.
Comment 3 Tobias Burnus 2007-04-27 15:59:13 UTC
Patch. Dump looks ok. Needs some testcases plus checking of the library version.

Index: trans-intrinsic.c
===================================================================
--- trans-intrinsic.c   (Revision 124216)
+++ trans-intrinsic.c   (Arbeitskopie)
@@ -2007,10 +2007,10 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * s

   gcc_assert (loop.dimen == 1);

-  /* Initialize the position to zero, following Fortran 2003.  We are free
-     to do this because Fortran 95 allows the result of an entirely false
-     mask to be processor dependent.  */
-  gfc_add_modify_expr (&loop.pre, pos, gfc_index_zero_node);
+  /* For zero-sized arrays or if the mask is .false. for every element,
+     zero needs to be returned according to the Fortran 2003 standard.
+     As this number is always incremented by one, it is initialized to -1.  */
+  gfc_add_modify_expr (&loop.pre, pos, build_int_cst (TREE_TYPE(pos), -1));

   gfc_mark_ss_chain_used (arrayss, 1);
   if (maskss)
@@ -2084,7 +2084,7 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * s
         the pos variable the same way as above.  */

       gfc_init_block (&elseblock);
-      gfc_add_modify_expr (&elseblock, pos, gfc_index_zero_node);
+      gfc_add_modify_expr (&elseblock, pos, build_int_cst (TREE_TYPE(pos), -1));
       elsetmp = gfc_finish_block (&elseblock);

       tmp = build3_v (COND_EXPR, maskse.expr, tmp, elsetmp);
Comment 4 Tobias Burnus 2007-04-27 17:42:25 UTC
Created attachment 13456 [details]
Test case

Writing test cases first helps!
I need go back to the drawing board as the pos = pos + 1 is not added for empty arrays.

The attached test case works with NAG f95, g95 and sunf95; it fails 24 times ("1" should be "0") with ifort and 18 times with unmodified gfortran (0 not 1 plus other bugs).
It does not test the library function though.

The test case shows however some additional problems where
 pos = pos.1 - (from - 1)  ! from = starting index of the array
does not work correctly (e.g. "3" instead of "2")
Comment 5 Thomas Koenig 2007-04-27 20:16:05 UTC
> One should also check the library version of MIN/MAXLOC.

I'll do that.  This is an area that I am working on anyway
(PR 30694).

Thanks for providing the test case, btw.  It is quite
thorough :-)

Comment 6 Thomas Koenig 2007-04-27 21:00:12 UTC
Created attachment 13457 [details]
Test case for the library functions

The library function appears to be ok.  Here is the test
case (semi-automatically generated from Tobias' patch :-)
Comment 7 Paul Thomas 2007-06-21 18:14:41 UTC
This fixes Tobias' testcase of comment #4.  Note that it is a diff relative to the patch for 32298.  I'll resubmit the latter in this new form.

Paul

Index: gcc/fortran/trans-intrinsic.c
===================================================================
*** gcc/fortran/trans-intrinsic.c	(révision 125706)
--- gcc/fortran/trans-intrinsic.c	(copie de travail)
*************** gfc_conv_intrinsic_minmaxloc (gfc_se * s
*** 1928,1933 ****
--- 1928,1934 ----
    tree tmp;
    tree elsetmp;
    tree ifbody;
+   tree offset;
    gfc_loopinfo loop;
    gfc_actual_arglist *actual;
    gfc_ss *arrayss;
*************** gfc_conv_intrinsic_minmaxloc (gfc_se * s
*** 2045,2059 ****
    /* Assign the value to the limit...  */
    gfc_add_modify_expr (&ifblock, limit, arrayse.expr);
  
!   /* Remember where we are.  */
!   gfc_add_modify_expr (&ifblock, pos, loop.loopvar[0]);
  
    ifbody = gfc_finish_block (&ifblock);
  
!   /* If it is a more extreme value or pos is still zero.  */
!   tmp = build2 (TRUTH_OR_EXPR, boolean_type_node,
! 		  build2 (op, boolean_type_node, arrayse.expr, limit),
! 		  build2 (EQ_EXPR, boolean_type_node, pos, gfc_index_zero_node));
    tmp = build3_v (COND_EXPR, tmp, ifbody, build_empty_stmt ());
    gfc_add_expr_to_block (&block, tmp);
  
--- 2046,2068 ----
    /* Assign the value to the limit...  */
    gfc_add_modify_expr (&ifblock, limit, arrayse.expr);
  
!   /* Remember where we are.  An offset must be added to the loop
!      counter to obtain the required position.  */
!   if (loop.temp_dim)
!     offset = build_int_cst (type, 1);
!   else
!     offset =fold_build2 (MINUS_EXPR, gfc_array_index_type,
! 			 gfc_index_one_node, loop.from[0]);
!   offset = gfc_evaluate_now (offset, &block);
! 
!   tmp = build2 (PLUS_EXPR, TREE_TYPE (pos),
! 		loop.loopvar[0], offset);
!   gfc_add_modify_expr (&ifblock, pos, tmp);
  
    ifbody = gfc_finish_block (&ifblock);
  
!   /* If it is a more extreme value.  */
!   tmp = build2 (op, boolean_type_node, arrayse.expr, limit);
    tmp = build3_v (COND_EXPR, tmp, ifbody, build_empty_stmt ());
    gfc_add_expr_to_block (&block, tmp);
  
*************** gfc_conv_intrinsic_minmaxloc (gfc_se * s
*** 2098,2109 ****
      }
    gfc_cleanup_loop (&loop);
  
!   /* Return a value in the range 1..SIZE(array).  */
!   tmp = fold_build2 (MINUS_EXPR, gfc_array_index_type, loop.from[0],
! 		     gfc_index_one_node);
!   tmp = fold_build2 (MINUS_EXPR, gfc_array_index_type, pos, tmp);
!   /* And convert to the required type.  */
!   se->expr = convert (type, tmp);
  }
  
  static void
--- 2107,2113 ----
      }
    gfc_cleanup_loop (&loop);
  
!   se->expr = convert (type, pos);
  }
  
  static void


Comment 8 patchapp@dberlin.org 2007-06-22 02:05:32 UTC
Subject: Bug number PR31726

A patch for this bug has been added to the patch tracker.
The mailing list url for the patch is http://gcc.gnu.org/ml/gcc-patches/2007-06/msg01583.html
Comment 9 Paul Thomas 2007-06-24 11:04:15 UTC
Subject: Bug 31726

Author: pault
Date: Sun Jun 24 11:04:02 2007
New Revision: 125983

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=125983
Log:
2007-06-24  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/32298
	PR fortran/31726
	* trans-intrinsic.c (gfc_conv_intrinsic_minmaxloc): Calculate
	the offset between the loop counter and the position as
	defined. Add the offset within the loop so that the mask acts
	correctly.  Do not advance the location on the basis that it
	is zero.

2007-06-24  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/31726
	* gfortran.dg/minmaxloc_1.f90: New test.

	PR fortran/32298
	* gfortran.dg/minmaxloc_2.f90: New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/minmaxloc_1.f90
    trunk/gcc/testsuite/gfortran.dg/minmaxloc_2.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-intrinsic.c
    trunk/gcc/testsuite/ChangeLog

Comment 10 Paul Thomas 2007-06-24 11:05:27 UTC
Fixed on trunk

Paul
Comment 11 Thomas Koenig 2007-10-15 18:23:53 UTC
Subject: Bug 31726

Author: tkoenig
Date: Mon Oct 15 18:23:39 2007
New Revision: 129365

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=129365
Log:
2007-10-25  Thomas Koenig  <tkoenig@gcc.gnu.org>
	    Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/32298
	PR fortran/31726
	PR fortran/33354
	Backport from trunk
	* trans-intrinsic.c (gfc_conv_intrinsic_minmaxloc): Calculate
	the offset between the loop counter and the position as
	defined. Add the offset within the loop so that the mask acts
	correctly.  Do not advance the location on the basis that it
	is zero.

2007-10-15  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR fortran/33354
	* gfortran.dg/minmaxloc_4.f90:  New test.



Added:
    branches/gcc-4_2-branch/gcc/testsuite/gfortran.dg/minmaxloc_4.f90
Modified:
    branches/gcc-4_2-branch/gcc/fortran/ChangeLog
    branches/gcc-4_2-branch/gcc/fortran/trans-intrinsic.c
    branches/gcc-4_2-branch/gcc/testsuite/ChangeLog