[PATCH, fortran] Add -frecursive and fix local array handling with -fopenmp

[PATCH, fortran] Add -frecursive and fix local array handling with -fopenmp

[Asher Langton]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

Hi all,

Here's a patch to fix the OpenMP issue with local arrays being
allocated statically, as discussed here:

http://gcc.gnu.org/ml/fortran/2007-08/msg00000.html

I also added an '-frecursive' flag, which sets max_stack_var_size to -1.


-Asher


:ADDPATCH fortran:

2007-08-15  Asher Langton  <langton2@llnl.gov>

        * gfortran.h (gfc_option_t): Add flag_recursive.
	* lang.opt: Add -frecursive option and update -fopenmp.
	* invoke.texi: Document new option.
	* options.c (gfc_init_options, gfc_post_options,
	gfc_handle_option): Add -frecursive and modify -fopenmp.

2007-08-15  Asher Langton  <langton2@llnl.gov>

	* gfortran.dg/recursive_stack.f90: New.
	* gfortran.dg/openmp_stack.f90: New.

[-- Attachment #2: gfc070815_125412.diff --]
[-- Type: application/octet-stream, Size: 3626 bytes --]

Index: gfortran.h
===================================================================
--- gfortran.h	(revision 127522)
+++ gfortran.h	(working copy)
@@ -1867,6 +1867,7 @@ typedef struct
   int flag_openmp;
   int flag_sign_zero;
   int flag_module_private;
+  int flag_recursive;
 
   int fpe;
 
Index: lang.opt
===================================================================
--- lang.opt	(revision 127522)
+++ lang.opt	(working copy)
@@ -218,7 +218,7 @@ Set default accessibility of module enti
 
 fopenmp
 Fortran
-Enable OpenMP
+Enable OpenMP (sets fmax-stack-var-size to be unlimited)
 
 fpack-derived
 Fortran
@@ -240,6 +240,10 @@ frecord-marker=8
 Fortran RejectNegative
 Use an 8-byte record marker for unformatted files
 
+frecursive
+Fortran
+Allocate local variables on the stack to allow indirect recursion
+
 frepack-arrays
 Fortran
 Copy array sections into a contiguous block on procedure entry
Index: invoke.texi
===================================================================
--- invoke.texi	(revision 127522)
+++ invoke.texi	(working copy)
@@ -156,7 +156,7 @@ and warnings}.
 -fsecond-underscore @gol
 -fbounds-check  -fmax-stack-var-size=@var{n} @gol
 -fpack-derived  -frepack-arrays  -fshort-enums  -fexternal-blas @gol
--fblas-matmul-limit=@var{n}}
+-fblas-matmul-limit=@var{n}} -frecursive
 @end table
 
 @menu
@@ -296,7 +296,8 @@ and @code{c$omp}, @code{*$omp} and @code
 @code{!$} conditional compilation sentinels in free form
 and @code{c$}, @code{*$} and @code{!$} sentinels in fixed form, 
 and when linking arranges for the OpenMP runtime library to be linked
-in.
+in.  The @option{-fopenmp} flag will override any setting of
+@option{-fmax-stack-var-size=}.
 
 @item -frange-check
 @opindex @code{frange-check}
@@ -865,7 +866,8 @@ substring references.
 @item -fmax-stack-var-size=@var{n}
 @opindex @code{fmax-stack-var-size}
 This option specifies the size in bytes of the largest array that will be put
-on the stack.
+on the stack.  The @option{-fopenmp} and @option{-frecursive} flags
+can override this setting.
 
 This option currently only affects local arrays declared with constant
 bounds, and may not apply to all character variables.
@@ -919,6 +921,12 @@ geometric mean of the dimensions of the 
 
 The default value for @var{n} is 30.
 
