Re: [OG12][PATCH] OpenMP: Fix ICE with OMP metadirectives

[Tobias Burnus <tobias@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 7548 bytes --]

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  <pa@codesourcery.com><mailto:pa@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