[Bug fortran/32298] New: Intrinsic MINLOC / MAXLOC gives wrong results (gfortran build 20070522)

[Bug fortran/32298] New: Intrinsic MINLOC / MAXLOC gives wrong results (gfortran build 20070522)

[jens dot bischoff at freenet dot de]

The following source code:
!-------------------------
PROGRAM ERR_MINLOC

   INTEGER, PARAMETER :: N = 7

   DOUBLE PRECISION, DIMENSION (N), PARAMETER :: A &
     = (/ 0.3D0, 0.455D0, 0.6D0, 0.7D0, 0.72D0, 0.76D0, 0.79D0 /)

   DOUBLE PRECISION :: B
   INTEGER          :: I, J, K

  DO I = 1, N
    B = A(I)
    J = MINLOC (ABS (A - B), 1)
    K = MAXLOC (ABS (A - B), 1)
    PRINT *, I, J, K
  END DO  
 STOP

END PROGRAM ERR_MINLOC
!----------------------

gives the following result:
(superflouos blanks are deleted):
--------------------------
 1 2 7
 2 2 7
 3 3 7
 4 4 2
 5 5 2
 6 6 2
 7 7 2
--------------------------

The correct result is:
--------------------------
 1 1 7
 2 2 7
 3 3 1
 4 4 1
 5 5 1
 6 6 1
 7 7 1
--------------------------


-- 
           Summary: Intrinsic MINLOC / MAXLOC gives wrong results (gfortran
                    build 20070522)
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: jens dot bischoff at freenet dot de
GCC target triplet: i386-pc-mingw32


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

[Bug fortran/32298] MINLOC / MAXLOC: off-by one for PARAMETER arrays

[burnus at gcc dot gnu dot org]

------- Comment #1 from burnus at gcc dot gnu dot org  2007-06-12 16:05 -------
The result is ok, if one removes PARAMETER.

If one looks at the dump one find the following difference between arrays which
are parameters and those which are variables:

-                S.3 = 0;
+                S.3 = 1;
                 while (1)
                  {
-                   if (S.3 > 6) goto L.3;
+                   if (S.3 > 7) goto L.3;
-                   if (ABS_EXPR <A.2[S.3] - D.1367> < limit.1 || pos.0 == 0)
+                   if (ABS_EXPR <a[S.3 + -1] - D.1367> < limit.1 || pos.0 ==
0)
                      {
-                       limit.1 = ABS_EXPR <A.2[S.3] - D.1367>;
+                        limit.1 = ABS_EXPR <a[S.3 + -1] - D.1367>;
-                        pos.0 = S.3;
+                        pos.0 = S.3;

(This might also affect MAXVAL/MINVAL, though I did not manage to cook up an
example.)


-- 

burnus at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
           Keywords|                            |wrong-code
   Last reconfirmed|0000-00-00 00:00:00         |2007-06-12 16:05:26
               date|                            |
            Summary|Intrinsic MINLOC / MAXLOC   |MINLOC / MAXLOC: off-by one
                   |gives wrong results         |for PARAMETER arrays
                   |(gfortran build 20070522)   |


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

[Bug fortran/32298] MINLOC / MAXLOC: off-by one for PARAMETER arrays

[burnus at gcc dot gnu dot org]

-- 

burnus at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu dot
                   |                            |org
 GCC target triplet|i386-pc-mingw32             |
      Known to fail|                            |4.3.0 4.2.0 4.1.3
   Target Milestone|---                         |4.3.0


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

[Bug fortran/32298] MINLOC / MAXLOC: off-by one for PARAMETER arrays

[dfranke at gcc dot gnu dot org]

------- Comment #2 from dfranke at gcc dot gnu dot org  2007-06-16 14:00 -------
A simplified testcase:

$> cat pr32298.f90
PROGRAM ERR_MINLOC
  INTEGER, PARAMETER :: N = 7
  DOUBLE PRECISION, DIMENSION (N), PARAMETER :: A &
    = (/ 0.3D0, 0.455D0, 0.6D0, 0.7D0, 0.72D0, 0.76D0, 0.79D0 /)

  INTEGER :: I, K
  I = 7
  K = MAXLOC (ABS (A - A(I)), 1)

  PRINT *, I, K
END PROGRAM ERR_MINLOC

$> gfortran-svn -fdump-tree-original pr32298.f90
$> cat pr32298.f90.003t.original
[...]
    pos.0 = 0;
    {
      int4 S.3;
      S.3 = 0;
      while (1)
        {
          if (S.3 > 6) goto L.1;
          if (ABS_EXPR <A.2[S.3] - D.1011> > limit.1 || pos.0 == 0)
            {
              limit.1 = ABS_EXPR <A.2[S.3] - D.1011>;
              pos.0 = S.3;
            }
          S.3 = S.3 + 1;
        }
      L.1:;
    }
    k = pos.0 + 1;
  }

Variable pos.0 starts at 0 and thus triggers the right part of the or-clause
within the if-statement. Hence, it gets assigned the value of S.3, which
happens to be 0 within the first iteration. Within the second iteration of the
while, pos.0 still equals 0 which triggers the if-statement again - pos.0 will
now be set to 1 as S.3 was increased during the last iteration. At the end, K
ist set to pos.0 + 1, which (wrongly) equals 2 in this case.

Someone who intimiately knows the scalarizer should be able to fix this
easily?!


> (This might also affect MAXVAL/MINVAL, though I did not manage to cook 
> up an example.)

I don't think so as MINVAL/MAXVAL do not track the respective positions.


-- 

dfranke at gcc dot gnu dot org changed:

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


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

[Bug fortran/32298] MINLOC / MAXLOC: off-by one for PARAMETER arrays

[pault at gcc dot gnu dot org]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3804 bytes --]



