[Bug fortran/35994] New: MAXLOC and MINLOC off by one with mask

[Bug fortran/35994] New: MAXLOC and MINLOC off by one with mask

[dick dot hendrickson at gmail dot com]

The MAXLOC and MINLOC functions give an off by one wrong
answer when there is a mask argument.

Dick Hendrickson

      program GA4076

! fails on Windows XP
! gcc version 4.4.0 20080312 (experimental) [trunk revision 133139]

      REAL DDA(100)
      dda = (/(J1,J1=1,100)/)

      IDS = MAXLOC(DDA,1)
      print *, 1, ids, ids.eq.100         !expect 100

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

      IDS = minLOC(DDA,1)
      print *, 3, ids, ids.eq.1           !expect 1

      IDS = MinLOC(DDA,1, (/(J1,J1=1,100)/) > 50)
      print *, 4, ids, ids.eq.51          !expect 51

      END 

C:\gfortran:gfortran ga4076.f

C:\gfortran:a
           1         100 T
           2         101 F
           3           1 T
           4          52 F


-- 
           Summary: MAXLOC and MINLOC off by one with mask
           Product: gcc
           Version: 4.4.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: dick dot hendrickson at gmail dot com


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

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

[tkoenig at gcc dot gnu dot org]

------- Comment #1 from tkoenig at gcc dot gnu dot org  2008-04-20 20:50 -------
Confirmed.  Ouch.


-- 

tkoenig at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tkoenig at gcc dot gnu dot
                   |                            |org
           Severity|normal                      |major
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
           Keywords|                            |wrong-code
   Last reconfirmed|0000-00-00 00:00:00         |2008-04-20 20:50:27
               date|                            |


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

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

[tkoenig at gcc dot gnu dot org]

------- Comment #2 from tkoenig at gcc dot gnu dot org  2008-04-22 18:43 -------
$ gfortran ga4076.f90 
$ ./a.out
           1         100 T
           2         101 F
           3           1 T
           4          52 F
$ gfortran-4.2 ga4076.f90 
$ ./a.out
           1         100 T
           2         100 T
           3           1 T
           4          51 T
$ gfortran-4.3 -static ga4076.f90 
$ ./a.out
           1         100 T
           2         101 F
           3           1 T
           4          52 F


-- 

tkoenig at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|2008-04-20 20:50:27         |2008-04-22 18:43:07
               date|                            |
            Summary|MAXLOC and MINLOC off by one|[4.3/4.4 regression] MAXLOC
                   |with mask                   |and MINLOC off by one with
                   |                            |mask
   Target Milestone|---                         |4.3.1


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

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

[tkoenig at gcc dot gnu dot org]

------- Comment #3 from tkoenig at gcc dot gnu dot org  2008-04-22 18:57 -------
This one is seriously strange.

It depends on the mask argument being a compile-time constant.
Apparently, simplification goes down a wrong path somewhere.
Looking at the 

$ cat try.f90
      program GA4076

! fails on Windows XP
! gcc version 4.4.0 20080312 (experimental) [trunk revision 133139]

      REAL DDA(100)
      dda = (/(J1,J1=1,100)/)

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

      END 
$ gfortran -fdump-tree-original try.f90
$ ./a.out
           2         101 F

The loop generated for maxloc lacks any reference to the mask:

      pos.2 = 0;
      {
        integer(kind=4) S.6;

        S.6 = 1;
        while (1)
          {
            if (S.6 > 100) goto L.1;
            if (A.5[S.6 + -1])
              {
                offset.3 = 1;
                if (dda[S.6 + -1] > limit.4 || pos.2 == 0 && dda[S.6 + -1] ==
limit.4)
                  {
                    limit.4 = dda[S.6 + -1];
                    pos.2 = S.6 + offset.3;
                  }
              }
            S.6 = S.6 + 1;
          }
        L.1:;
      }
      ids = pos.2;
    }

The following works:

$ cat try2.f90
      program GA4076

      REAL DDA(100)
      dda = (/(J1,J1=1,100)/)

      IDS = MAXLOC(DDA,1, dda > 50)
      print *, 2, ids, ids.eq.100         !expect 100

      END 
$ gfortran try2.f90
$ ./a.out
           2         100 T

