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 statement 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-December/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-February/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 bits, 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 whether that's a completely separate code location which requires a completely separate patch. – If the latter, the patch LGTM, otherwise, it would be great 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 = "!OMP END METADIRECTIVE"; + p = "!$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 = { 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'). – Thus, I tried also with two 'block' to check this is also accepted. Likewise, the following code is mishandled in an odd way – but only if all when/default use the same delimited directive: ! diagnostic, accepts-invalid -- not ideal but neither ICE nor rejecting valid code !$OMP begin metadirective & !$OMP when ( user = { 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 – 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 sure. The following handles "end parallel" if there is only "parallel" in when/default; 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 = { 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" (→ loosely structured block, then "end parallel" 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. – Especially when just browsing through the comments ("git log"), I am happy if the log is short. However, when studying a commit in more detail, I am happy about understanding what/why a patch did something. ) (Personally, I also would also add 'Fortran' to the subject line if only related Fortran; first, it makes it more likely to get reviewed by Fortran maintainers 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 email 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 sensitively 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 <p...@codesourcery.com><mailto:p...@codesourcery.com> + + * 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 that 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. dev_num vs. devnum) and suddenly the wrong variable is used ... (in this example, even an implicitly declared REAL number while surely a device number should be an integer). Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
