[Bug fortran/37903] New: wrong-code for complicated vector subscripts

[Bug fortran/37903] New: wrong-code for complicated vector subscripts

[mikael dot morin at tele2 dot fr]

Modified code from a comment for PR36091
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36091#c3

      integer :: i(-1:1) = 1, j(3) = 1, k(3)
      k = j((/1,1,1/)+i)
      end

Wrong code, the temporary array generated is too small.


-- 
           Summary: wrong-code for complicated vector subscripts
           Product: gcc
           Version: 4.4.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: mikael dot morin at tele2 dot fr


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[burnus at gcc dot gnu dot org]

------- Comment #1 from burnus at gcc dot gnu dot org  2008-10-23 20:11 -------
Works with 4.1/4.2...


-- 

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         |2008-10-23 20:11:43
               date|                            |
            Summary|wrong-code for complicated  |[4.3/4.4 Regression] wrong-
                   |vector subscripts           |code for complicated vector
                   |                            |subscripts


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[dominiq at lps dot ens dot fr]

------- Comment #2 from dominiq at lps dot ens dot fr  2008-10-23 20:30 -------
> Modified code from a comment for PR36091
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36091#c3

First the code came from http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31610#c16

Second, if I change the lower bound of 'i' as in

      integer :: i(-9:1) = 1, j(11) = 1, k(11)
      k = j((/1,1,1,1,1,1,1,1,1,1,1/)+i)
      print *, k
      end

the size of the temporary does not change: the lower bound is probably not
taken into account.

Third 4.2 uses 2 temporaries (of the right size).


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[mikael dot morin at tele2 dot fr]

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



------- Comment #3 from mikael dot morin at tele2 dot fr  2008-10-23 21:27 -------
Quickfix (understand: not regression tested):

Index: trans-array.c
===================================================================
--- trans-array.c       (révision 141321)
+++ trans-array.c       (copie de travail)
@@ -3380,7 +3380,7 @@
            {
              /* The frontend has worked out the size for us.  */
              loopspec[n] = ss;
-             continue;
+             break;
            }

          if (ss->type == GFC_SS_CONSTRUCTOR)


This forces to use the array constructor's ss to setup the loop.
As it is zero-based, the upper bounds has the proper value and the array has
the proper size. 
I suspect this is only hiding the problem though. 


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[mikael dot morin at tele2 dot fr]

------- Comment #4 from mikael dot morin at tele2 dot fr  2008-10-23 22:31 -------
There is this comment at the beginning of gfc_trans_create_temp_array.

/* TODO: Investigate why "if (n < loop->temp_dim)
   gcc_assert (integer_zerop (loop->from[n]));" fails here.  */

This is the case here: n=0, loop->temp_dim=1, loop.from[0]=<lbound(i)>=-1

I don't understand the use of the (n >= loop->temp_dim) condition preventing
the loop bounds from being moved (to be zero-based).

The problem arises farther in the function, when gfc_index_zero_node is used
instead of loop.from, and only the (wrong) loop.to is taken into account. 


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[dominiq at lps dot ens dot fr]

------- Comment #5 from dominiq at lps dot ens dot fr  2008-10-24 05:48 -------
> Quickfix (understand: not regression tested): ...

With the patch the temporary has the right size, but there is one regression:

FAIL: gfortran.dg/array_constructor_15.f90  -O  scan-tree-dump-times original
"atmp" 0

If I understand the test correctly, it just checks that no temporary has been
created.


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[rguenth at gcc dot gnu dot org]