+@item -frecursive
+@opindex @code{frecursive}
+Allow indirect recursion by forcing all local arrays to be allocated
+on the stack.  This flag will override any value set with
+@option{fmax-stack-var-size=}.
+
 @end table
 
 @xref{Code Gen Options,,Options for Code Generation Conventions,
Index: options.c
===================================================================
--- options.c	(revision 127522)
+++ options.c	(working copy)
@@ -103,6 +103,7 @@ gfc_init_options (unsigned int argc ATTR
   gfc_option.flag_d_lines = -1;
   gfc_option.flag_openmp = 0;
   gfc_option.flag_sign_zero = 1;
+  gfc_option.flag_recursive = 0;
 
   gfc_option.fpe = 0;
 
@@ -294,6 +295,11 @@ gfc_post_options (const char **pfilename
   if (!gfc_option.flag_automatic)
     gfc_option.flag_max_stack_var_size = 0;
   
+  /* For OpenMP and indirect recusion, force local variables to
+     be allocated on the stack.  */
+  if (gfc_option.flag_openmp || gfc_option.flag_recursive)
+    gfc_option.flag_max_stack_var_size = -1;
+
   if (pedantic)
     { 
       gfc_option.warn_ampersand = 1;
@@ -698,6 +704,12 @@ gfc_handle_option (size_t scode, const c
 			 MAX_SUBRECORD_LENGTH);
 
       gfc_option.max_subrecord_length = value;
+      break;
+
+    case OPT_frecursive:
+      gfc_option.flag_recursive = 1;
+      break;
+
     }
 
   return result;

[-- Attachment #3: openmp_stack.f90 --]
[-- Type: application/octet-stream, Size: 520 bytes --]

! { dg-do run}
! { dg-options "-fopenmp" }
program openmp_stack
  implicit none
  integer id
  integer ilocs(2)
  integer omp_get_thread_num, foo
  call omp_set_num_threads (2)
!$omp parallel private (id)
  id = omp_get_thread_num() + 1
  ilocs(id) = foo()
!$omp end parallel
  ! Check that the two threads are not sharing a location for
  ! the array x in foo()
  if (ilocs(1) .eq. ilocs(2)) call abort
end program openmp_stack

integer function foo ()
  implicit none
  real x(100,100)
  foo = loc(x)
end function foo

[-- Attachment #4: recursive_stack.f90 --]
[-- Type: application/octet-stream, Size: 404 bytes --]

! { dg-do run}
! { dg-options "-frecursive" }
program recursive_stack
  call foo (.true.)
end program recursive_stack

subroutine foo (recurse)
  logical recurse
  integer iarray(100,100)
  if (recurse) then
     iarray(49,49) = 17
     call bar
     if (iarray(49,49) .ne. 17) call abort
  else
     iarray(49,49) = 21
  end if
end subroutine foo

subroutine bar
  call foo (.false.)
end subroutine bar

Re: [PATCH, fortran] Add -frecursive and fix local array handling with -fopenmp

[Janne Blomqvist]

On 8/15/07, Asher Langton <langton2@llnl.gov> wrote:
> Here's a patch to fix the OpenMP issue with local arrays being
> allocated statically, as discussed here:
>
> http://gcc.gnu.org/ml/fortran/2007-08/msg00000.html
>
> I also added an '-frecursive' flag, which sets max_stack_var_size to -1.

For a slightly modified approach, why not leave the -frecursive option
part of the patch out as unnecessary (one less option to clutter the
manual with :) ), and make flag_max_stack_var_size=-1 the default
rather than the current (arbitrary?) limit of 32768. Those who then
really want some cutoff size to store local variable statically can
then use the -fmax-stack-var-size= option explicitly rather than start
seeing weird effects for when going over the limit like Asher did when
using OpenMP (I guess it's not very obvious why things start to fail
in this case).

And, since running out of stack space due to large arrays on the stack
seems to be much more common in Fortran rather than recursing
infinitely, add something like the following to
libgfortran/runtime/main.c:

#include <sys/time.h>
#include <sys/resource.h>

#ifdef HAVE_GETRLIMIT && HAVE_SETRLIMIT
/*  Try to remove stack space limits.  */
void
set_stack_rlimit()
{
  struct rlimit rlim;
  getrlimit (RLIMIT_STACK, &rlim);
  rlim.rlim_cur = rlim.rlim_max;
  setrlimit (RLIMIT_STACK, &rlim);
  // + some appropriate error handling, checking return values etc.
}
#endif

-- 
Janne Blomqvist

Re: [PATCH, fortran] Add -frecursive and fix local array handling with -fopenmp

[Tobias Burnus]

Hi Janne, Hi Asher, hi all,

Janne Blomqvist wrote:
> On 8/15/07, Asher Langton <langton2@llnl.gov> wrote:
>   
>> I also added an '-frecursive' flag, which sets max_stack_var_size to -1.
> For a slightly modified approach, why not leave the -frecursive option
> part of the patch out as unnecessary [...], and make
> flag_max_stack_var_size=-1 the default
> rather than the current (arbitrary?) limit of 32768. Those who then
> really want some cutoff size to store local variable statically can
> then use the -fmax-stack-var-size= option explicitly rather than start
> seeing weird effects for when going over the limit like Asher did when
> using OpenMP (I guess it's not very obvious why things start to fail
> in this case).

ifort has the options:
-auto-scalar (default): put scalars to the stack
-save: put variables in static memory (except in recursively declared
procedures)
-auto/-automatic: Puts all local, non-SAVE variables on the stack
-openmp/-recursive: Implies -auto/-automatic

gfortran (currently):
-fno-automatic: As Intel's -save: use static memory
-fmax-stack-var-size=<n> stack vs. static memory; incl. n=-1 to put all
on the stack

Actually, the gfortran man page (invoke.texi) is a bit unclear. I
therefore suggest the attach patch (which is independent of Asher's patch).

As stack overflows are hard to debug, I am in favour of keeping
-fmax-stack-var-size=n with n > 0.
(Which in a way even Intel does - though using -auto-scalar their limit
is even lower.)

Note that you only see "weird things" if you have either parallel code
(-fopenmp) which is fixed by Asher's patch or whether you are using
recurse code without specifying RESCURSIVE, which is invalid Fortran.


Regarding Asher's patch, I think it is OK as it.


> And, since running out of stack space due to large arrays on the stack
> seems to be much more common in Fortran rather than recursing
> infinitely, add something like the following to
> libgfortran/runtime/main.c:
>   
I'm not sure whether the users like a program tinker with the ulimit
(soft) limits. I have also to admit that I did not see yet reports of
stack-overflow crashes using gfortran; as ifort uses the stack also for
automatic arrays (gfortran always uses the heap), I saw several reports
for ifort and was bitten by it more than once. If gfortran had
-fmax-stack-var-size=-1 by default, I'm sure we would see more of such
reports as g77 programs often had rather large local arrays.

Taken into account hard limits and that the needed stack size depends on
the program, I think having reasonable error/warning messages is
impossible and except of "unlimited" (which is not always wanted) or the
hard limit (which might be still to low) one has no choice in
runtime/main.c. I would therefore suggest to keep the status quo.

Tobias

PS: A rather nice overview about static vs. heap vs. stack can be found at:
http://softwarecommunity.intel.com/isn/Community/en-US/forums/post/116178.aspx

Index: invoke.texi
===================================================================
--- invoke.texi (Revision 127532)
+++ invoke.texi (Arbeitskopie)
@@ -710,10 +710,12 @@
 @opindex @code{fno-automatic}
 @cindex @code{SAVE} statement
 @cindex statement, @code{SAVE}
-Treat each program unit as if the @code{SAVE} statement was specified for
-every local variable and array referenced in it. Does not affect common
-blocks. (Some Fortran compilers provide this option under the name
-@option{-static}.)
+Treat each program unit (except those marked as RECURSIVE) as if the
+@code{SAVE} statement was specified for every local variable and array
+referenced in it. Does not affect common blocks. (Some Fortran compilers
+provide this option under the name @option{-static} or @option{-save}.)
+The default is @option{-fautomatic} which uses the stack for local
+variables smaller than the value given by @option{-fmax-stack-var-size}.

 @item -ff2c
 @opindex ff2c
@@ -865,7 +867,8 @@
 @item -fmax-stack-var-size=@var{n}
 @opindex @code{fmax-stack-var-size}
 This option specifies the size in bytes of the largest array that will be put
-on the stack.
+on the stack; if the size is exceeded static memory is used (except in
+procedures marked as RECURSIVE). If @var{n}=-1 the stack is always used.

 This option currently only affects local arrays declared with constant
 bounds, and may not apply to all character variables.

Re: [PATCH, fortran] Add -frecursive and fix local array handling with -fopenmp

[Jakub Jelinek]

On Thu, Aug 16, 2007 at 10:50:52AM +0200, Tobias Burnus wrote:
> Note that you only see "weird things" if you have either parallel code
> (-fopenmp) which is fixed by Asher's patch or whether you are using
> recurse code without specifying RESCURSIVE, which is invalid Fortran.
> 
> 
> Regarding Asher's patch, I think it is OK as it.

I don't like that, overriding explicit -fmax-stack-var-size=XXX by -fopenmp
sounds like a very bad idea to me.
-fopenmp certainly can change the default, when -fmax-stack-var-size=XXX
isn't explicitly used, from 32K to -1, but if the user requests particular
cutoff we should honor that.

Silently changing RLIMIT_STACK in libgfortran is IMHO not a good idea,
for OpenMP programs that will not help, because at that point libpthread
is already initialized and thread stacks will have different default size
already anyway, and when Fortran programs spawn other programs it would
cause bad havoc (e.g. recent make decided to do this silly thing and
suddenly several programs started weirdly misbehaving when run from
Makefile as opposed to when run without make - we had to change make
to make sure stack limit is restored before execve).

	Jakub

Re: [PATCH, fortran] Add -frecursive and fix local array handling with -fopenmp

[Asher Langton]

On 8/16/07, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Aug 16, 2007 at 10:50:52AM +0200, Tobias Burnus wrote:
> > Note that you only see "weird things" if you have either parallel code
> > (-fopenmp) which is fixed by Asher's patch or whether you are using
> > recurse code without specifying RESCURSIVE, which is invalid Fortran.
> >
> >
> > Regarding Asher's patch, I think it is OK as it.
>
> I don't like that, overriding explicit -fmax-stack-var-size=XXX by -fopenmp
> sounds like a very bad idea to me.
> -fopenmp certainly can change the default, when -fmax-stack-var-size=XXX
> isn't explicitly used, from 32K to -1, but if the user requests particular
> cutoff we should honor that.

That's reasonable.  The ifort compiler seems to do the same thing, but
issues a warning:

% ifort -save -openmp openmp_stack.f90
ifort: warning: explicit static allocation of locals specified,
overriding OpenMP's implicit auto allocation

When -recursive and -save are specified together, ifort seems to
silently override the -save flag.


-Asher

Re: [PATCH, fortran] Add -frecursive and fix local array handling with -fopenmp

[Asher Langton]

On 8/16/07, Tobias Burnus <burnus@net-b.de> wrote:
> gfortran (currently):
> -fno-automatic: As Intel's -save: use static memory
> -fmax-stack-var-size=<n> stack vs. static memory; incl. n=-1 to put all
> on the stack

As of now, fmax-stack-var-size=<n> doesn't accept n=-1 on the command
line, although setting n=-1 works internally.

Re: [PATCH, fortran] Add -frecursive and fix local array handling with -fopenmp

[Tobias Burnus]

Asher Langton wrote:
> On 8/16/07, Tobias Burnus <burnus@net-b.de> wrote:
>   
>> gfortran (currently):
>> -fno-automatic: As Intel's -save: use static memory
>> -fmax-stack-var-size=<n> stack vs. static memory; incl. n=-1 to put all
>> on the stack
>>     
> As of now, fmax-stack-var-size=<n> doesn't accept n=-1 on the command
> line, although setting n=-1 works internally.
Good point! I should have tested this. I think the following would be an
alternative:

- If @var{n}=-1 the stack is always used.
+ Use @code{-frecursive} to put all variables on the stack.


(depends on the OpenMP/-frecursive patch). Or is it be better to keep
these words and to remove
   RejectNegative
from lang.opt ?

Tobias

Re: [PATCH, fortran] Add -frecursive and fix local array handling with -fopenmp

[Asher Langton]

On 8/17/07, Tobias Burnus <burnus@net-b.de> wrote:
> Asher Langton wrote:
> > On 8/16/07, Tobias Burnus <burnus@net-b.de> wrote:
> >
> >> gfortran (currently):
> >> -fno-automatic: As Intel's -save: use static memory
> >> -fmax-stack-var-size=<n> stack vs. static memory; incl. n=-1 to put all
> >> on the stack
> >>
> > As of now, fmax-stack-var-size=<n> doesn't accept n=-1 on the command
> > line, although setting n=-1 works internally.
> Good point! I should have tested this. I think the following would be an
> alternative:
>
> - If @var{n}=-1 the stack is always used.
> + Use @code{-frecursive} to put all variables on the stack.
>
>
> (depends on the OpenMP/-frecursive patch). Or is it be better to keep
> these words and to remove
>    RejectNegative
> from lang.opt ?

I'm in favor of removing RejectNegative, and then leaving your
documentation as it is.  In that case, I could omit the -frecursive
flag from my patch.

-Asher

Re: [PATCH, fortran] Add -frecursive and fix local array handling with -fopenmp

[François-Xavier Coudert]

> And, since running out of stack space due to large arrays on the stack
> seems to be much more common in Fortran rather than recursing
> infinitely, add something like the following to
> libgfortran/runtime/main.c:
>
> #include <sys/time.h>
> #include <sys/resource.h>
>
> #ifdef HAVE_GETRLIMIT && HAVE_SETRLIMIT
> /*  Try to remove stack space limits.  */
> void
> set_stack_rlimit()

I don't think that's a good idea. People are free to set their stack
limit the way they want, we should honor this and shouldn't try to be
smart about this. If I set a stack limit on my system, it's for a
reason. If someone set it on me without my knowledge, then I can
complain to that someone when things go wrong (someone being either
distro vendor or sysadmin).

FX