[Bug fortran/40551] New: Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[Bug fortran/40551] New: Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[burnus at gcc dot gnu dot org]

Bug reported (with longer test case) by Maciej Zwierzycki
  at http://gcc.gnu.org/ml/fortran/2009-06/msg00254.html


The following program should produce  1   2 -42 -42
                      but it produces 1 -42   2 -42

For
  a(1,:) = func()
gfortran decides to pass result value by reference - and uses an array
descriptor for this (why?):
  func (struct array1_integer(kind=4) & __result)
and
  func (&parm.7);

However, as "func(2)" is an explicit-shaped array, it should be contiguous and
thus for "call sub(func)" simply the address is passed, ignoring the stride:

  sub ((integer(kind=4)[0:] *) parm.4.data);

At some place the copy-in/copy-out must happen. Actually, I had assumed that
already the call to "func" would cause a temporary be created. That's actually
what happens in case of g95:
  SC1 = *func_ ();;

Test case:


integer :: a(2,2)
a = -42
a(1,:) = func()
print *,a
contains
 function func()
   integer :: func(2)
   call sub(func)
 end function func
 subroutine sub(a)
   integer :: a(2)
   a = [1,2]
 end subroutine
end


-- 
           Summary: Wrong code due to missing copy-in/copy-out stried array
                    to assumed-size dummy
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: major
          Priority: P3
         Component: fortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: burnus at gcc dot gnu dot org
OtherBugsDependingO 40443
             nThis:


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[burnus at gcc dot gnu dot org]

------- Comment #1 from burnus at gcc dot gnu dot org  2009-06-25 13:32 -------
Fails with 4.3.x to 4.5.0
(In 4.1.x/4.2.x it also fails, but there no return value is set at all, i.e.
one gets "warning: Function does not return a value" - and four times "-42".)


-- 

burnus at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |4.4.1 4.5.0 4.3.3


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[dominiq at lps dot ens dot fr]

------- Comment #2 from dominiq at lps dot ens dot fr  2009-06-25 14:21 -------
> The following program should produce  1   2 -42 -42
>                       but it produces 1 -42   2 -42

You probably mean the opposite! If so, I confirm the problem.


-- 


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[burnus at gcc dot gnu dot org]

------- Comment #3 from burnus at gcc dot gnu dot org  2009-06-25 15:06 -------
Another example. The following two subroutines are essentially identical,
except that one has an explicit interface and one an implicit interface. The
only extra information the explicit interface provides is that the function
takes no arguments.

If one looks at the dumps, one finds:
  D.1548 = func ();
and
  func (&parm.1);
which is surely incompatible.

subroutine test1()
  integer :: array(2)
  external :: func
  integer :: func
  array = func()
end subroutine test1

subroutine test2
 integer :: array2(2)
 interface
   function func()
     integer :: func(2)
   end function func
 end interface
  array2 = func()
end subroutine test2


-- 

burnus at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-06-25 15:06:14
               date|                            |


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[burnus at gcc dot gnu dot org]

------- Comment #4 from burnus at gcc dot gnu dot org  2009-06-25 15:43 -------
(In reply to comment #3)
> Another example.

Which is invalid. Mea culpa:
"A procedure ... shall have an explicit interface if ... (3) The procedure has
a result that (a) is an array"

(That is something, -fwhole-file should be able to catch; currently it just
gives an ICE. I failed to find the restriction to specify dimension in F77, but
presumably I simply looked at the wrong place.)


I see three possibilities:

a) Returning a pointer instead of passing by the _result array descriptor by
reference. (As g95 does. Fix in caller+callee)
b) Doing the copy-in/copy-out in the called function (fix in callee)
c) Passing an array descriptor, but one which is contiguous (fix in caller)
d) Variant of A+C: Just pass a pointer and not an array descriptor the function

Notes:
(a) Means that there is only a single copy out. It will break the ABI, but
that's anyway planned.
(b) Means potentially many copy-in/copy-out. This might not be intended (some
people use automatic arrays vs. assumed-shape arrays on purpose to decide
whether a contiguous array or strides should be used - depending on the size
and what the function does, either is faster).
(c) Is a compromise - there is also only a single copy out.
(d) Is a variant of (a) and (c); it save some space but I think only for little
gain.


