Re: [PATCH] Fortran/OpenMP: event handle in task detach cannot be a coarray [PR104131]
Dear all,
Tobias argued in the PR that the testcase should actually be valid.
Therefore withdrawing the patch.
Sorry for expecting this to be a low-hanging fruit...
Harald
On 10/24/23 22:23, rep.dot@gmail.com wrote:
On 24 October 2023 21:25:01 CEST, Harald Anlauf wrote:
Dear all,
the attached simple patch adds a forgotten check that an event handle
cannot be a coarray. This case appears to have been overlooked in the
original fix for this PR.
I intend to commit as obvious within 24h unless there are comments.
diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
index 1cc65d7fa49..08081dacde4 100644
--- a/gcc/fortran/openmp.cc
+++ b/gcc/fortran/openmp.cc
@@ -8967,6 +8967,9 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses
*omp_clauses,
else if (omp_clauses->detach->symtree->n.sym->attr.dimension > 0)
gfc_error ("The event handle at %L must not be an array element",
_clauses->detach->where);
+ else if (omp_clauses->detach->symtree->n.sym->attr.codimension)
+ gfc_error ("The event handle at %L must not be a coarray",
ISTM that we usually do not mention "element" when talking about undue
(co)array access.
Maybe we want to streamline this specific error message?
LGTM otherwise.
Thanks for your dedication!
+ _clauses->detach->where);
else if (omp_clauses->detach->symtree->n.sym->ts.type == BT_DERIVED
|| omp_clauses->detach->symtree->n.sym->ts.type == BT_CLASS)
gfc_error ("The event handle at %L must not be part of "
Re: [PATCH] Fortran/OpenMP: event handle in task detach cannot be a coarray [PR104131]
On 24 October 2023 21:25:01 CEST, Harald Anlauf wrote:
>Dear all,
>
>the attached simple patch adds a forgotten check that an event handle
>cannot be a coarray. This case appears to have been overlooked in the
>original fix for this PR.
>
>I intend to commit as obvious within 24h unless there are comments.
diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
index 1cc65d7fa49..08081dacde4 100644
--- a/gcc/fortran/openmp.cc
+++ b/gcc/fortran/openmp.cc
@@ -8967,6 +8967,9 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses
*omp_clauses,
else if (omp_clauses->detach->symtree->n.sym->attr.dimension > 0)
gfc_error ("The event handle at %L must not be an array element",
_clauses->detach->where);
+ else if (omp_clauses->detach->symtree->n.sym->attr.codimension)
+ gfc_error ("The event handle at %L must not be a coarray",
ISTM that we usually do not mention "element" when talking about undue
(co)array access.
Maybe we want to streamline this specific error message?
LGTM otherwise.
Thanks for your dedication!
+ _clauses->detach->where);
else if (omp_clauses->detach->symtree->n.sym->ts.type == BT_DERIVED
|| omp_clauses->detach->symtree->n.sym->ts.type == BT_CLASS)
gfc_error ("The event handle at %L must not be part of "
[PATCH] Fortran/OpenMP: event handle in task detach cannot be a coarray [PR104131]
Dear all,
the attached simple patch adds a forgotten check that an event handle
cannot be a coarray. This case appears to have been overlooked in the
original fix for this PR.
I intend to commit as obvious within 24h unless there are comments.
Thanks,
Harald
From 2b5ed32cacfe84dc4df74b4dccf16ac830d9eb98 Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Tue, 24 Oct 2023 21:18:02 +0200
Subject: [PATCH] Fortran/OpenMP: event handle in task detach cannot be a
coarray [PR104131]
gcc/fortran/ChangeLog:
PR fortran/104131
* openmp.cc (resolve_omp_clauses): Add check that event handle is
not a coarray.
gcc/testsuite/ChangeLog:
PR fortran/104131
* gfortran.dg/gomp/pr104131-2.f90: New test.
---
gcc/fortran/openmp.cc | 3 +++
gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90 | 12
2 files changed, 15 insertions(+)
create mode 100644 gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90
diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
index 1cc65d7fa49..08081dacde4 100644
--- a/gcc/fortran/openmp.cc
+++ b/gcc/fortran/openmp.cc
@@ -8967,6 +8967,9 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
else if (omp_clauses->detach->symtree->n.sym->attr.dimension > 0)
gfc_error ("The event handle at %L must not be an array element",
_clauses->detach->where);
+ else if (omp_clauses->detach->symtree->n.sym->attr.codimension)
+ gfc_error ("The event handle at %L must not be a coarray",
+ _clauses->detach->where);
else if (omp_clauses->detach->symtree->n.sym->ts.type == BT_DERIVED
|| omp_clauses->detach->symtree->n.sym->ts.type == BT_CLASS)
gfc_error ("The event handle at %L must not be part of "
diff --git a/gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90 b/gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90
new file mode 100644
index 000..3978a6ac31a
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90
@@ -0,0 +1,12 @@
+! { dg-do compile }
+! { dg-options "-fopenmp -fcoarray=single" }
+! PR fortran/104131 - event handle cannot be a coarray
+
+program p
+ use iso_c_binding, only: c_intptr_t
+ implicit none
+ integer, parameter :: omp_event_handle_kind = c_intptr_t
+ integer (kind=omp_event_handle_kind) :: x[*]
+!$omp task detach (x) ! { dg-error "The event handle at \\\(1\\\) must not be a coarray" }
+!$omp end task
+end
--
2.35.3
Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr)
Hi PA, hello all,
First, I hesitate to review/approve a patch I am involved in; Thus, I would like
if someone could have a second look.
Regarding the patch itself:
On 20.10.23 16:02, Paul-Antoine Arraswrote:
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):
[...]
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?
...
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.
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.
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.
(Besides: It keeps getting outdated by any other commit to that file.)
As you have a commit, running the following, possibly with the commit hash as
argument (unless it is the last commit) will show that the nightly script will
use:
./contrib/gcc-changelog/git_check_commit.py -v -p
It is additionally a good check whether you got the syntax right. (This is run
as pre-commit hook.)
* * *
Regarding the patch, I think it will work, but I wonder whether we can do
better - esp. regarding c_ptr vs. c_funptr.
I started by looking why the current code fails:
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)
-
- 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 does not work because the pointers to the derived type are different:
(gdb) p *ts1
$10 = {type = BT_INTEGER, kind = 8, u = {derived = 0x30c66b0, cl = 0x30c66b0,
pad = 51144368}, interface = 0x0, is_c_interop = 1, is_iso_c = 0, f90_type =
BT_VOID, deferred = false,
interop_kind = 0x0}
(gdb) p *ts2
$11 = {type = BT_DERIVED, kind = 0, u = {derived = 0x30c2930, cl = 0x30c2930,
pad = 51128624}, interface = 0x0, is_c_interop = 0, is_iso_c = 0, f90_type =
BT_UNKNOWN,
deferred = false, interop_kind = 0x0}
The reason seems to be that they are freshly created
in different namespaces. Consequently, attr.use_assoc = 1
and the namespace is different, i.e.
(gdb) p ts1->u.derived->ns->proc_name->name
$18 = 0x76ff4138 "foo"
(gdb) p ts2->u.derived->ns->proc_name->name
$19 = 0x76ffc260 "foo_variant"
* * *
Having said this, I think we can combine the current
and the modified version, i.e.
+ 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))
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.
While in this example, the name pointers are identical, I fear that
won't hold in some more complex indirect use via module-use. Thus,
strcmp is used.
(gdb) p ts1->u.derived->name
$13 = 0x76ff4100 "c_ptr"
(gdb) p ts2->u.derived->name
$14 = 0x76ff4100 "c_ptr"
* * *
Additionally, I think it would be good to have a testcase which checks for
c_funptr vs. c_ptr
mismatch.
Just changing c_ptr to c_funptr in the testcase (+ commenting the c_f_pointer)
prints:
Error: variant ‘foo_variant’ and base ‘foo’ at (1) have incompatible types:
Type mismatch in argument 'c_bv' (INTEGER(8)/TYPE(c_funptr))
I think that would be a good invalid testcase besides the valid one.
But with a tweak to get better messages to give:
Error: variant ‘foo_variant’ and base ‘foo’ at (1) have incompatible types:
Type mismatch in argument 'c_bv' (TYPE(c_ptr)/TYPE(c_funptr))
cf. misc.cc in the
[Patch] OpenMP/Fortran: Handle unlisted items in 'omp allocators' + exec. 'omp allocate'
This patch assumes that EXEC_OMP_ALLOCATE/EXEC_OMP_ALLOCATORS is/will be later handled as currently done in OG13, https://github.com/gcc-mirror/gcc/blob/devel/omp/gcc-13/gcc/fortran/trans-openmp.cc Depending how we want to handle it in mainline, the patch still could make sense - or parts should be modified, e.g. we might want to handle standard Fortran allocation (malloc) and OpenMP one (GOMP_alloc) directly in trans-sstmt.cc; if so, we might want to skip adding another allocate-stmt. - We probably still want to do the 'allocate' and diagnostic hanlding in openmp.cc in all cases. In any case, we surely need to handle on mainline: * the dump-parse-tree.cc patch is surely needed and without removing the empty entry (n->sym == NULL), it needs an additional fix in order not to crash. * Rejecting coarrays in the empty-list case, which presumably makes most sense inside openmp.cc. * * * On mainline, an executable '!$omp allocate' / '!$omp allocators' stops in trans-openmp.cc with a sorry, not yet implemented. However, OG13 has some implementation for executable '!$omp allocate'; trying to merge mainline into OG13, I found the following issues: * -fdump-parse-tree did not dump the clauses (trivial issue) (simple oversight) * The not-specified items should be better handled => done now during resolution in openmp.cc. * * * While -fdump-tree-original can be used to test it, the "sorry" makes it hard to write a testsuite test. Some testcases exist like gfortran.dg/gomp/allocate-5.f90, which contains code similar to the last example, but it is only a parse + 'sorry'-shows-up testcase. (Well, the two new 'error:' cases can be tested and are tested but they are more boring.) * * * The spec states: For !$omp allocators allocate(align(4):a,b) allocate(a,b,c,d) only a and b are allocated with an OpenMP allocator (→ omp_get_default_allocator()) and alignment of 4. - 'c' and 'd' are allocated in the normal Fortran way. The deprecated works as follows: !$omp allocate(a,b) align(4) !$omp allocate align(16) ! not: no list argument after 'allocate') allocate(a,b,c,d) where a and b will be allocated with an alignment of 4 and the rest, here, c and d, with the settings of the directive without argument list, i.e. c and d are allocated with an alignment of 16. The question is what is supposed to happen for: !$omp allocate(a,b) align(4) allocate(a,b,c,d) Should that use the default allocator for c and d, i.e. the same as !$omp allocate(a,b) align(4) !$omp allocate allocate(a,b,c,d) Or should it use the normal Fortran allocator, following what 'allocators' does? The spec does not really tell (and that syntax is deprecated in 5.2, removed in TR11/OpenMP 6). Thus, GCC now prints an error. However, it would be trivial to choose either of the other variants. * * * The attached patch now handles the not-specified items: * It adds them in the last case to the list; namelist->sym == NULL is the no-arguments case; this item is also removed, avoiding n->sym == NULL special cases later on. * For the first two cases, a new Fortran ALLOCATE statement is created, containing the non-treated items. Comments, suggestions, remarks? 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 OpenMP: Handle unlisted items in 'omp allocators' + exec. 'omp allocate' gcc/fortran/ChangeLog: * dump-parse-tree.cc (show_omp_node): Show clauses for EXEC_OMP_ALLOCATE and EXEC_OMP_ALLOCATORS. * openmp.cc (resolve_omp_clauses): Process nonlisted items for EXEC_OMP_ALLOCATE and EXEC_OMP_ALLOCATORS. gcc/testsuite/ChangeLog: * gfortran.dg/gomp/allocate-14.f90: Add new checks. * gfortran.dg/gomp/allocate-5.f90: Remove items from an allocate-stmt that are not explicitly/implicited listed in 'omp allocate'. gcc/fortran/dump-parse-tree.cc | 2 + gcc/fortran/openmp.cc | 112 - gcc/testsuite/gfortran.dg/gomp/allocate-14.f90 | 41 + gcc/testsuite/gfortran.dg/gomp/allocate-5.f90 | 4 +- 4 files changed, 155 insertions(+), 4 deletions(-) diff --git a/gcc/fortran/dump-parse-tree.cc b/gcc/fortran/dump-parse-tree.cc index 68122e3e6fd..1440524f971 100644 --- a/gcc/fortran/dump-parse-tree.cc +++ b/gcc/fortran/dump-parse-tree.cc @@ -2241,6 +2241,8 @@ show_omp_node (int level, gfc_code *c) case EXEC_OACC_CACHE: case EXEC_OACC_ENTER_DATA: case EXEC_OACC_EXIT_DATA: +case EXEC_OMP_ALLOCATE: +case EXEC_OMP_ALLOCATORS: case EXEC_OMP_ASSUME: case EXEC_OMP_CANCEL: case EXEC_OMP_CANCELLATION_POINT: diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc index 1cc65d7fa49..95e0aaafa58 100644 --- a/gcc/fortran/openmp.cc +++ b/gcc/fortran/openmp.cc @@ -7924,10 +7924,14 @@ resolve_omp_clauses
Re: OpenMP/Fortran: Group handling of 'if' clause without and with modifier
CC: fortran@ for completeness. On 24.10.23 10:55, Thomas Schwinge wrote: OK to push (after testing) the attached "OpenMP/Fortran: Group handling of 'if' clause without and with modifier"? That makes an upcoming change a bit lighter. LGTM. (The patch just moves some code up (in the same functions) such that 'if()' and 'if(:)' are next to each other.) Tobias From a6e15fe6b08e2ced98435739506f9fc10db96a63 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Tue, 24 Oct 2023 10:43:40 +0200 Subject: [PATCH] OpenMP/Fortran: Group handling of 'if' clause without and with modifier The 'if' clause with modifier was introduced in commit b4c3a85be96585374bf95c981ba2f602667cf5b7 (Subversion r242037) "Partial OpenMP 4.5 fortran support", but -- in some instances -- didn't place it next to the existing handling of 'if' clause without modifier. Unify that; no change in behavior. gcc/fortran/ * dump-parse-tree.cc (show_omp_clauses): Group handling of 'if' clause without and with modifier. * frontend-passes.cc (gfc_code_walker): Likewise. * gfortran.h (gfc_omp_clauses): Likewise. * openmp.cc (gfc_free_omp_clauses): Likewise. - 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
