[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
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->previo
[PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
Hi all,
The attached patch fixes a bug that causes valid OpenMP declare variant
directive and functions to be rejected with the following error (see
testcase):
c_ptr.f90:41:37:
41 | !$omp declare variant(foo_variant) &
| 1
Error: variant ‘foo_variant’ and base ‘foo’ at (1) have incompatible
types: Type mismatch in argument 'c_bv' (INTEGER(8)/TYPE(c_ptr))
The fix consists in special-casing this situation in gfc_compare_types().
OK for mainline?
Thanks,
--
PA
From 8e5fa4828678a1388e75795de2a1f253d9f0ec95 Mon Sep 17 00:00:00 2001
From: Paul-Antoine Arras
Date: Fri, 20 Oct 2023 12:42:49 +0200
Subject: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and
TYPE(c_ptr)
In the context of an OpenMP declare variant directive, arguments of type C_PTR
are sometimes recognised as C_PTR in the base function and as INTEGER(8) in the
variant - or the other way around, depending on the parsing order.
This patch prevents such situation from turning into a compile error.
2023-10-20 Paul-Antoine Arras
Tobias Burnus
gcc/fortran/ChangeLog:
* interface.cc (gfc_compare_types): Return true in this situation.
gcc/testsuite/ChangeLog:
* gfortran.dg/c_ptr_tests_20.f90: New test.
---
gcc/fortran/ChangeLog.omp| 5 ++
gcc/fortran/interface.cc | 17 +++---
gcc/testsuite/ChangeLog.omp | 4 ++
gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90 | 56
4 files changed, 76 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
diff --git a/gcc/fortran/ChangeLog.omp b/gcc/fortran/ChangeLog.omp
index 62a33475ee5..299223ceaa7 100644
--- a/gcc/fortran/ChangeLog.omp
+++ b/gcc/fortran/ChangeLog.omp
@@ -1,3 +1,8 @@
+2023-10-20 Paul-Antoine Arras
+ Tobias Burnus
+
+ * interface.cc (gfc_compare_types): Return true in this situation.
+
2023-09-19 Tobias Burnus
Backported from master:
diff --git a/gcc/fortran/interface.cc b/gcc/fortran/interface.cc
index e9843e9549c..8bd35fd6d22 100644
--- a/gcc/fortran/interface.cc
+++ b/gcc/fortran/interface.cc
@@ -705,12 +705,17 @@ gfc_compare_types (gfc_typespec *ts1, gfc_typespec *ts2)
/* Special case for our C interop types. FIXME: There should be a
better way of doing this. When ISO C binding is cleared up,
- this can probably be removed. See PR 57048. */
-
- if (((ts1->type == BT_INTEGER && ts2->type == BT_DERIVED)
- || (ts1->type == BT_DERIVED && ts2->type == BT_INTEGER))
- && ts1->u.derived && ts2->u.derived
- && ts1->u.derived == ts2->u.derived)
+ this can probably be removed. See PR 57048.
+ Note that this does not distinguish between c_ptr and c_funptr. */
+
+ if ((ts1->type == BT_INTEGER && ts2->type == BT_DERIVED
+ && ts1->f90_type == BT_VOID
+ && ts2->u.derived->ts.is_iso_c
+ && ts2->u.derived->ts.u.derived->ts.f90_type == BT_VOID)
+ || (ts2->type == BT_INTEGER && ts1->type == BT_DERIVED
+ && ts2->f90_type == BT_VOID
+ && ts1->u.derived->ts.is_iso_c
+ && ts1->u.derived->ts.u.derived->ts.f90_type == BT_VOID))
return true;
/* The _data component is not always present, therefore check for its
diff --git a/gcc/testsuite/ChangeLog.omp b/gcc/testsuite/ChangeLog.omp
index e9f4d1c63e6..1fc9b0606dc 100644
--- a/gcc/testsuite/ChangeLog.omp
+++ b/gcc/testsuite/ChangeLog.omp
@@ -1,3 +1,7 @@
+2023-10-20 Paul-Antoine Arras
+
+ * gfortran.dg/c_ptr_tests_20.f90: New test.
+
2023-09-20 Tobias Burnus
Backported from master:
diff --git a/gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90 b/gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
new file mode 100644
index 000..777181cece0
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
@@ -0,0 +1,56 @@
+! { dg-do compile }
+!
+! This failed to compile the declare variant directive due to the C_PTR
+! arguments to foo being recognised as INTEGER(8)
+
+program adjust_args
+ use iso_c_binding, only: c_loc
+ implicit none
+
+ integer, parameter :: N = 1024
+ real, allocatable, target :: av(:), bv(:), cv(:)
+
+ call foo(c_loc(bv), c_loc(av), N)
+
+ !$omp target data map(to: av(:N)) map(from: cv(:N))
+ !$omp parallel
+ call foo(c_loc(cv), c_loc(av), N)
+ !$omp end parallel
+ !$omp end target data
+
+contains
+ subroutine foo_variant(c_d_bv, c_d_av, n)
+use iso_c_binding, only: c_ptr, c_f_pointer
+type(c_ptr), intent(in) :: c_d_bv, c_d_av
+integer, intent(in) :: n
+real, pointer :: f_d_bv(:)
+real, pointer :: f_d_av(:)
+integer :: i
+
+call c_f_pointer(c_d_bv, f_d_bv, [n])
+call c_f_pointer(c_d_av, f_d_av, [n])
+!$omp target teams loop is_device_ptr(f_d_bv, f_d_av)
+do i = 1, n
+
Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
Hi Tobias,
Please see the updated patch attached incorporating your input and
details below.
On 24/10/2023 18:12, you wrote:
On 20.10.23 16:02, Paul-Antoine Arras wrote:
gcc/fortran/ChangeLog:
* interface.cc (gfc_compare_types): Return true in this situation.
That's a bad description. It makes sense when reading the commit log but
if you
only read gcc/fortran/ChangeLog, 'this situation' is a dangling reference.
Updated Changelog with a more helpful description.
gcc/fortran/ChangeLog.omp | 5 ++
gcc/testsuite/ChangeLog.omp | 4 ++
On mainline, the ChangeLog not ChangeLog.omp is used. This changelog is
automatically
filled by the data in the commit log. Thus, no need to include it in the
patch.
Removed ChangeLog.omp from the patch.
See attached patch for a combined version, which checks now
whether from_intmod == INTMOD_ISO_C_BINDING and then compares
the names (to distinguish c_ptr and c_funptr). Those are unaffected
by 'use' renames, hence, we should be fine.
Added the proposed diff for interface.cc and misc.cc to the patch.
Additionally, I think it would be good to have a testcase which checks for
c_funptr vs. c_ptr
mismatch.
Added new testcase c_ptr_tests_21.f90 to check that incompatibilities
between c_funptr vs. c_ptr are properly reported.
Is this latest revision ready to commit?
Thanks,
--
PA
From 691d1050ce39c27231dc610b799bf180871820b8 Mon Sep 17 00:00:00 2001
From: Paul-Antoine Arras
Date: Fri, 20 Oct 2023 12:42:49 +0200
Subject: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and
TYPE(c_ptr)
In the context of an OpenMP declare variant directive, arguments of type C_PTR
are sometimes recognised as C_PTR in the base function and as INTEGER(8) in the
variant - or the other way around, depending on the parsing order.
This patch prevents such situation from turning into a compile error.
2023-10-20 Paul-Antoine Arras
Tobias Burnus
gcc/fortran/ChangeLog:
* interface.cc (gfc_compare_types): Return true if one type is C_PTR
and the other is a compatible INTEGER(8).
* misc.cc (gfc_typename): Handle the case where an INTEGER(8) actually
holds a TYPE(C_PTR).
gcc/testsuite/ChangeLog:
* gfortran.dg/c_ptr_tests_20.f90: New test, checking that INTEGER(8)
and TYPE(C_PTR) are recognised as compatible.
* gfortran.dg/c_ptr_tests_21.f90: New test, exercising the error
detection for C_FUNPTR.
---
gcc/fortran/interface.cc | 16 --
gcc/fortran/misc.cc | 7 ++-
gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90 | 57
gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90 | 57
4 files changed, 132 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
create mode 100644 gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
diff --git a/gcc/fortran/interface.cc b/gcc/fortran/interface.cc
index e9843e9549c..ed1613b16fb 100644
--- a/gcc/fortran/interface.cc
+++ b/gcc/fortran/interface.cc
@@ -707,10 +707,18 @@ gfc_compare_types (gfc_typespec *ts1, gfc_typespec *ts2)
better way of doing this. When ISO C binding is cleared up,
this can probably be removed. See PR 57048. */
- if (((ts1->type == BT_INTEGER && ts2->type == BT_DERIVED)
- || (ts1->type == BT_DERIVED && ts2->type == BT_INTEGER))
- && ts1->u.derived && ts2->u.derived
- && ts1->u.derived == ts2->u.derived)
+ if ((ts1->type == BT_INTEGER
+ && ts2->type == BT_DERIVED
+ && ts1->f90_type == BT_VOID
+ && ts2->u.derived->from_intmod == INTMOD_ISO_C_BINDING
+ && ts1->u.derived
+ && strcmp (ts1->u.derived->name, ts2->u.derived->name) == 0)
+ || (ts2->type == BT_INTEGER
+ && ts1->type == BT_DERIVED
+ && ts2->f90_type == BT_VOID
+ && ts1->u.derived->from_intmod == INTMOD_ISO_C_BINDING
+ && ts2->u.derived
+ && strcmp (ts1->u.derived->name, ts2->u.derived->name) == 0))
return true;
/* The _data component is not always present, therefore check for its
diff --git a/gcc/fortran/misc.cc b/gcc/fortran/misc.cc
index bae6d292dc5..edffba07013 100644
--- a/gcc/fortran/misc.cc
+++ b/gcc/fortran/misc.cc
@@ -138,7 +138,12 @@ gfc_typename (gfc_typespec *ts, bool for_hash)
switch (ts->type)
{
case BT_INTEGER:
- sprintf (buffer, "INTEGER(%d)", ts->kind);
+ if (ts->f90_type == BT_VOID
+ && ts->u.derived
+ && ts->u.derived->from_intmod == INTMOD_ISO_C_BINDING)
+ sprintf (buffer, "TYPE(%s)", ts->u.derived->name);
+ else
+ sprintf (buffer, "INTEGER(%d)", ts->kind);
break;
case BT_REAL:
sprintf (buffer, "REAL(%d)", ts->ki
Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
Hi Thomas,
On 26/10/2023 18:16, you wrote:
Hi!
On 2023-10-26T13:24:04+0200, Paul-Antoine Arras wrote:
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
@@ -0,0 +1,57 @@
+! { dg-do compile }
+! { dg-additional-options "-fopenmp" }
+[...]
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
@@ -0,0 +1,57 @@
+! { dg-do compile }
+! { dg-additional-options "-fopenmp" }
+[...]
OpenMP is not universally supported across different GCC configurations,
so this will FAIL for some. Therefore, please either guard with
effective target:
@item fopenmp
Target supports OpenMP via @option{-fopenmp}.
Would the following be enough?
diff --git gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
index 7dd510400f3..131603d3819 100644
--- gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
+++ gcc/testsuite/gfortran.dg/c_ptr_tests_20.f90
@@ -1,4 +1,5 @@
! { dg-do compile }
+! { dg-require-effective-target fopenmp }
! { dg-additional-options "-fopenmp" }
!
! This failed to compile the declare variant directive due to the C_PTR
diff --git gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90 gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
index 05ccb771eee..060d29d0275 100644
--- gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
+++ gcc/testsuite/gfortran.dg/c_ptr_tests_21.f90
@@ -1,4 +1,5 @@
! { dg-do compile }
+! { dg-require-effective-target fopenmp }
! { dg-additional-options "-fopenmp" }
!
! Ensure that C_PTR and C_FUNPTR are reported as incompatible types in variant
Thanks,
--
PA
Re: [PATCH v4 6/7] OpenMP: Fortran front-end support for dispatch + adjust_args
On 27/12/2024 19:52, Paul-Antoine Arras wrote: On 23/12/2024 21:04, Tobias Burnus wrote: For adjust-args-10.f90, I wonder whether it is sufficient as compile- time only or whether it makes more sense to have a "dg-do run" to check that type(C_ptr) value vs. not-value works. I think either is fine, but if it stays in gcc/, can you manually run it once to re- check that it works? (I think I did check it and it worked.) Running adjust-args-10.f90 manually exhibited a bug that no other testcase triggered. So I fixed the bug; then moved adjust-args-10.f90 to the libgomp testsuite, renamed it to dispatch-3.f90 and made it dg-run. I forgot to mention, I had to comment out the two allocatables in functions f and g, otherwise I would get a segfault upon return. Is that correct? Thanks, -- PA
Re: [PATCH v4 6/7] OpenMP: Fortran front-end support for dispatch + adjust_args
Hi Tobias,
On 23/12/2024 21:04, Tobias Burnus wrote:
Paul-Antoine Arras wrote:
Replying to your last two messages here and attaching revised patches.
Regarding the C++ and ME patches:
==> 0003-C-fix.patch <==
Subject: [PATCH 3/4] C++ fix
==> 0004-ME-fixes.patch <==
Subject: [PATCH 4/4] ME fixes
I think it is best to fold them into the Fortran patch; otherwise, they
would clearly need a better subject line.
Folded everything in one patch.
And for both changes and in either case, both need a ChangeLog entry.
Updated ChangeLog.
Additionally, the middle-end patch does not apply as it doesn't honor my
Dec 18 change to gimplify.cc.
Rebased and amended accordingly. I believe I did the right thing but
please have a quick look to be sure.
* * *
==> 0001-OpenMP-Fortran-front-end-support-for-dispatch-adjust.patch <==
Subject: [PATCH 1/4] OpenMP: Fortran front-end support for dispatch +
adjust_args
The following two patches do not work (at least with some testsuite
testing) as in gcc/testsuite/ neither omp_lib nor libgomp.{so,a} is
available.
For gcc/testsuite/gfortran.dg/gomp/adjust-args-10.f90, you can just
remove the 'omp_lib'.
And as gcc/testsuite/gfortran.dg/gomp/declare-variant-21.f90 contains !
{ dg-do run }
... I'd suggest to move it to libgomp (including its aux-21.f90 file).
Fixed as suggested.
For adjust-args-10.f90, I wonder whether it is sufficient as compile-
time only or whether it makes more sense to have a "dg-do run" to check
that type(C_ptr) value vs. not-value works. I think either is fine, but
if it stays in gcc/, can you manually run it once to re-check that it
works? (I think I did check it and it worked.)
Running adjust-args-10.f90 manually exhibited a bug that no other
testcase triggered. So I fixed the bug; then moved adjust-args-10.f90 to
the libgomp testsuite, renamed it to dispatch-3.f90 and made it dg-run.
* * *
Note that gcc/testsuite/gfortran.dg/gomp/adjust-args-9.f90
and ...-10.f90 are missing a ChangeLog entry.
Likewise for dispatch-9a.f90.
Updated ChangeLog.
BTW: If you have applied (committed) the patch locally, run ./contrib/
gcc-changelog/git_check_commit.py -v -p — the '-v' will output new files
that have not been listed as warning and -p shows the patch log for
checking it. Additionally, it has the usual "git push" checks of GCC.
Thanks for the tip!
* * *
Otherwise, LGTM. Thanks!
[As gimplify.cc couldn't be applied, I have not played with the patch
but I believe that it should be okay, based on past playing and looking
at the patch.]
Tobias
PS: Besides fixing the minor issues above, I think you have a follow-up/
cleanup patch available addressing some issues related to the C/C++ FE,
including where the '#pragma' is handled etc. I am looking forward to
that follow-up patch as well :-)
Yes, I'll post the follow-up patches soon.
Thanks,
--
PAcommit 59c7645e4af84e64141098085e27c39861b32522
Author: Paul-Antoine Arras
Date: Fri May 24 19:13:50 2024 +0200
OpenMP: Fortran front-end support for dispatch + adjust_args
This patch adds support for the `dispatch` construct and the `adjust_args`
clause to the Fortran front-end.
Handling of `adjust_args` across translation units is missing due to PR115271.
Minor modifications to the C++ FE and the ME are also folded into this patch as
a side effect of the Fortran work.
gcc/c-family/ChangeLog:
* c-attribs.cc: (c_common_gnu_attributes): Rename "omp declare variant
variant adjust_args" into "omp declare variant variant args" to also
accommodate append_args.
gcc/cp/ChangeLog:
* parser.cc (cp_parser_omp_dispatch): Handle INDIRECT_REF.
gcc/fortran/ChangeLog:
* dump-parse-tree.cc (show_omp_clauses): Handle novariants and nocontext
clauses.
(show_omp_node): Handle EXEC_OMP_DISPATCH.
(show_code_node): Likewise.
* frontend-passes.cc (gfc_code_walker): Handle novariants and nocontext.
* gfortran.h (enum gfc_statement): Add ST_OMP_DISPATCH.
(symbol_attribute): Add omp_declare_variant_need_device_ptr.
(gfc_omp_clauses): Add novariants and nocontext.
(gfc_omp_declare_variant): Add need_device_ptr_arg_list.
(enum gfc_exec_op): Add EXEC_OMP_DISPATCH.
* match.h (gfc_match_omp_dispatch): Declare.
* openmp.cc (gfc_free_omp_clauses): Free novariants and nocontext
clauses.
(gfc_free_omp_declare_variant_list): Free need_device_ptr_arg_list
namelist.
(enum omp_mask2): Add OMP_CLAUSE_NOVARIANTS and OMP_CLAUSE_NOCONTEXT.
(gfc_match_omp_clauses): Handle OMP_CLAUSE_NOVARIANTS and
OMP_CLAUSE_NOCONTEXT.
(OMP_DISPATCH_CLAUSES): De
[PATCH] OpenMP/Fortran: Add missing pop_state in parse_omp_dispatch
When the ST_NONE case is taken, the function returns immediately. Not calling
pop_state causes a dangling pointer.
gcc/fortran/ChangeLog:
* parse.cc (parse_omp_dispatch): Add missing pop_state.
---
gcc/fortran/parse.cc | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/gcc/fortran/parse.cc b/gcc/fortran/parse.cc
index 00cd23d7729..5094d9d3ead 100644
--- a/gcc/fortran/parse.cc
+++ b/gcc/fortran/parse.cc
@@ -6375,7 +6375,10 @@ parse_omp_dispatch (void)
st = next_statement ();
if (st == ST_NONE)
-return st;
+{
+ pop_state ();
+ return st;
+}
if (st == ST_CALL || st == ST_ASSIGNMENT)
accept_statement (st);
else
--
2.47.2
Re: [PATCH] OpenMP/Fortran: Add missing pop_state in parse_omp_dispatch
Pushed to master as obvious. This should fix PR118714.
On 31/01/2025 11:46, Paul-Antoine Arras wrote:
When the ST_NONE case is taken, the function returns immediately. Not calling
pop_state causes a dangling pointer.
gcc/fortran/ChangeLog:
* parse.cc (parse_omp_dispatch): Add missing pop_state.
---
gcc/fortran/parse.cc | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/gcc/fortran/parse.cc b/gcc/fortran/parse.cc
index 00cd23d7729..5094d9d3ead 100644
--- a/gcc/fortran/parse.cc
+++ b/gcc/fortran/parse.cc
@@ -6375,7 +6375,10 @@ parse_omp_dispatch (void)
st = next_statement ();
if (st == ST_NONE)
-return st;
+{
+ pop_state ();
+ return st;
+}
if (st == ST_CALL || st == ST_ASSIGNMENT)
accept_statement (st);
else
--
PA
Re: [PATCH] Accept commas between clauses in OpenMP declare variant
On 13/01/2025 16:51, Tobias Burnus wrote:
Hi PA,
Paul-Antoine Arras wrote:
I am not sure I am getting that part. Is this what you are suggesting?
Yes, something like that, but not quite, as you found out.
I think we need something like the following (untested):
diff --git gcc/fortran/openmp.cc gcc/fortran/openmp.cc
index 9d28dc9..e3abbeeef98 100644
--- gcc/fortran/openmp.cc
+++ gcc/fortran/openmp.cc
@@ -6532,7 +6532,6 @@ gfc_match_omp_context_selector_specification
(gfc_omp_declare_variant *odv)
match
gfc_match_omp_declare_variant (void)
{
- bool first_p = true;
char buf[GFC_MAX_SYMBOL_LEN + 1];
if (gfc_match (" (") != MATCH_YES)
@@ -6590,7 +6589,7 @@ gfc_match_omp_declare_variant (void)
return MATCH_ERROR;
}
- bool has_match = false, has_adjust_args = false;
+ bool has_match = false, has_adjust_args = false, error_p = false;
locus adjust_args_loc;
for (;;)
@@ -6614,13 +6613,9 @@ gfc_match_omp_declare_variant (void)
}
else
{
- if (first_p)
- {
- gfc_error ("expected % or % at %C");
- return MATCH_ERROR;
- }
- else
- break;
+ if (!has_match)
if (gfc_match_omp_eos () != MATCH_YES)
Yes, exactly what I was missing!
+ error_p = true;
+ break;
}
if (gfc_match (" (") != MATCH_YES)
@@ -,8 +6661,12 @@ gfc_match_omp_declare_variant (void)
for (gfc_omp_namelist *n = *head; n != NULL; n = n->next)
n->u.need_device_ptr = true;
}
+ }
- first_p = false;
+ if (error_p)
if (error || (!has_match && !has_adjust_args))
as the missing 'match' is handled more explicitly by the next error.
Right.
+ {
+ gfc_error ("expected % or % at %C");
+ return MATCH_ERROR;
}
* * *
The rest looks good to me.
Thanks,
Tobias
Applied, tested and committed.
Thanks,
--
PA
Re: [PATCH v4 6/7] OpenMP: Fortran front-end support for dispatch + adjust_args
Hi Tobias,
Replying to your last two messages here and attaching revised patches.
On 16/12/2024 22:34, Tobias Burnus wrote:
I have not looked in depth at the patch, but managed to
write C-ism code, which caused a segfault (due to a missing "call"),
after gfortran issued a reasonable error. Can you fix it
and, just to make sure, add it as another testcase?
foo.f90:18:7:
18 | g(a,b,c)
| 1
Error: ‘g’ at (1) is not a variable
foo.f90:20:3:
20 | end
| 1
Error: Unexpected END statement at (1)
Segmentation fault
* * *
The problem seems to be that during parsing,
the location data is NULL, which the error diagnostic
does not like:
(gdb) p gfc_current_locus
$33 = {nextc = 0x0, u = {lb = 0x0, location = 0}}
(gdb) p gfc_at_eof()
$36 = true
and st == ST_NONE.
I think the simplest is to check for the last one,
and then return early. This will then print:
foo.f90:18:7:
18 | g(a,b,c)
| 1
Error: ‘g’ at (1) is not a variable
foo.f90:20:3:
20 | end
| 1
Error: Unexpected END statement at (1)
f951: Error: Unexpected end of file in ‘foo.f90’
When the if st is ST_NONE then return check is added:
+static gfc_statement
+parse_omp_dispatch (void)
+{
...
+ st = next_statement ();
+ if (st == ST_NONE)
+return st;
+ if (st == ST_CALL || st == ST_ASSIGNMENT)
+accept_statement (st);
+ else
Fixed as suggested. Added testcase.
* * *
Handling of `adjust_args` across translation units is missing due to
PR115271.
Namely, https://gcc.gnu.org/PR115271 is about not storing 'declare variant'
inside
module files; when repeating the decl in an interface, it obviously works as
* * *
I think the patch is now okay, but I want to re-read it tomorrow - thus, please
hold off for a couple of ours.
Possibly, others have comments as well :-)
* * *
TODO: We need to handle 'type(C), dimension(:)' - but I wonder
whether that shouldn't be handled as part of 'use_device_addr'
and we need to check whether the spec has to be updated.
I filed the OpenMP lang-spec Issue #4443.
... and we eventually have to handle 'need_device_addr'/'has_device_addr', but
those are follow-up topics.
Keeping an eye on the open issue.
On 17/12/2024 14:11, Tobias Burnus wrote:
Additional comments: Can you hoist the condition out of the loop in:
+ for (gfc_omp_namelist *n = *head; n != NULL; n = n->next) + if
(need_device_ptr_p) + n->u.need_device_ptr = true;
Sure.
* * *
I was about to complain that it didn't handle VALUE + OPTIONAL
correctly, but that's a generic gfortran bug (or two):
->https://gcc.gnu.org/PR118080
* * *
There is a bug - 'nowait' is not propagated. Trying:
!$omp dispatch depend(inout:x) nowait
call g(a)
!$omp end dispatch
gives (-fdump-tree-gimple): #pragma omp taskwait depend(inout:&x) nowait
but doing the equivalent !$omp dispatch depend(inout:x) call g(a) !$omp
end dispatch nowait gives: #pragma omp taskwait depend(inout:&x) i.e.
the 'nowait' got lost. * * *
Fixed and added testcase.
Similar the original C code, which to my knowledge is now
fixed + tested for, there is an issue related to handling nested
function calls.
I think the attached testcase is fine, but it segfaults unless
the default device is the initial device. The problem is that
the pointer conversion also happens for the inner function but
it should only do so for the outer one.
See attached testcase. – I think it can be seen by looking at the
dump (and adding an -fdump-tree-gimple + scan test probably won't
harm, as not everyone has a GPU and we might implement map as
selfmap on APUs).
This is actually not specific to the Fortran FE. So I had to modify the
middle end and the C++ parser as well. See attached pactches.
Otherwise LGTM.
Tobias
Thanks,
--
PAFrom e470fc10269d9bb0a4b263c18f03e289973807c4 Mon Sep 17 00:00:00 2001
From: Paul-Antoine Arras
Date: Fri, 24 May 2024 19:13:50 +0200
Subject: [PATCH 1/4] OpenMP: Fortran front-end support for dispatch +
adjust_args
This patch adds support for the `dispatch` construct and the `adjust_args`
clause to the Fortran front-end.
Handling of `adjust_args` across translation units is missing due to PR115271.
gcc/fortran/ChangeLog:
* dump-parse-tree.cc (show_omp_clauses): Handle novariants and nocontext
clauses.
(show_omp_node): Handle EXEC_OMP_DISPATCH.
(show_code_node): Likewise.
* frontend-passes.cc (gfc_code_walker): Handle novariants and nocontext.
* gfortran.h (enum gfc_statement): Add ST_OMP_DISPATCH.
(symbol_attribute): Add omp_declare_variant_need_device_ptr.
(gfc_omp_clauses): Add novariants and nocontext.
(gfc_omp_declare_variant): Add need_device_ptr_arg_list.
(enum gfc_exec_op): Add EXEC_OMP_DISPATCH.
* match.h (gfc_match_omp_dispatch): Declare.
* openmp.cc (gfc_free_omp_clauses): Free novariants and nocontext
clauses.
(gfc_free
