Re: [Patch, Fortran] PR98301 Re: RANDOM_INIT() and coarray Fortran

[Steve Kargl <sgk@troutmask.apl.washington.edu>]

Andre,

Thanks for taking care of OpenCoarray portion of RANDOM_INIT.
My last non-coarray aware patch is attached to the PR in bugzilla.
Since the change over to git, I no longer commit to the source tree.
I suggest combining your patch with my patch if you intend to 
commit; otherwise, attach your patch to the PR and sit patiently
until someone can do the combined commit.

-- 
steve  


On Fri, Apr 23, 2021 at 06:43:57PM +0200, Andre Vehreschild wrote:
> Hi folks,
> 
> please find attached the library part for supporting RANDOM_INIT for
> coarray=lib enabled fortran translations. There is also a patch for the
> Opencoarray library to add RANDOM_INIT there.
> 
> I am not sure, whether I have modified the gfortran.map file in the libgfortran
> directory correctly, or even had to. So please take specific care if that is
> correct.
> 
> Bootstrapped and regtested fine on x86_64/f33.
> 
> Ok for trunk?
> 
> Regards,
> 	Andre
> 
> PR fortran/98301 - random_init() is broken
> 
> Correct implementation of random_init() when -fcoarray=lib is given.
> 
> gcc/fortran/ChangeLog:
> 
> 	PR fortran/98301
> 	* trans-decl.c (gfc_build_builtin_function_decls): Move decl.
> 	* trans-intrinsic.c (conv_intrinsic_random_init): Use bool for
> 	lib-call of caf_random_init instead of logical (4-byte).
> 
> libgfortran/ChangeLog:
> 
> 	PR fortran/98301
> 	* caf/libcaf.h (_gfortran_caf_random_init): New function.
> 	* caf/single.c (_gfortran_caf_random_init): New function.
> 	* gfortran.map: Added fndecl.
> 	* intrinsics/random_init.f90: Modified comment.
> 
> 
> 
> On Sat, 3 Apr 2021 22:33:31 -0700
> Steve Kargl via Fortran <fortran@gcc.gnu.org> wrote:
> 
> > On Sat, Apr 03, 2021 at 08:30:31PM -0700, Damian Rouson wrote:
> > > Hi Steve,
> > >
> > > I hope the gfortran developers won't commit a patch that replaces
> > > existing behavior with a stub that simply emits an error message.
> >
> > The current behavior is incorrect with respect to the Fortran standard
> > if one runs a compiled program multiple times.  The patch fixes a bug.
> >
> > > It has been a while since I looked at the previous emails regarding
> > > problems with the current behavior so I'm not expressing an opinion
> > > about the current behavior.  I'm simply advocating against breaking
> > > existing codes that rely on the current behavior if nothing better is
> > > being provided.
> >
> > It cannot be helped.  The underlying coarray implementation must
> > handle RANDOM_INIT().  AFAICT, there must be communication between
> > images if RANDOM_INIT() is used.  libgfortran is not compiled with
> > -fcoarray=lib (or -fcoarray=shared), so the coarray implementation(s)
> > must deal with RANDOM_INIT().
> >
> > > Or as a compromise, would you mind changing the patch so that the
> > > error message is emitted only in the problematic cases? You presented
> > > one problematic case out of the four possible combinations of
> > > RANDOM_INIT() arguments.  With only four possible combinations to
> > > enumerate, I hope this suggestion isn't burdensome.
> > Consider,
> >
> >    read(*,*) repeatable, image_distinct
> >    call random_init(repeatable, image_distinct)
> >
> > for -fcoarray=none or -fcoarray=single, the image_distinct
> > argument is irrevelant.   This is simply translated to
> >
> > _gfortran_random_init(repeatable, image_distinct)
> >
> > For -fcoarray=lib (and by extension OpenCoarray), a simple 1 line
> > patch on top of my patch would generate
> >
> > _gfortran_caf_random_init(repeatable, image_distinct)
> >
> > where is it assumed the function is coarray aware.  Similarly, if
> > -fcoarray=shared, then a 1 line patch would generate
> >
> > _gfortran_cas_random_init(repeatable, image_distinct)
> >
> > where is it assumed the function is coarray aware.
> >
> > There are 4 cases:
> > (1)   repeatable=.true., image_distinct=.true.
> > (2)   repeatable=.true., image_distinct=.false.
> > (3)   repeatable=.false., image_distinct=.true.
> > (4)   repeatable=.false., image_distinct=.false.
> >
> > IIRC, cases (1)-(3) don't require communication.  case (4) does.
> > That is, case (4) requires the same set of random seeds to be
> > used on all images.
> >
> > > Regarding "someone who cares about coarray Fortran," finding people to
> > > work on such an effort is quite challenging.
> >
> > Finding people willing to work on gfortran is as challenging if
> > not more so.  A year ago, I posted in c.l.f a list of 20+ PRs
> > with patches that I had created.  I don't do git.  It would take
> > someone with git-fu little time to take my patches and apply
> > them to a source tree.  Testing was done by me, but I would
> > encourage git-fu aware individuals to bootstrap and test the patches.
> > Then commit the patch.  Harald has stepped up and grabbed a few.
> > TO BE CLEAR, I AM NOT RANTING AT THE PEOPLE WHO HAVE CONTRIBUTED
> > AND MAINTAINED GFORTRAN FOR YEARS.  gfortran needs new blood, or
> > it is destined for the bit bucket.
> >
> > > I believe Andre
> > > Verheschild has some limited availability so I'm cc'ing him and will
> > > discuss it with him if he's interested.  If you know others who might
> > > be interested, please let us know.
> >
> > Don't know any new people who are interested in gfortran.
> >
> 
> 
> --
> Andre Vehreschild * Email: vehre ad gmx dot de

> diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c
> index c99336cb19d..de0e8fb0314 100644
> --- a/gcc/fortran/trans-decl.c
> +++ b/gcc/fortran/trans-decl.c
> @@ -170,6 +170,7 @@ tree gfor_fndecl_co_min;
>  tree gfor_fndecl_co_reduce;
>  tree gfor_fndecl_co_sum;
>  tree gfor_fndecl_caf_is_present;
> +tree gfor_fndecl_caf_random_init;
> 
> 
>  /* Math functions.  Many other math functions are handled in
> @@ -234,7 +235,6 @@ tree gfor_fndecl_zgemm;
> 
>  /* RANDOM_INIT function.  */
>  tree gfor_fndecl_random_init;      /* libgfortran, 1 image only.  */
> -tree gfor_fndecl_caf_random_init;  /* OpenCoarray call.  */
>  tree gfor_fndecl_cas_random_init;  /* Shared memory coarray.  */
> 
>  static void
> @@ -3517,23 +3517,17 @@ gfc_build_intrinsic_function_decls (void)
>  	void_type_node, 3, gfc_logical4_type_node, gfc_logical4_type_node,
>  	gfc_int4_type_node);
> 
> + // gfor_fndecl_caf_rand_init is defined in the lib-coarray section below.
> +
>  /* FIXME: This is a temporary workaround until someone that uses coarray
>     Fortran and random_init() can implement the OpenCoarray and/or the
>     shared memory routines.  Both require communication between images, so
>     these routines cannot live in libgfortran.  */
>  #if 1
> -  gfor_fndecl_caf_random_init = gfc_build_library_function_decl (
> -	get_identifier (PREFIX("random_init_foobar")),
> -	void_type_node, 2, gfc_logical4_type_node, gfc_logical4_type_node);
> -
>    gfor_fndecl_cas_random_init = gfc_build_library_function_decl (
>  	get_identifier (PREFIX("random_init_foobar")),
>  	void_type_node, 2, gfc_logical4_type_node, gfc_logical4_type_node);
>  #else
> -  gfor_fndecl_caf_random_init = gfc_build_library_function_decl (
> -	get_identifier (PREFIX("caf_random_init")),
> -	void_type_node, 2, gfc_logical4_type_node, gfc_logical4_type_node);
> -
>    gfor_fndecl_cas_random_init = gfc_build_library_function_decl (
>  	get_identifier (PREFIX("cas_random_init")),
>  	void_type_node, 2, gfc_logical4_type_node, gfc_logical4_type_node);
> @@ -4104,6 +4098,10 @@ gfc_build_builtin_function_decls (void)
>  	get_identifier (PREFIX("caf_is_present")), ". r . r ",
>  	integer_type_node, 3, pvoid_type_node, integer_type_node,
>  	pvoid_type_node);
> +
> +      gfor_fndecl_caf_random_init = gfc_build_library_function_decl (
> +	    get_identifier (PREFIX("caf_random_init")),
> +	    void_type_node, 2, logical_type_node, logical_type_node);
>      }
> 
>    gfc_build_intrinsic_function_decls ();
> diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c
> index 900c6198d9e..8319e94b893 100644
> --- a/gcc/fortran/trans-intrinsic.c
> +++ b/gcc/fortran/trans-intrinsic.c
> @@ -3828,27 +3828,27 @@ conv_intrinsic_random_init (gfc_code *code)
>    stmtblock_t block;
>    gfc_se se;
>    tree arg1, arg2, tmp;
> -  tree logical4_type_node = gfc_get_logical_type (4);
> +  /* On none coarray == lib compiles use LOGICAL(4) else regular LOGICAL.  */
> +  tree used_bool_type_node = flag_coarray == GFC_FCOARRAY_LIB
> +			     ? logical_type_node
> +			     : gfc_get_logical_type (4);
> 
>    /* Make the function call.  */
>    gfc_init_block (&block);
>    gfc_init_se (&se, NULL);
> 
> -  /* Convert REPEATABLE to a LOGICAL(4) entity.  */
> +  /* Convert REPEATABLE to the desired LOGICAL entity.  */
>    gfc_conv_expr (&se, code->ext.actual->expr);
>    gfc_add_block_to_block (&block, &se.pre);
> -  arg1 = fold_convert (logical4_type_node, gfc_evaluate_now (se.expr, &block));
> +  arg1 = fold_convert (used_bool_type_node, gfc_evaluate_now (se.expr, &block));
>    gfc_add_block_to_block (&block, &se.post);
> 
> -  /* Convert IMAGE_DISTINCT to a LOGICAL(4) entity.  */
> +  /* Convert IMAGE_DISTINCT to the desired LOGICAL entity.  */
>    gfc_conv_expr (&se, code->ext.actual->next->expr);
>    gfc_add_block_to_block (&block, &se.pre);
> -  arg2 = fold_convert (logical4_type_node, gfc_evaluate_now (se.expr, &block));
> +  arg2 = fold_convert (used_bool_type_node, gfc_evaluate_now (se.expr, &block));
>    gfc_add_block_to_block (&block, &se.post);
> 
> -  /* Create the hidden argument.  For non-coarray codes and -fcoarray=single,
> -     simply set this to 0.  For -fcoarray=lib, generate a call to
> -     THIS_IMAGE() without arguments.  */
>    if (flag_coarray == GFC_FCOARRAY_LIB)
>      {
>        tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_random_init,
> diff --git a/libgfortran/caf/libcaf.h b/libgfortran/caf/libcaf.h
> index 5abb753f6fd..c66d0379042 100644
> --- a/libgfortran/caf/libcaf.h
> +++ b/libgfortran/caf/libcaf.h
> @@ -261,4 +261,6 @@ void _gfortran_caf_stopped_images (gfc_descriptor_t *,
> 
>  int _gfortran_caf_is_present (caf_token_t, int, caf_reference_t *);
> 
> +void _gfortran_caf_random_init (bool, bool);
> +
>  #endif  /* LIBCAF_H  */
> diff --git a/libgfortran/caf/single.c b/libgfortran/caf/single.c
> index a291c4452c9..fc8e3b3b94a 100644
> --- a/libgfortran/caf/single.c
> +++ b/libgfortran/caf/single.c
> @@ -3135,3 +3135,13 @@ _gfortran_caf_is_present (caf_token_t token,
>      }
>    return memptr != NULL;
>  }
> +
> +/* Reference the libraries implementation.  */
> +extern void _gfortran_random_init (int32_t, int32_t, int32_t);
> +
> +void _gfortran_caf_random_init (bool repeatable, bool image_distinct)
> +{
> +  /* In a single image implementation always forward to the gfortran
> +     routine.  */
> +  _gfortran_random_init (repeatable, image_distinct, 1);
> +}
> diff --git a/libgfortran/gfortran.map b/libgfortran/gfortran.map
> index b594b59849a..626804c37dd 100644
> --- a/libgfortran/gfortran.map
> +++ b/libgfortran/gfortran.map
> @@ -1634,3 +1634,8 @@ GFORTRAN_11 {
>    global:
>    _gfortran_random_init_foobar;
>  } GFORTRAN_10.2;
> +
> +GFORTRAN_12 {
> +  global:
> +  _gfortran_caf_random_init;
> +} GFORTRAN_11;
> diff --git a/libgfortran/intrinsics/random_init.f90 b/libgfortran/intrinsics/random_init.f90
> index bbb45937087..233a1ca073a 100644
> --- a/libgfortran/intrinsics/random_init.f90
> +++ b/libgfortran/intrinsics/random_init.f90
> @@ -100,7 +100,7 @@ impure subroutine _gfortran_random_init(repeatable, image_distinct, image_num)
>  end subroutine _gfortran_random_init
>  !
>  ! This is a temporary stub implementation until random_init is
> -! implemented for -fcoarray=shared and -fcoarray=lib.
> +! implemented for -fcoarray=shared.
>  !
>  subroutine _gfortran_random_init_foobar(repeatable, image_distinct)
> 


-- 
Steve