Re: [Fortran, Patch] First patch for coarray FAILED IMAGES (TS 18508)

[Mikael Morin <morin-mikael at orange dot fr>]

Hello,

this is mostly good in general, but is lacking tests.
Especially, tests for successfull matching, and tests for every error 
you are adding in the patch (except maybe the -fcoarray= one).
Also tests that the code executes successfullly with -fcoarray=single, 
and that it produces the right function calls with -fcoarray=lib.

more specific comments below.

Mikael

Le 15/07/2016 à 19:34, Alessandro Fanfarillo a écrit :
> Third *PING*
>
> 2016-07-04 16:46 GMT-06:00 Alessandro Fanfarillo <fanfarillo.gcc@gmail.com>:
> > * PING *
> >
> > 2016-06-21 10:59 GMT-06:00 Alessandro Fanfarillo <fanfarillo.gcc@gmail.com>:
> > > * PING *
> > >
> > > 2016-06-06 15:05 GMT-06:00 Alessandro Fanfarillo <fanfarillo.gcc@gmail.com>:
> > > > Dear all,
> > > >
> > > > please find in attachment the first patch (of n) for the FAILED IMAGES
> > > > capability defined in the coarray TS 18508.
> > > > The patch adds support for three new intrinsic functions defined in
> > > > the TS for simulating a failure (fail image), checking an image status
> > > > (image_status) and getting the list of failed images (failed_images).
> > > > The patch has been built and regtested on x86_64-pc-linux-gnu.
> > > >
> > > > Ok for trunk?
> > > >
> > > > Alessandro
>
> first_complete_patch.diff
>
> commit b3bca5b09f4cbcf18f2409dae2485a16a7c06498
> Author: Alessandro Fanfarillo <fanfarillo@ing.uniroma2.it>
> Date:   Mon Jun 6 14:27:37 2016 -0600
>
>     First patch Failed Images CAF TS-18508
>
> diff --git a/gcc/fortran/match.c b/gcc/fortran/match.c
> index f3a4a43..9f519ff 100644
> --- a/gcc/fortran/match.c
> +++ b/gcc/fortran/match.c
> @@ -1594,6 +1594,7 @@ gfc_match_if (gfc_statement *if_type)
>    match ("event post", gfc_match_event_post, ST_EVENT_POST)
>    match ("event wait", gfc_match_event_wait, ST_EVENT_WAIT)
>    match ("exit", gfc_match_exit, ST_EXIT)
> +  match ("fail image", gfc_match_fail_image, ST_FAIL_IMAGE)
>    match ("flush", gfc_match_flush, ST_FLUSH)
>    match ("forall", match_simple_forall, ST_FORALL)
>    match ("go to", gfc_match_goto, ST_GOTO)
> @@ -3073,6 +3074,41 @@ gfc_match_event_wait (void)
>    return event_statement (ST_EVENT_WAIT);
>  }
>
> +/* Match a FAIl IMAGE statement */
> +
> +static match
> +fail_image_statement (gfc_statement st)
> +{
> +  if (flag_coarray == GFC_FCOARRAY_NONE)
> +    {
> +      gfc_fatal_error ("Coarrays disabled at %C, use %<-fcoarray=%> to enable");
> +      return MATCH_ERROR;
> +    }
> +
> +  if (gfc_match_char ('(') == MATCH_YES)
> +    goto syntax;
> +
> +  if(st == ST_FAIL_IMAGE)
> +    new_st.op = EXEC_FAIL_IMAGE;
> +  else
> +    gcc_unreachable();
You can use
	gcc_assert (st == ST_FAIL_IMAGE);
	foo...;
instead of
	if (st == ST_FAIL_IMAGE)
		foo...;
	else
		gcc_unreachable ();
> +
> +  return MATCH_YES;
> +
> + syntax:
> +  gfc_syntax_error (st);
> +
> +  return MATCH_ERROR;
> +}
> +
> +match
> +gfc_match_fail_image (void)
> +{
> +  /* if (!gfc_notify_std (GFC_STD_F2008_TS, "FAIL IMAGE statement at %C")) */
> +  /*   return MATCH_ERROR; */
> +
Can this be uncommented?

> +  return fail_image_statement (ST_FAIL_IMAGE);
> +}
>
>  /* Match LOCK/UNLOCK statement. Syntax:
>       LOCK ( lock-variable [ , lock-stat-list ] )
> diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c
> index 1aaf4e2..b2f5596 100644
> --- a/gcc/fortran/trans-intrinsic.c
> +++ b/gcc/fortran/trans-intrinsic.c
> @@ -1647,6 +1647,24 @@ trans_this_image (gfc_se * se, gfc_expr *expr)
>  					       m, lbound));
>  }
>
> +static void
> +gfc_conv_intrinsic_image_status (gfc_se *se, gfc_expr *expr)
> +{
> +  unsigned int num_args;
> +  tree *args,tmp;
> +
> +  num_args = gfc_intrinsic_argument_list_length (expr);
> +  args = XALLOCAVEC (tree, num_args);
> +
> +  gfc_conv_intrinsic_function_args (se, expr, args, num_args);
> +
> +  if (flag_coarray == GFC_FCOARRAY_LIB)
> +    {
Can everything be put under the if?
Does it work with -fcoarray=single?

> +      tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_image_status, 2,
> +  				 args[0], build_int_cst (integer_type_node, -1));
> +      se->expr = tmp;
> +    }
> +}
>
>  static void
> diff --git a/gcc/fortran/trans-stmt.c b/gcc/fortran/trans-stmt.c
> index 7d3cf8c..ce0eae7 100644
> --- a/gcc/fortran/trans-stmt.c
> +++ b/gcc/fortran/trans-stmt.c
> @@ -674,6 +674,31 @@ gfc_trans_stop (gfc_code *code, bool error_stop)
>    return gfc_finish_block (&se.pre);
>  }
>
> +/* Translate the FAIL IMAGE statement.  We have to translate this statement
> +   to a runtime library call.  */
> +
> +tree
> +gfc_trans_fail_image (gfc_code *code ATTRIBUTE_UNUSED)
> +{
> +  tree gfc_int4_type_node = gfc_get_int_type (4);
> +  gfc_se se;
> +  tree tmp;
> +
> +  /* Start a new block for this statement.  */
> +  gfc_init_se (&se, NULL);
> +  gfc_start_block (&se.pre);
> +
> +  tmp = build_int_cst (gfc_int4_type_node, 0);
This tmp doesn't seem to be used.

> +  tmp = build_call_expr_loc (input_location,
> +			     gfor_fndecl_caf_fail_image, 1,
> +			     build_int_cst (pchar_type_node, 0));
> +
> +  gfc_add_expr_to_block (&se.pre, tmp);
> +
> +  gfc_add_block_to_block (&se.pre, &se.post);
> +
> +  return gfc_finish_block (&se.pre);
> +}
>
>  tree
>  gfc_trans_lock_unlock (gfc_code *code, gfc_exec_op op)