-- 


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[burnus at gcc dot gnu dot org]

------- Comment #5 from burnus at gcc dot gnu dot org  2009-06-25 16:04 -------
I think I would go for option (b) of creating a new array descriptor, which is
then used for copy out. The place where currently the creation of a temporary
is prevented is:
  gfc_trans_arrayfunc_assign

If the return symbol is assumed shape, deferred shape (pointer, allocatable) or
the actual is known to be contiguous (whole array, which is not an
assumed-shape/deferred-shape dummy) one can continue as is, otherwise one needs
to pack the arrays. (Todo: Check that no copy in happens.)

At some point, we need a contiguous check - also for F2008's CONTIGUOUS
attribute.


-- 


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[pault at gcc dot gnu dot org]

------- Comment #6 from pault at gcc dot gnu dot org  2009-06-28 20:31 -------
(In reply to comment #5)
The following fixes the problem.  It needs to be checked to see if it is
over-restrictive.

Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c    (revision 149003)
+++ gcc/fortran/trans-expr.c    (working copy)
@@ -4818,7 +4823,8 @@
   tree tmp;

   /* Special case a single function returning an array.  */
-  if (expr2->expr_type == EXPR_FUNCTION && expr2->rank > 0)
+  if (expr2->expr_type == EXPR_FUNCTION && expr2->rank > 0
+       && !(expr1->ref && !gfc_full_array_ref_p (expr1->ref)))
     {
       tmp = gfc_trans_arrayfunc_assign (expr1, expr2);
       if (tmp)

Cheers

Paul


-- 


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[pault at gcc dot gnu dot org]

------- Comment #7 from pault at gcc dot gnu dot org  2009-06-29 09:20 -------
(In reply to comment #6)
> (In reply to comment #5)
> The following fixes the problem.  It needs to be checked to see if it is
> over-restrictive.

Yes it is - there is no need for this when the lhs section is contiguous.

I have modified gfc_full_array_ref_p to return a pointer to a boolean, which
indicates if the reference is contiguous or not. This works correctly with the
testcase and variants.

I will regtest and post to the list tonight.

Cheers

Paul


-- 


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[pault at gcc dot gnu dot org]

------- Comment #8 from pault at gcc dot gnu dot org  2009-06-29 15:18 -------
Created an attachment (id=18092)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=18092&action=view)
Provisional fix for the PR

This is not yet regtested but will be in an hour or two.

I must also run through the references to gfc_full_array_ref_p to see if any of
them could use the 'contiguous' output.

Cheers

Paul


-- 

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


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[pault at gcc dot gnu dot org]

------- Comment #9 from pault at gcc dot gnu dot org  2009-06-29 20:39 -------
Subject: Bug 40551

Author: pault
Date: Mon Jun 29 20:38:59 2009
New Revision: 149062

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

        PR fortran/40551
        * dependency.h : Add second bool* argument to prototype of
        gfc_full_array_ref_p.
        * dependency.c (gfc_full_array_ref_p): If second argument is
        present, return true if last dimension of reference is an
        element or has unity stride.
        * trans-array.c : Add NULL second argument to references to
        gfc_full_array_ref_p.
        * trans-expr.c : The same, except for;
        (gfc_trans_arrayfunc_assign): Return fail if lhs reference
        is not a full array or a contiguous section.

2009-06-29  Paul Thomas  <pault@gcc.gnu.org>

        PR fortran/40551
        * gfortran.dg/func_assign_2.f90 : New test.

Added:
    trunk/gcc/testsuite/gfortran.dg/func_assign_2.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/dependency.c
    trunk/gcc/fortran/dependency.h
    trunk/gcc/fortran/trans-array.c
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog


-- 


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[pault at gcc dot gnu dot org]

------- Comment #10 from pault at gcc dot gnu dot org  2009-06-30 06:24 -------
(In reply to comment #9)
> Subject: Bug 40551

Tobias, 

You were right - contiguous does need initializing.  Not for this case but some
of the other uses that I referred to.

Cheers

Paul


-- 


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[pault at gcc dot gnu dot org]

------- Comment #11 from pault at gcc dot gnu dot org  2009-07-05 19:06 -------
Subject: Bug 40551

Author: pault
Date: Sun Jul  5 19:06:05 2009
New Revision: 149261

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=149261
Log:
2009-07-05  Paul Thomas  <pault@gcc.gnu.org>

        PR fortran/40551
        * dependency.h : Add second bool* argument to prototype of
        gfc_full_array_ref_p.
        * dependency.c (gfc_full_array_ref_p): If second argument is
        present, return true if last dimension of reference is an
        element or has unity stride.
        * trans-array.c : Add NULL second argument to references to
        gfc_full_array_ref_p.
        * trans-expr.c : The same, except for;
        (gfc_trans_arrayfunc_assign): Return fail if lhs reference
        is not a full array or a contiguous section.

2009-07-05  Paul Thomas  <pault@gcc.gnu.org>

        PR fortran/40551
        * gfortran.dg/func_assign_2.f90 : New test.

Added:
    branches/gcc-4_4-branch/gcc/testsuite/gfortran.dg/func_assign_2.f90
Modified:
    branches/gcc-4_4-branch/gcc/fortran/ChangeLog
    branches/gcc-4_4-branch/gcc/fortran/dependency.c
    branches/gcc-4_4-branch/gcc/fortran/dependency.h
    branches/gcc-4_4-branch/gcc/fortran/trans-array.c
    branches/gcc-4_4-branch/gcc/fortran/trans-expr.c
    branches/gcc-4_4-branch/gcc/testsuite/ChangeLog


-- 


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[burnus at gcc dot gnu dot org]

------- Comment #12 from burnus at gcc dot gnu dot org  2009-07-07 12:47 -------
> You were right - contiguous does need initializing.  Not for this case but
> some of the other uses that I referred to.

Paul, what is the plan? You had a 4.4 and 4.5 check in, contiguous is not
initialized in 4.5. Do you want to fix that? Or can this PR be closed?


-- 


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

[Bug fortran/40551] Wrong code due to missing copy-in/copy-out stried array to assumed-size dummy

[burnus at gcc dot gnu dot org]

------- Comment #13 from burnus at gcc dot gnu dot org  2009-07-07 15:27 -------
(In reply to comment #12)
> > You were right - contiguous does need initializing.  Not for this case but
> > some of the other uses that I referred to.
> Paul, what is the plan? You had a 4.4 and 4.5 check in, contiguous is not
> initialized in 4.5. Do you want to fix that?

I somehow missed that it was checked in as part of PR 40646,
http://gcc.gnu.org/viewcvs?view=rev&revision=149262.

Thus: I think this PR be closed, can it?


-- 


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

[Bug fortran/40551] Optimizations possible using gfc_full_array_ref_p

[pault at gcc dot gnu dot org]

------- Comment #14 from pault at gcc dot gnu dot org  2009-07-09 17:06 -------
(In reply to comment #13)

> Thus: I think this PR be closed, can it?
> 

Tobias,

I was keeping it open for comment #8.  I already built a patch for zero,
array-to-array and constant constructor assignments.  Unfortunately it caused
some breakages so it is sat on one side until I do some of the other more
urgent work.

I have changed the title to better reflect what is to come:-)

Paul


-- 

pault at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|wrong-code                  |missed-optimization
            Summary|Wrong code due to missing   |Optimizations possible using
                   |copy-in/copy-out stried     |gfc_full_array_ref_p
                   |array to assumed-size dummy |


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

[Bug fortran/40551] Optimizations possible using gfc_full_array_ref_p

[pault at gcc dot gnu dot org]

------- Comment #15 from pault at gcc dot gnu dot org  2009-07-09 17:10 -------
Sorry - I forgot about 40629.

Closing, closing.... now!

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=40551