User account creation filtered due to spam.

Bug 40551 - Optimizations possible using gfc_full_array_ref_p
Summary: Optimizations possible using gfc_full_array_ref_p
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.5.0
: P3 major
Target Milestone: ---
Assignee: Paul Thomas
URL:
Keywords: missed-optimization
Depends on:
Blocks: 32834
  Show dependency treegraph
 
Reported: 2009-06-25 13:30 UTC by Tobias Burnus
Modified: 2009-07-09 17:10 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.4.1 4.5.0 4.3.3
Last reconfirmed: 2009-06-25 15:06:14


Attachments
Provisional fix for the PR (1.28 KB, patch)
2009-06-29 15:18 UTC, Paul Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2009-06-25 13:30:03 UTC
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
Comment 1 Tobias Burnus 2009-06-25 13:32:26 UTC
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".)
Comment 2 Dominique d'Humieres 2009-06-25 14:21:54 UTC
> 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.
Comment 3 Tobias Burnus 2009-06-25 15:06:13 UTC
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
Comment 4 Tobias Burnus 2009-06-25 15:43:01 UTC
(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.
Comment 5 Tobias Burnus 2009-06-25 16:04:43 UTC
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.
Comment 6 Paul Thomas 2009-06-28 20:31:46 UTC
(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
Comment 7 Paul Thomas 2009-06-29 09:20:53 UTC
(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
Comment 8 Paul Thomas 2009-06-29 15:18:58 UTC
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
Comment 9 Paul Thomas 2009-06-29 20:39:20 UTC
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

Comment 10 Paul Thomas 2009-06-30 06:24:06 UTC
(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
Comment 11 Paul Thomas 2009-07-05 19:06:18 UTC
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

Comment 12 Tobias Burnus 2009-07-07 12:47:06 UTC
> 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?
Comment 13 Tobias Burnus 2009-07-07 15:27:42 UTC
(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?
Comment 14 Paul Thomas 2009-07-09 17:06:21 UTC
(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
Comment 15 Paul Thomas 2009-07-09 17:10:52 UTC
Sorry - I forgot about 40629.

Closing, closing.... now!

Paul