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

[Andre Vehreschild <vehre@gmx.de>]

Hi Alessandro,

there are still some violations of the style guide:

contrib/check_GNU_style.sh first_complete_patch_REV2.diff

emits:

Lines should not exceed 80 characters.
154:+  add_sym_2 ("failed_images", GFC_ISYM_FAILED_IMAGES,
CLASS_TRANSFORMATIONAL, ACTUAL_NO, BT_INTEGER, 155:+             dd,
GFC_STD_F2008_TS, gfc_check_failed_images, gfc_simplify_failed_images,
156:+             gfc_resolve_failed_images, "team", BT_INTEGER, di, OPTIONAL,
"kind", BT_INTEGER, di, OPTIONAL); 165:+  add_sym_2 ("image_status",
GFC_ISYM_IMAGE_STATUS, CLASS_ELEMENTAL, ACTUAL_NO, BT_INTEGER,
166:+             di, GFC_STD_F2008_TS, gfc_check_image_status,
gfc_simplify_image_status, 167:+             gfc_resolve_image_status, "image",
BT_INTEGER, di, REQUIRED, "team", BT_INTEGER, di, OPTIONAL);
247:+gfc_resolve_image_status (gfc_expr *f, gfc_expr *image, gfc_expr *team
ATTRIBUTE_UNUSED) 409:+  result = transformational_result (result, 0,
BT_INTEGER, kind->ts.kind, &gfc_current_locus);
420:+gfc_simplify_image_status(gfc_expr *image ATTRIBUTE_UNUSED, gfc_expr *team
ATTRIBUTE_UNUSED) 469:+      gfor_fndecl_caf_failed_images =
gfc_build_library_function_decl_with_spec ( 471:+        pvoid_type_node, 3,
pvoid_type_node, integer_type_node, integer_type_node); 

<snipp> The remainder of the script output needs no fix, because its in Fortran
code.

You should fix the above, where they are not in a Fortran testcases. This
allows people with 80 column terminals to read the whole line without scrolling.

The if in resolve.c at 8837: resolve_failed_image (... is intentional? It is
doing nothing. So do you plan to add more code, or will there never be
anything. If the later I recommend to just put a comment there and remove the
empty if.

There still is no test when -fcoarray=single is used. This shouldn't be so
hard, should it?

Regards,
	Andre

On Mon, 19 Sep 2016 08:30:12 -0700
Alessandro Fanfarillo <fanfarillo.gcc@gmail.com> wrote:

> * PING *
> 
> On Sep 7, 2016 3:01 PM, "Alessandro Fanfarillo" <fanfarillo.gcc@gmail.com>
> wrote:
> 
> > Dear all,
> > the attached patch supports failed images also when -fcoarray=single is
> > used.
> >
> > Built and regtested on x86_64-pc-linux-gnu.
> >
> > Cheers,
> > Alessandro
> >
> > 2016-08-09 5:22 GMT-06:00 Paul Richard Thomas <
> > paul.richard.thomas@gmail.com>:
> > > Hi Sandro,
> > >
> > > As far as I can see, this is OK barring a couple of minor wrinkles and
> > > a question:
> > >
> > > For coarray_failed_images_err.f90 and coarray_image_status_err.f90 you
> > > have used the option -fdump-tree-original without making use of the
> > > tree dump.
> > >
> > > Mikael asked you to provide an executable test with -fcoarray=single.
> > > Is this not possible for some reason?
> > >
> > > Otherwise, this is OK for trunk.
> > >
> > > Thanks for the patch.
> > >
> > > Paul
> > >
> > > On 4 August 2016 at 05:07, Alessandro Fanfarillo
> > > <fanfarillo.gcc@gmail.com> wrote:
> > >> * PING *
> > >>
> > >> 2016-07-21 13:05 GMT-06:00 Alessandro Fanfarillo <
> > fanfarillo.gcc@gmail.com>:
> > >>> Dear Mikael and all,
> > >>>
> > >>> in attachment the new patch, built and regtested on
> > x86_64-pc-linux-gnu.
> > >>>
> > >>> Cheers,
> > >>> Alessandro
> > >>>
> > >>> 2016-07-20 13:17 GMT-06:00 Mikael Morin <morin-mikael@orange.fr>:
> > >>>> Le 20/07/2016 à 11:39, Andre Vehreschild a écrit :
> > >>>>>
> > >>>>> Hi Mikael,
> > >>>>>
> > >>>>>
> > >>>>>>> +  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 ();
> > >>>>>
> > >>>>>
> > >>>>> Be careful, this is not 100% identical in the general case. For older
> > >>>>> gcc version (gcc < 4008) gcc_assert() is mapped to nothing, esp. not
> > to
> > >>>>> an abort(), so the behavior can change. But in this case everything
> > is
> > >>>>> fine, because the patch is most likely not backported.
> > >>>>>
> > >>>> Didn't know about this. The difference seems to be very subtle.
> > >>>> I don't mind much anyway. The original version can stay if preferred,
> > this
> > >>>> was just a suggestion.
> > >>>>
> > >>>> By the way, if the function is inlined in its single caller, the
> > assert or
> > >>>> unreachable statement can be removed, which avoids choosing between
> > them.
> > >>>> That's another suggestion.
> > >>>>
> > >>>>
> > >>>>>>> +
> > >>>>>>> +  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?
> > >>>>>
> > >>>>>
> > >>>>> IMO coarray=single should not generate code here, therefore putting
> > >>>>> everything under the if should to fine.
> > >>>>>
> > >>>> My point was more avoiding generating code for the arguments if they
> > are not
> > >>>> used in the end.
> > >>>> Regarding the -fcoarray=single case, the function returns a result,
> > which
> > >>>> can be used in an expression, so I don't think it will work without
> > at least
> > >>>> hardcoding a fixed value as result in that case.
> > >>>> But even that wouldn't be enough, as the function wouldn't work
> > consistently
> > >>>> with the fail image statement.
> > >>>>
> > >>>>> Sorry for the comments ...
> > >>>>>
> > >>>> Comments are welcome here, as far as I know. ;-)
> > >>>>
> > >>>> Mikael
> > >
> > >
> > >
> > > --
> > > The difference between genius and stupidity is; genius has its limits.
> > >
> > > Albert Einstein
> >


-- 
Andre Vehreschild * Email: vehre ad gmx dot de