Re: [OG12][PATCH] OpenMP: Fix ICE with OMP metadirectives
Hi Paul-Antoine, hi all, On 28.09.22 15:47, Paul-Antoine Arras wrote: Here is a revised patch resulting from your comments. The only issue that I could not easily fix is the following code triggering an ICE: [...] I filed a PR on Bugzilla: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107067 Is it OK for you? Thanks for the update – LGTM. (We may want to revise some general block handling, but that's something for either upstreaming – or even after the upstreaming hen fixing the PR.) Two minor remarks: (a) Don't forget to update the date in the ChangeLog.omp. (b) On can shorten bug URL by using https://gcc.gnu.org/PR107067 which is, well, shorter. (Similarly, https://gcc.gnu.org/g: or https://gcc.gnu.org/r... for the svn or 'git gcc-descr [--full]' revision number also works.) 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
Re: [OG12][PATCH] OpenMP: Fix ICE with OMP metadirectives
Hi Tobias,
Thanks for your review.
Here is a revised patch resulting from your comments.
The only issue that I could not easily fix is the following code
triggering an ICE:
```
!$OMP begin metadirective &
!$OMP when ( user = { condition ( UseDevice ) } &
!$OMP : nothing ) &
!$OMP default ( parallel )
block
call foo()
end block
call bar() <--- ICE
!$omp end metadirective
```
I filed a PR on Bugzilla:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107067
Is it OK for you?
--
PAFrom 03476214bda71c6581b7978cf9fd5b701896a052 Mon Sep 17 00:00:00 2001
From: Paul-Antoine Arras
Date: Wed, 21 Sep 2022 15:52:56 +
Subject: [PATCH] OpenMP: Fix ICE with OMP metadirectives
Problem: ending an OpenMP metadirective block with an OMP end statement
results in an internal compiler error.
Solution: reject invalid end statements and issue a proper diagnostic.
This revision also fixes a couple of minor metadirective issues and adds
related test cases.
gcc/fortran/ChangeLog:
* parse.cc (gfc_ascii_statement): Missing $ in !$OMP END METADIRECTIVE.
(parse_omp_structured_block): Fix handling of OMP end metadirective.
(parse_omp_metadirective_body): Reject OMP end statements
at the end of an OMP metadirective.
gcc/testsuite/ChangeLog:
* gfortran.dg/gomp/metadirective-1.f90: Match !$OMP END METADIRECTIVE.
* gfortran.dg/gomp/metadirective-10.f90: New test.
* gfortran.dg/gomp/metadirective-11.f90: New xfail test.
* gfortran.dg/gomp/metadirective-9.f90: New test.
---
gcc/fortran/ChangeLog.omp | 7
gcc/fortran/parse.cc | 32 ++-
gcc/testsuite/ChangeLog.omp | 7
.../gfortran.dg/gomp/metadirective-1.f90 | 2 +-
.../gfortran.dg/gomp/metadirective-10.f90 | 40 +++
.../gfortran.dg/gomp/metadirective-11.f90 | 33 +++
.../gfortran.dg/gomp/metadirective-9.f90 | 30 ++
7 files changed, 141 insertions(+), 10 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/gomp/metadirective-10.f90
create mode 100644 gcc/testsuite/gfortran.dg/gomp/metadirective-11.f90
create mode 100644 gcc/testsuite/gfortran.dg/gomp/metadirective-9.f90
diff --git gcc/fortran/ChangeLog.omp gcc/fortran/ChangeLog.omp
index 8c89cd5bd43..14f897e8fec 100644
--- gcc/fortran/ChangeLog.omp
+++ gcc/fortran/ChangeLog.omp
@@ -1,3 +1,10 @@
+2022-09-21 Paul-Antoine Arras
+
+ * parse.cc (gfc_ascii_statement): Missing $ in !$OMP END METADIRECTIVE.
+ (parse_omp_structured_block): Fix handling of OMP end metadirective.
+ (parse_omp_metadirective_body): Reject OMP end statements
+ at the end of an OMP metadirective.
+
2022-09-09 Tobias Burnus
Backport from mainline:
diff --git gcc/fortran/parse.cc gcc/fortran/parse.cc
index b35d76a4f6b..fc88111a7ad 100644
--- gcc/fortran/parse.cc
+++ gcc/fortran/parse.cc
@@ -2517,7 +2517,7 @@ gfc_ascii_statement (gfc_statement st)
p = "!$OMP END MASTER TASKLOOP SIMD";
break;
case ST_OMP_END_METADIRECTIVE:
- p = "!OMP END METADIRECTIVE";
+ p = "!$OMP END METADIRECTIVE";
break;
case ST_OMP_END_ORDERED:
p = "!$OMP END ORDERED";
@@ -5643,9 +5643,15 @@ parse_omp_structured_block (gfc_statement omp_st, bool
workshare_stmts_only)
np->block = NULL;
omp_end_st = gfc_omp_end_stmt (omp_st, false, true);
- if (omp_st == ST_NONE)
+ if (omp_end_st == ST_NONE)
gcc_unreachable ();
+ /* If handling a metadirective variant, treat 'omp end metadirective'
+ as the expected end statement for the current construct. */
+ if (gfc_state_stack->previous != NULL
+ && gfc_state_stack->previous->state == COMP_OMP_BEGIN_METADIRECTIVE)
+omp_end_st = ST_OMP_END_METADIRECTIVE;
+
bool block_construct = false;
gfc_namespace *my_ns = NULL;
gfc_namespace *my_parent = NULL;
@@ -5744,13 +5750,6 @@ parse_omp_structured_block (gfc_statement omp_st, bool
workshare_stmts_only)
else
st = parse_executable (st);
- /* If handling a metadirective variant, treat 'omp end metadirective'
-as the expected end statement for the current construct. */
- if (st == ST_OMP_END_METADIRECTIVE
- && gfc_state_stack->previous != NULL
- && gfc_state_stack->previous->state == COMP_OMP_BEGIN_METADIRECTIVE)
- st = omp_end_st;
-
if (st == ST_NONE)
unexpected_eof ();
else if (st == ST_OMP_SECTION
@@ -5863,6 +5862,21 @@ parse_omp_metadirective_body (gfc_statement omp_st)
break;
}
+ if (gfc_state_stack->state == COMP_OMP_METADIRECTIVE
+ && startswith (gfc_ascii_statement (st), "!$OMP END "))
+ {
+ for (gfc_state_data *p = gfc_state_stack; p; p = p->previous)
+ if (p->state == COMP_OMP_STRUCTURED_BLOCK
+ || p->state == COMP_OMP_BEGIN_METADIRECTIVE)
+ goto finish;
+
Re: [OG12][PATCH] OpenMP: Fix ICE with OMP metadirectives
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 i
[OG12][PATCH] OpenMP: Fix ICE with OMP metadirectives
Hello,
Here is a patch that fixes an ICE in gfortran triggered by an invalid
end statement at the end of an OMP metadirective:
```
!$OMP metadirective ...
...
!$OMP end ...
```
Does this fix look correct?
Thanks,
--
Paul-Antoine ArrasFrom 73ecbc2672a5352a08260f7a9d0de6d2c29ea2b6 Mon Sep 17 00:00:00 2001
From: Paul-Antoine Arras
Date: Wed, 21 Sep 2022 15:52:56 +
Subject: [PATCH] OpenMP: Fix ICE with OMP metadirectives
Problem: ending an OpenMP metadirective block with an OMP end statement
results in an internal compiler error.
Solution: reject invalid end statements and issue a proper diagnostic.
Also add a new test to check this behaviour.
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/fortran/ChangeLog.omp | 5
gcc/fortran/parse.cc | 14 +
gcc/testsuite/ChangeLog.omp | 4 +++
.../gfortran.dg/gomp/metadirective-9.f90 | 29 +++
4 files changed, 52 insertions(+)
create mode 100644 gcc/testsuite/gfortran.dg/gomp/metadirective-9.f90
diff --git gcc/fortran/ChangeLog.omp gcc/fortran/ChangeLog.omp
index 8c89cd5bd43..7b253608bf8 100644
--- gcc/fortran/ChangeLog.omp
+++ gcc/fortran/ChangeLog.omp
@@ -1,3 +1,8 @@
+2022-09-21 Paul-Antoine Arras
+
+* parse.cc (parse_omp_metadirective_body): Reject OMP end statements
+at the end of an OMP metadirective.
+
2022-09-09 Tobias Burnus
Backport from mainline:
diff --git gcc/fortran/parse.cc gcc/fortran/parse.cc
index b35d76a4f6b..1f1fa0eba0e 100644
--- gcc/fortran/parse.cc
+++ gcc/fortran/parse.cc
@@ -5863,6 +5863,20 @@ parse_omp_metadirective_body (gfc_statement omp_st)
break;
}
+ if (gfc_state_stack->state == COMP_OMP_METADIRECTIVE
+ && startswith (gfc_ascii_statement (st), "!$OMP END "))
+ {
+ for (gfc_state_data *p = gfc_state_stack; p; p = p->previous)
+ if (p->state == COMP_OMP_STRUCTURED_BLOCK)
+ goto finish;
+ gfc_error (
+ "Unexpected %s statement in an OMP METADIRECTIVE block at %C",
+ gfc_ascii_statement (st));
+ reject_statement ();
+ st = next_statement ();
+ }
+finish:
+
gfc_in_metadirective_body = old_in_metadirective_body;
if (gfc_state_stack->head)
diff --git gcc/testsuite/ChangeLog.omp gcc/testsuite/ChangeLog.omp
index e0c8c138620..f075354af4d 100644
--- 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.
+
2022-09-09 Paul-Antoine Arras
Backport from mainline:
diff --git gcc/testsuite/gfortran.dg/gomp/metadirective-9.f90
gcc/testsuite/gfortran.dg/gomp/metadirective-9.f90
new file mode 100644
index 000..4db37dd0ef9
--- /dev/null
+++ gcc/testsuite/gfortran.dg/gomp/metadirective-9.f90
@@ -0,0 +1,29 @@
+! { dg-do compile }
+
+program OpenMP_Metadirective_WrongEnd_Test
+
+ integer :: &
+iV, jV, kV
+ integer, dimension ( 3 ) :: &
+lV, uV
+ logical :: &
+UseDevice
+
+!$OMP metadirective &
+!$OMP when ( user = { condition ( UseDevice ) } &
+!$OMP : target teams distribute parallel do simd collapse ( 3 ) &
+!$OMP private ( iaVS ) ) &
+!$OMP default ( parallel do simd collapse ( 3 ) private ( iaVS ) )
+do kV = lV ( 3 ), uV ( 3 )
+ do jV = lV ( 2 ), uV ( 2 )
+do iV = lV ( 1 ), uV ( 1 )
+
+
+end do
+ end do
+end do
+!$OMP end target teams distribute parallel do simd ! { dg-error
"Unexpected !.OMP END TARGET TEAMS DISTRIBUTE PARALLEL DO SIMD statement in an
OMP METADIRECTIVE block at .1." }
+
+
+end program
+
--
2.31.1
