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
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".)
> 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.
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
(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.
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.
(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
(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
Created attachment 18092 [details] 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
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
(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
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
> 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?
(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?
(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
Sorry - I forgot about 40629. Closing, closing.... now! Paul