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

[Alessandro Fanfarillo <fanfarillo dot gcc at gmail dot com>]

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

Attachment: first_complete_patch_REV2.diff
Description: Text document