-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.3.3


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[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=37903

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[pault at gcc dot gnu dot org]

------- Comment #6 from pault at gcc dot gnu dot org  2008-10-28 06:25 -------
(In reply to comment #3)

> I suspect this is only hiding the problem though. 
> 
 Indeed it is - the old problem returns with 

      integer :: i(-1:1) = 1, j(3) = 1, k(3)
      k = j(I + (/1,1,1/))
      end

for obvious reasons.  It's a nice try though and you are obviously getting to
the nub of the problem.

Paul


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[mikael dot morin at tele2 dot fr]

------- Comment #7 from mikael dot morin at tele2 dot fr  2008-10-28 12:21 -------
This is happening because
(1) gfc_trans_constant_array_constructor sets loop->temp_dim to 1.
(2) gfc_conv_loop_setup choses the last ss whose shape is known (that of i). 
(3) gfc_conv_loop_setup sets loop bounds (-1->1) according to the chosen ss
(i). 
(4) gfc_trans_create_temp_array conditionally resets the loop bounds (if n >=
loop->temp_dim).
(5) gfc_trans_create_temp_array later assumes that the loop starts at zero. 


(2) explains why it works with y(i+(/1,1,1/)) but not with y((/1,1,1/)+i)
My patch in comment #3 changes the "last ss" to "first ss" and of course it
doesn't work as was said in comment #6

I have two fixes almost ready. 


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[mikael dot morin at tele2 dot fr]

------- Comment #8 from mikael dot morin at tele2 dot fr  2008-10-28 14:42 -------
Created an attachment (id=16573)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=16573&action=view)
second fix

This patch solves the problem by releasing the (4) condition, making the loop
bounds reset unconditional at the beginning of gfc_trans_create_temp_array. 
This is harmless because most of the time the loop->from is already zero (there
was an assertion for that before PR31610) and the code adds nothing, while in
other cases the bounds reset is executed. 


I had initially a problem though, because then the array indexes for accessing
elements of (/1,1,1/) were wrong as the offset for that array was set too
early, taking the initial loop bounds into account instead of the later
zero-based ones. 

I looked at what happened in the case of i as its indexes were not
miscalculated and found that there was a code for that at the end of
gfc_conv_loop_setup.

What this patch does is remove the code setting the offset according to the
loop in gfc_trans_array_constructor, and allow GFC_SS_CONSTRUCTOR in
gfc_conv_loop_setup's code.


This patch passes the regression tests without any new failure.


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[dominiq at lps dot ens dot fr]

------- Comment #9 from dominiq at lps dot ens dot fr  2008-10-28 17:18 -------
The patch in comment #8 works as expected on my tests and regtest without
regression but conflicts with the patch for pr35681 in
http://gcc.gnu.org/ml/fortran/2008-10/msg00256.html.

Some synchronization seems needed.


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[mikael dot morin at tele2 dot fr]

------- Comment #10 from mikael dot morin at tele2 dot fr  2008-10-28 18:27 -------
Created an attachment (id=16574)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=16574&action=view)
first fix

This patch tries to solve the problem by changing the (2) condition. 
It tries to use an ss whose starting value is zero when there is one (In this
case it is that of (/1,1,1/)). 

The regression tests pass with both this patch applied and that one
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31610#c21
reverted (ICEing is better than wrong-coding). 

I was initially dissatisfied by this patch as it was not changing the beginning
of gfc_trans_create_temp_array, which was leaving room for another case where
the loop would not start at zero. I failed to find such a case though. 

However, I believe it is still valuable as it produces a (just a little) better
code (see array indexes) than the other one. 
Now I suggest we apply both patches. 
Any comments ?


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[mikael dot morin at tele2 dot fr]

