From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 93658 invoked by alias); 19 Sep 2016 15:55:26 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 93629 invoked by uid 89); 19 Sep 2016 15:55:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.1 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=gcc_unreachable, gmx, vehreschild, Match X-Spam-User: qpsmtpd, 2 recipients X-HELO: mout.gmx.net Received: from mout.gmx.net (HELO mout.gmx.net) (212.227.15.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 19 Sep 2016 15:55:15 +0000 Received: from vepi2 ([84.63.206.51]) by mail.gmx.com (mrgmx002) with ESMTPSA (Nemesis) id 0MXr3H-1bS3zC2MsR-00Wq3d; Mon, 19 Sep 2016 17:55:05 +0200 Date: Mon, 19 Sep 2016 15:58:00 -0000 From: Andre Vehreschild To: Alessandro Fanfarillo Cc: Paul Richard Thomas , gfortran , gcc-patches , Mikael Morin , Tobias Burnus Subject: Re: [Fortran, Patch] First patch for coarray FAILED IMAGES (TS 18508) Message-ID: <20160919175505.09db48b0@vepi2> In-Reply-To: References: <20160720113913.24e1f404@vepi2> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-UI-Out-Filterresults: notjunk:1;V01:K0:Q5rG6jFptjY=:EzhRKV5pepsch6hsfCxICI uF6+qT3CDuupLot+h8fK7IM4ar2kdzbRmbyWZIiQrrnFfcbkHToS/bfXDQTXFEDG0NmoZ5dGh JpO7WjDvsl9rHZ0ZYvCURQ/hBP9OkOd9CURNivZNQfOYx1O8Xx9+MD68m9+k1/jRb5NPYzlxx LfAKGMN1mAlHSm2tf/ic89O61y+kfnBT7GxsZYukWsT0zuJjEk30dqJCCK3tlN0LIANHkEUYk DMo8qDfaoalg8E6cQumV9LKNgyc+WVnUkg3Q4VKOlaxnP81++Huqkdl+ufDkH/jOHx9dD1fVk 9+1pB1/vXV+Pk7bJHyimEHSKCePt5dl2edtUsildOqIR1oYgy/7GrtBL8fugELbgyS32ooxP4 FBMLA4h6US6szZzV0tkAVFcYmnghmOqF1rRgwZ25uCcNrSqA304e5xMY+x5KgNYkwvW4oOg5h 1Thak57wCEgsuRLwbIL3mR9Ra8T+PC7fBnYKyEWHUSomtuR9NICULR1XGH9MQXCDV1pKaPyGG zjxV3LgGHF3XGWAZ07mvqG2J5VHdjFN5R7cxnIwg5nh9wQx4YPhS2oharjh1I+zxlEkdGqsUs ht/B8PhCdYmTNjKZ4PQ/edWp+bQIJSET60R8L1siY1/cev+nnOE+Y/EAz3SVXqSFX89O9By5F MV0qq5bvGUEKj5rUD9x7X1rbJWBneuCcip1g6nPFEh9G7Amy0OKpFRYR2aaKhN6r50uYuq+78 /CXiBo057iKyqcmwjJeGKov/ekPvYYE71eFKX/B29Jbizb6FMvQLjM2ql6472uyYe1CoJh+Z+ QHX6Rw0 X-SW-Source: 2016-09/txt/msg01183.txt.bz2 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, OPTION= AL, "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, "ima= ge", 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 =3D 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 =3D gfc_build_library_function_decl_with_spec ( 471:+ pvoid_type_node, 3, pvoid_type_node, integer_type_node, integer_type_node);=20 The remainder of the script output needs no fix, because its in For= tran 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 scrol= ling. 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 t= he empty if. There still is no test when -fcoarray=3Dsingle is used. This shouldn't be so hard, should it? Regards, Andre On Mon, 19 Sep 2016 08:30:12 -0700 Alessandro Fanfarillo wrote: > * PING * >=20 > On Sep 7, 2016 3:01 PM, "Alessandro Fanfarillo" > wrote: >=20 > > Dear all, > > the attached patch supports failed images also when -fcoarray=3Dsingle = 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=3Dsingl= e. > > > 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 > > > 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 : > > >>>> Le 20/07/2016 =C3=A0 11:39, Andre Vehreschild a =C3=A9crit : > > >>>>> > > >>>>> Hi Mikael, > > >>>>> > > >>>>> > > >>>>>>> + if(st =3D=3D ST_FAIL_IMAGE) > > >>>>>>> + new_st.op =3D EXEC_FAIL_IMAGE; > > >>>>>>> + else > > >>>>>>> + gcc_unreachable(); > > >>>>>> > > >>>>>> You can use > > >>>>>> gcc_assert (st =3D=3D ST_FAIL_IMAGE); > > >>>>>> foo...; > > >>>>>> instead of > > >>>>>> if (st =3D=3D ST_FAIL_IMAGE) > > >>>>>> foo...; > > >>>>>> else > > >>>>>> gcc_unreachable (); > > >>>>> > > >>>>> > > >>>>> Be careful, this is not 100% identical in the general case. For o= lder > > >>>>> gcc version (gcc < 4008) gcc_assert() is mapped to nothing, esp. = not > > to > > >>>>> an abort(), so the behavior can change. But in this case everythi= ng > > 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 preferr= ed, > > 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 statem= ent > > >>>>>>> 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 =3D gfc_intrinsic_argument_list_length (expr); > > >>>>>>> + args =3D XALLOCAVEC (tree, num_args); > > >>>>>>> + > > >>>>>>> + gfc_conv_intrinsic_function_args (se, expr, args, num_args); > > >>>>>>> + > > >>>>>>> + if (flag_coarray =3D=3D GFC_FCOARRAY_LIB) > > >>>>>>> + { > > >>>>>> > > >>>>>> Can everything be put under the if? > > >>>>>> Does it work with -fcoarray=3Dsingle? > > >>>>> > > >>>>> > > >>>>> IMO coarray=3Dsingle should not generate code here, therefore put= ting > > >>>>> everything under the if should to fine. > > >>>>> > > >>>> My point was more avoiding generating code for the arguments if th= ey > > are not > > >>>> used in the end. > > >>>> Regarding the -fcoarray=3Dsingle case, the function returns a resu= lt, > > 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 > > --=20 Andre Vehreschild * Email: vehre ad gmx dot de=20