[Bug fortran/35994] [4.3/4.4 regression] MAXLOC and MINLOC off by one with mask

["fxcoudert at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org>]

------- Comment #5 from fxcoudert at gcc dot gnu dot org  2008-04-23 08:11 -------
It's not about constantness, but about creating a temporary or not. The
following testcase uses a constant mask and it still gives the right answer:

      REAL DDA(100)
      logical, parameter :: mask(1:100) = (/(J1,J1=1,100)/) > 50
      dda = (/(J1,J1=1,100)/)

      IDS = MAXLOC(DDA,1, mask)
      print *, ids ! expect 100
      END

while the following also uses a compile-time constant mask and gives the wrong
answer (51 when it should be 50):

      REAL DDA(100)
      logical, parameter :: mask(1:100) = (/(J1,J1=1,100)/) > 50
      dda = (/(J1,J1=1,100)/)

      IDS = MAXLOC(DDA,1, .not. mask)
      print *, ids ! expect 50
      END


The code in question is in gfc_conv_intrinsic_minmaxloc (trans-intrinsic.c):

  /* Remember where we are.  An offset must be added to the loop
     counter to obtain the required position.  */
  if (loop.temp_dim)
    tmp = build_int_cst (gfc_array_index_type, 1);
  else
    tmp = fold_build2 (MINUS_EXPR, gfc_array_index_type,
                       gfc_index_one_node, loop.from[0]);

All the failing cases I've seen so far happen in the (loop.temp_dim == true)
branch, and would be fixed by putting a zero there instead of the 1. But...
that's not all! It can also be something else than an off-by-one, it can be
off-by-more-than-that:

      REAL DDA(5:104)
      dda = (/(J1,J1=1,100)/)

      IDS = MAXLOC(DDA,1)
      print *, ids ! expect 100
      IDS = MAXLOC(DDA,1, (/(J1,J1=1,100)/) > 50)
      print *, ids ! expect 100

      END

gives 105 instead of 100 for the second call to MAXLOC. I'm not too sure what
is the correct approach, maybe something like (can't test right now):

Index: trans-intrinsic.c
===================================================================
--- trans-intrinsic.c   (revision 134439)
+++ trans-intrinsic.c   (working copy)
@@ -2171,11 +2171,11 @@

   /* Remember where we are.  An offset must be added to the loop
      counter to obtain the required position.  */
-  if (loop.temp_dim)
+  if (loop.from[0])
     tmp = build_int_cst (gfc_array_index_type, 1);
   else
-    tmp =fold_build2 (MINUS_EXPR, gfc_array_index_type,
-                        gfc_index_one_node, loop.from[0]);
+    tmp = fold_build2 (MINUS_EXPR, gfc_array_index_type,
+                      gfc_index_one_node, loop.from[0]);
   gfc_add_modify_expr (&block, offset, tmp);

   tmp = fold_build2 (PLUS_EXPR, TREE_TYPE (pos),

(the loop.temp_dim vs. loop.from[0] is important, the rest is whitespace
change).


-- 

fxcoudert at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fxcoudert at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35994