------- Comment #11 from mikael dot morin at tele2 dot fr  2008-10-28 18:43 -------
(In reply to comment #9)
> 
> Some synchronization seems needed.
> 

Well, may patch is made against trunk, so I will leave it as is for now.
If Daniel commits his patch for PR35861, I can provide an updated patch.
I quickly looked at it, and it should be rather straightforward (it looks like
a tabs problem only).


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[domob at gcc dot gnu dot org]

------- Comment #12 from domob at gcc dot gnu dot org  2008-10-28 18:46 -------
(In reply to comment #11)
> Well, may patch is made against trunk, so I will leave it as is for now.
> If Daniel commits his patch for PR35861, I can provide an updated patch.
> I quickly looked at it, and it should be rather straightforward (it looks like
> a tabs problem only).

I do think it is a trivial merge, too.  But as discussed with Dominique on IRC,
I think we could also review and check-in your patch prior to mine, as it is
simpler and might get review faster.  Then I'd do the merge.


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[pault at gcc dot gnu dot org]

------- Comment #13 from pault at gcc dot gnu dot org  2008-10-30 11:08 -------
(In reply to comment #11)

> Well, may patch is made against trunk, so I will leave it as is for now.
> If Daniel commits his patch for PR35861, I can provide an updated patch.
> I quickly looked at it, and it should be rather straightforward (it looks like
> a tabs problem only).
> 
Mikael,

Have you commit rights now?  If so, you should assign yourself the PR and post
the patch on the gfortran and gcc-patches lists.

If not, do you want me to do the honours?

Thanks

Paul


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[pault at gcc dot gnu dot org]

------- Comment #14 from pault at gcc dot gnu dot org  2008-10-30 20:47 -------
Subject: Bug 37903

Author: pault
Date: Thu Oct 30 20:45:09 2008
New Revision: 141467

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=141467
Log:
2008-10-30  Mikael Morin  <mikael.morin@tele2.fr>

        PR fortran/37903
        * trans-array.c (gfc_trans_create_temp_array): If n is less
        than the temporary dimension, assert that loop->from is
        zero (reverts to earlier versions). If there is at least one
        null loop->to[n], it is a callee allocated array so set the
        size to NULL and break.
        (gfc_trans_constant_array_constructor): Set the offset to zero.
        (gfc_trans_array_constructor): Remove loop shifting around the
        temporary creation.
        (gfc_conv_loop_setup): Prefer zero-based descriptors if
        possible.  Calculate the translation from loop variables to
        array indices if an array constructor.

2008-10-30  Mikael Morin  <mikael.morin@tele2.fr>

        PR fortran/37749
        * trans-array.c (gfc_trans_create_temp_array): If size is NULL
        use the array bounds for loop->to.

2008-10-30  Mikael Morin  <mikael.morin@tele2.fr>

        PR fortran/37903
        * gfortran.dg/vector_subscript_4.f90: New test.

2008-10-30  Mikael Morin  <mikael.morin@tele2.fr>

        PR fortran/37749
        * gfortran.dg/vector_subscript__5.f90: New test.

Added:
    trunk/gcc/testsuite/gfortran.dg/vector_subscript_4.f90
    trunk/gcc/testsuite/gfortran.dg/vector_subscript_5.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-array.c
    trunk/gcc/testsuite/ChangeLog


-- 


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[pault at gcc dot gnu dot org]

------- Comment #15 from pault at gcc dot gnu dot org  2008-11-01 19:46 -------
Mikael,

Thanks for the report and the fix:-)

Fixed on trunk and 4.3

Paul


-- 

pault at gcc dot gnu dot org changed:

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


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

[Bug fortran/37903] [4.3/4.4 Regression] wrong-code for complicated vector subscripts

[pault at gcc dot gnu dot org]

------- Comment #16 from pault at gcc dot gnu dot org  2008-11-01 19:47 -------
Subject: Bug 37903

Author: pault
Date: Sat Nov  1 19:45:41 2008
New Revision: 141521

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=141521
Log:
2008-11-01  Mikael Morin  <mikael.morin@tele2.fr>

        PR fortran/37903
        * trans-array.c (gfc_trans_create_temp_array): If n is less
        than the temporary dimension, assert that loop->from is
        zero (reverts to earlier versions). If there is at least one
        null loop->to[n], it is a callee allocated array so set the
        size to NULL and break.
        (gfc_trans_constant_array_constructor): Set the offset to zero.
        (gfc_trans_array_constructor): Remove loop shifting around the
        temporary creation.
        (gfc_conv_loop_setup): Prefer zero-based descriptors if
        possible.  Calculate the translation from loop variables to
        array indices if an array constructor.

2008-11-01  Mikael Morin  <mikael.morin@tele2.fr>

        PR fortran/37749
        * trans-array.c (gfc_trans_create_temp_array): If size is NULL
        use the array bounds for loop->to.

2008-11-01  Mikael Morin  <mikael.morin@tele2.fr>

        PR fortran/37903
        * gfortran.dg/vector_subscript_4.f90: New test.

2008-11-01  Mikael Morin  <mikael.morin@tele2.fr>

        PR fortran/37749
        * gfortran.dg/vector_subscript__5.f90: New test.

Added:
    branches/gcc-4_3-branch/gcc/testsuite/gfortran.dg/vector_subscript_4.f90
    branches/gcc-4_3-branch/gcc/testsuite/gfortran.dg/vector_subscript_5.f90
Modified:
    branches/gcc-4_3-branch/gcc/fortran/ChangeLog
    branches/gcc-4_3-branch/gcc/fortran/trans-array.c
    branches/gcc-4_3-branch/gcc/testsuite/ChangeLog


-- 


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