Paul, can you maybe shed any light on this?


-- 

tkoenig at gcc dot gnu dot org changed:

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


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

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

[jakub at gcc dot gnu dot org]

-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4


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

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

[pault at gcc dot gnu dot org]

------- Comment #4 from pault at gcc dot gnu dot org  2008-04-22 22:34 -------

> Paul, can you maybe shed any light on this?
> 

Not for some days, I'm afraid.  I'm in China, up to my eyeballs with day-time
work.

It'll be high priority when I get back.

Paul


-- 


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

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

[fxcoudert at gcc dot gnu dot 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

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

[jvdelisle at gcc dot gnu dot org]

------- Comment #6 from jvdelisle at gcc dot gnu dot org  2008-04-24 02:04 -------
FX, I will test the patch tonight and see how it works and if good, submit to
list for approval.


-- 


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

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

[jvdelisle at gcc dot gnu dot org]

------- Comment #7 from jvdelisle at gcc dot gnu dot org  2008-04-24 03:48 -------
Subject: Bug 35994

Author: jvdelisle
Date: Thu Apr 24 03:48:00 2008
New Revision: 134615

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=134615
Log:
2008-04-23  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
            Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

        PR fortran/35994
        * trans-instrinsic.c (gfc_conv_intrinsic_minmaxloc): Correctly adjust
        loop counter offset.

Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-intrinsic.c


-- 


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

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

[jvdelisle at gcc dot gnu dot org]

------- Comment #8 from jvdelisle at gcc dot gnu dot org  2008-04-24 03:49 -------
Fixed on 4.4, needs to be backported to 4.3.1


-- 

jvdelisle at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |jvdelisle at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2008-04-22 18:43:07         |2008-04-24 03:49:44
               date|                            |


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

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

[jvdelisle at gcc dot gnu dot org]

------- Comment #9 from jvdelisle at gcc dot gnu dot org  2008-04-24 03:55 -------
Subject: Bug 35994

Author: jvdelisle
Date: Thu Apr 24 03:55:05 2008
New Revision: 134616

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=134616
Log:
2008-04-23  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

        PR fortran/35994
        * gfortran.dg/minmaxloc_5.f90: New test.
        * gfortran.dg/minmaxloc_6.f90: New test.

Added:
    trunk/gcc/testsuite/gfortran.dg/minmaxloc_5.f90
    trunk/gcc/testsuite/gfortran.dg/minmaxloc_6.f90
Modified:
    trunk/gcc/testsuite/ChangeLog


-- 


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

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

[jvdelisle at gcc dot gnu dot org]

------- Comment #10 from jvdelisle at gcc dot gnu dot org  2008-04-26 15:34 -------
Subject: Bug 35994

Author: jvdelisle
Date: Sat Apr 26 15:33:27 2008
New Revision: 134708

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=134708
Log:
2008-04-26  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
            Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

        PR fortran/35994
        * trans-instrinsic.c (gfc_conv_intrinsic_minmaxloc):
         Correctly adjust loop counter offset.

Modified:
    branches/gcc-4_3-branch/gcc/fortran/ChangeLog
    branches/gcc-4_3-branch/gcc/fortran/trans-intrinsic.c


-- 


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

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

[jvdelisle at gcc dot gnu dot org]

------- Comment #11 from jvdelisle at gcc dot gnu dot org  2008-04-26 15:36 -------
Subject: Bug 35994

Author: jvdelisle
Date: Sat Apr 26 15:35:14 2008
New Revision: 134709

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=134709
Log:
2008-04-26  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

        PR fortran/35994
        * gfortran.dg/minmaxloc_5.f90: New test.
        * gfortran.dg/minmaxloc_6.f90: New test.

Added:
    branches/gcc-4_3-branch/gcc/testsuite/gfortran.dg/minmaxloc_5.f90
    branches/gcc-4_3-branch/gcc/testsuite/gfortran.dg/minmaxloc_6.f90
Modified:
    branches/gcc-4_3-branch/gcc/testsuite/ChangeLog


-- 


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

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

[jvdelisle at gcc dot gnu dot org]

------- Comment #12 from jvdelisle at gcc dot gnu dot org  2008-04-26 15:36 -------
Fixed now on 4.3.1  Closing


-- 

jvdelisle at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


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