------- Comment #3 from pault at gcc dot gnu dot org  2007-06-20 12:15 -------
This does the job by detecting the generation of a temporary. In this case the
loop is zero based but the calculated value for the position does not reflect
this.

I'll have a think about whether or not this is the cleanest way to fix the bug
and then I will submit tonight.

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
*** 2046,2052 ****
    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);

--- 2046,2060 ----
    gfc_add_modify_expr (&ifblock, limit, arrayse.expr);

    /* Remember where we are.  */
!   if (loop.temp_dim)
!     {
!       tmp = build2 (PLUS_EXPR, TREE_TYPE (pos),
!                   loop.loopvar[0],
!                   build_int_cst (type, 1));
!       gfc_add_modify_expr (&ifblock, pos, tmp);
!     }
!   else
!     gfc_add_modify_expr (&ifblock, pos, loop.loopvar[0]);

    ifbody = gfc_finish_block (&ifblock);

*************** gfc_conv_intrinsic_minmaxloc (gfc_se * s
*** 2099,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,2120 ----
    gfc_cleanup_loop (&loop);

    /* Return a value in the range 1..SIZE(array).  */
!   if (!loop.temp_dim)
!     {
!       tmp = fold_build2 (MINUS_EXPR, gfc_array_index_type, loop.from[0],
!                        gfc_index_one_node);
!       pos = fold_build2 (MINUS_EXPR, gfc_array_index_type, pos, tmp);
!     }
    /* And convert to the required type.  */
!   se->expr = convert (type, pos);
  }

  static void
Index: gcc/testsuite/gfortran.dg/minmax_loc_1.f90
===================================================================
*** gcc/testsuite/gfortran.dg/minmax_loc_1.f90  (révision 0)
--- gcc/testsuite/gfortran.dg/minmax_loc_1.f90  (révision 0)
***************
*** 0 ****
--- 1,29 ----
+ ! { dg-do run }
+ ! Tests the fix for PR32298, in which the scalarizer would generate
+ ! a temporary in the course of evaluating MINLOC or MAXLOC, thereby
+ ! setting the start of the scalarizer loop to zero.
+ !
+ ! Contributed by Jens Bischoff <jens.bischoff@freenet.de> 
+ !
+ PROGRAM ERR_MINLOC
+ 
+    INTEGER, PARAMETER :: N = 7
+ 
+    DOUBLE PRECISION, DIMENSION (N), PARAMETER :: A &
+      = (/ 0.3D0, 0.455D0, 0.6D0, 0.7D0, 0.72D0, 0.76D0, 0.79D0 /)
+ 
+    DOUBLE PRECISION :: B
+    INTEGER          :: I, J(N), K(N)
+ 
+   DO I = 1, N
+     B = A(I)
+     J(I) = MINLOC (ABS (A - B), 1)
+     K(I) = MAXLOC (ABS (A - B), 1)
+   END DO
+ 
+   if (any (J .NE. (/1,2,3,4,5,6,7/))) call abort ()
+   if (any (K .NE. (/7,7,1,1,1,1,1/))) call abort ()
+ 
+  STOP
+ 
+ END PROGRAM ERR_MINLOC


-- 

pault at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |pault at gcc dot gnu dot org
                   |dot org                     |
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2007-06-12 16:05:26         |2007-06-20 12:15:24
               date|                            |


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

[Bug fortran/32298] MINLOC / MAXLOC: off-by one for PARAMETER arrays

[patchapp at dberlin dot org]

------- Comment #4 from patchapp at dberlin dot org  2007-06-21 05:05 -------
Subject: Bug number PR32298

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/msg01521.html


-- 


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

[Bug fortran/32298] MINLOC / MAXLOC: off-by one for PARAMETER arrays

[pault at gcc dot gnu dot org]

------- Comment #5 from pault at gcc dot gnu dot org  2007-06-24 11:04 -------
Subject: Bug 32298

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


-- 


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

[Bug fortran/32298] MINLOC / MAXLOC: off-by one for PARAMETER arrays

[pault at gcc dot gnu dot org]

------- Comment #6 from pault at gcc dot gnu dot org  2007-06-24 11:06 -------
Fixed on trunk

Paul


-- 

pault at gcc dot gnu dot org changed:

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


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

[Bug fortran/32298] MINLOC / MAXLOC: off-by one for PARAMETER arrays

[tkoenig at gcc dot gnu dot org]

------- Comment #7 from tkoenig at gcc dot gnu dot org  2007-10-15 18:23 -------
Subject: Bug 32298

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


-- 


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