From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id 88A4E3858421; Thu, 22 Sep 2022 09:01:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 88A4E3858421 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.93,335,1654588800"; d="scan'208,217";a="86214082" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa1.mentor.iphmx.com with ESMTP; 22 Sep 2022 01:01:54 -0800 IronPort-SDR: bI4Godkqtq3cUtXB9EWgIODc4Bvl9H6vebwxg0OAilen8xYleGAuMhdr2zd++SyLZLo5ZE1wjK HCJq29NUUyp6jDHbU4+8dOIMDctzrpCQIZxXGNdeAz2XnOFhXGot2gSq2T1V4mynqO+UbupbrY i2ARJ5dx6H2n01NITVLCl8Hadx2pv+YtO8Sr0tB0mAnT/MQyaZBbo9Zrb+7rbPIfaCXwDVI5Dd 5apNHhXAua/CB+6lz8CiPNcm20i3GKSc93rBdO0MePyxComxUsHdwB4IlZDDjhKXQ5sfgxNE+q LkU= Content-Type: multipart/alternative; boundary="------------KR0u04YfCMLpYjRvXIABdIg0" Message-ID: <1696e0a6-52e3-dc61-08a8-eaa26c5c3e1e@codesourcery.com> Date: Thu, 22 Sep 2022 11:01:48 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [OG12][PATCH] OpenMP: Fix ICE with OMP metadirectives Content-Language: en-US To: Paul-Antoine Arras , , References: From: Tobias Burnus In-Reply-To: X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) To svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,KAM_DMARC_STATUS,KAM_SHORT,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --------------KR0u04YfCMLpYjRvXIABdIg0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: quoted-printable Hello Paul-Antoine, hi all, On 21.09.22 23:18, Paul-Antoine Arras wrote: Here is a patch that fixes an ICE in gfortran triggered by an invalid end s= tatement at the end of an OMP metadirective: Remark for other reads of this email: This only applies to OG12 as mainline does not have the following patches: --------------------------- Patch set from December: https://gcc.gnu.org/pipermail/gcc-patches/2021-Dec= ember/thread.html#586599 Reviews in May (multiple, e.g. 1/7 is at https://gcc.gnu.org/pipermail/gcc-= patches/2022-May/595762.html ) Fortran follow-up patches: https://gcc.gnu.org/pipermail/gcc-patches/2022-F= ebruary/590368.html --------------------------- I played a bit with the patch. I think it looks okayish. It seems to handle the code in question correctly. I worried about some bit= s, which turned out to be unfounded. However, I found some related issues that look similar (but are unaffected of the patch). I do not quickly see whether your patch should handle them as well or wheth= er that's a completely separate code location which requires a completely sepa= rate patch. =E2=80=93 If the latter, the patch LGTM, otherwise, it would be grea= t if it could handle the other issues as well. First, can you include in your patch also: --- a/gcc/fortran/parse.cc +++ b/gcc/fortran/parse.cc @@ -2520 +2520 @@ gfc_ascii_statement (gfc_statement st) - p =3D "!OMP END METADIRECTIVE"; + p =3D "!$OMP END METADIRECTIVE"; The following first two examples are about "omp begin/end metadirective" - = while "your" ICE was about the non-delimited "omp metadirective". The following program gives an ICE - and I believe it is valid code. When replacing 'nothing' by 'parallel', it is instead rejected ("Unexpected !OMP END METADIRECTIVE statement".) ! ice-on-valid-code, rejects-valid -- this is bad! subroutine test2 logical :: UseDevice !$OMP begin metadirective & !$OMP when ( user =3D { condition ( UseDevice ) } & !$OMP : nothing ) & !$OMP default ( parallel ) block call bar() end block !block ! call foo() !end block !$omp end metadirective end I wonder whether it is also related to strictly nested blocks, but, in any case, (strictly/loosely structures) blocks do not apply here ('begin/end metadirective' has association 'delimited' not 'block'). =E2=80=93 Thus, I tried also with two 'block' to check this is also accepted. Likewise, the following code is mishandled in an odd way =E2=80=93 but only if all when/default use the same delimited directive: ! diagnostic, accepts-invalid -- not ideal but neither ICE nor rejecting va= lid code !$OMP begin metadirective & !$OMP when ( user =3D { condition ( UseDevice ) } & !$OMP : parallel ) & !$OMP default ( parallel ) call bar() !!$omp end parallel ! (1) !!$omp end metadirective ! (2) end Uncommenting (2): it is accepted (and it should be) Uncommenting (1): This is accepted - but shouldn't. There is an "end metadirective" missing =E2=80=93 that is required. Uncommenting (1) and (2): The line (1) accepted but then (2) is rejected Note: This only happens if all directives in when/default are the same such that the 'end parallel' works for all of them. I also tried the non-delimited '!$omp metadirective' (i.e. no begin...end), but that seems to work fine. I still wonder whether it should be added as another testcase (three tests, could be in the same files), just to make su= re. The following handles "end parallel" if there is only "parallel" in when/de= fault; however, I think all variants of the following are valid (but bad style - for a (non-loop-associated) block-associated directive, using begin/end makes more sense than dumping an explicit end directive.) ! OK - add as three test cases (?) program test logical :: UseDevice !$OMP metadirective & !$OMP when ( user =3D { condition ( UseDevice ) } & !$OMP : parallel ) & !$OMP default ( parallel ) block ! ... end block !$omp end parallel ! Accepted, but only all cases have 'parallel' end program The "end parallel" is optional here as there is a strictly structured block (the "block ... end block"); without "end parallel" or without the "block" / "end block" (=E2=86=92 loosely structured block, then "end parall= el" is required), it also work. (Hence, three testcases.) * * * To the patch - one important comment to "ChangeLog(.omp)" and otherwise only some personal comments. Subject: [PATCH] OpenMP: Fix ICE with OMP metadirectives ... Also add a new test to check this behaviour. (Personally, I would remove the last line: I always would expect a testcase if feasible and 4 lines later there is also "New test.". But it also does not harm. =E2=80=93 Especially when just browsing through the comments ("gi= t log"), I am happy if the log is short. However, when studying a commit in more det= ail, I am happy about understanding what/why a patch did something. ) (Personally, I also would also add 'Fortran' to the subject line if only re= lated Fortran; first, it makes it more likely to get reviewed by Fortran maintain= ers and it also groups a patch a bit. But as OpenMP already groups it, this is = only a minor personal taste. - In tendency, it helps if the patch discussion ema= il thread and the commit match. I recently did miss to find a patch because of "Push"= vs. "push" because git log invoked "less" such that it searches case sensitivel= y by default ... Thus, it is better not to change the subject for this patch!) gcc/fortran/ChangeLog: * parse.cc (parse_omp_metadirective_body): Reject OMP end statements at the end of an OMP metadirective. gcc/testsuite/ChangeLog: * gfortran.dg/gomp/metadirective-9.f90: New test. ... --- gcc/testsuite/ChangeLog.omp +++ gcc/testsuite/ChangeLog.omp @@ -1,3 +1,7 @@ +2022-09-21 Paul-Antoine Arras + + * gfortran.dg/gomp/metadirective-9.f90: New test. There should be a 'tab' not spaces before '*'. That also applies to the commit log but you or mklog.py already did that. +++ gcc/testsuite/gfortran.dg/gomp/metadirective-9.f90 @@ -0,0 +1,29 @@ +! { dg-do compile } + +program OpenMP_Metadirective_WrongEnd_Test ... + !$OMP private ( iaVS ) ) & + !$OMP default ( parallel do simd collapse ( 3 ) private ( iaVS ) ) General Fortran comment: For testcases, it is perfectly fine - especially if they are compile only. However, when writing production code, it is strongly recommended to add an "implicit none" (after any 'use'/'import' lines, if any. Here it would be at "..." directly after the "program" line.) The "implicit none" helps to avoid some odd issues when writing code, which are hard to trace. In this testcase, adding the implicit line will cause th= at is fails to compile as iaVS is not declared and it is therefore implicitly declared as = integer. Here, it is perfectly fine - but a typical problem is a misspelling (e.g. d= ev_num vs. devnum) and suddenly the wrong variable is used ... (in this example, even an impli= citly declared REAL number while surely a device number should be an integer). Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955 --------------KR0u04YfCMLpYjRvXIABdIg0--