Hi Tobias,
Here are an updated patch and a few questions.
On 07/01/2025 13:18, Tobias Burnus wrote:
Paul-Antoine Arras:
Add support to the Fortran parser for the new OpenMP syntax that allows a
comma after the directive name and between clauses of declare variant.
The C and C++ parsers already support this syntax so only a new test
is added.
Note: only the optional comma between directive name and (first) clause
is new (since 5.2). The one between clauses is old (since OpenMP 2.5).
While the patch supports both, 'new OpenMP syntax' is a bit misleading.
For 'declare variant', the comma-between-clauses support is now required
as only with 5.1 and this patch set, more clauses ('adjust_args' and
'append_args') are supported.
This second type of comma (between directive and first clause) is
supported in C/C++ since OpenMP 5.1; I think mainly because of the
added [[omp::directive(...)]] C++11 attribute feature.
In Fortran, this comma is only permitted since 5.2; I think mostly
for consistency with C/C++.
I rephrased the commit message by just removing "new". Should it be more
detailed?
* * *
BTW: Adding support for this comma for all directives is tracked as
to-be-done Fortran item for 5.2 (under other features as Appendix B
does not list it.)
→ https://gcc.gnu.org/onlinedocs/libgomp/OpenMP-5_002e2.html
* * *
--- a/gcc/fortran/openmp.cc
+++ b/gcc/fortran/openmp.cc
@@ -6595,6 +6595,10 @@ gfc_match_omp_declare_variant (void)
for (;;)
{
+ gfc_gobble_whitespace ();
+ gfc_match_char (',');
+ gfc_gobble_whitespace ();
+
I think that will do, but having a better error would be IMHO better.
For the first fail, we get:
13 | !$omp declare variant(f) ,,match(construct={dispatch})
adjust_args(need_device_ptr : c)
| 1
Error: expected ‘match’ or ‘adjust_args’ at (1)
which is quite helpful. But moving the ,, later gives:
13 | !$omp declare variant(f)
match(construct={dispatch}),,adjust_args(need_device_ptr : c)
| 1
Error: Invalid character in name at (1)
That's kind of helpful but not really useful.
And adding a valid but not yet supported clause (it's supported for C/C++):
13 | !$omp declare variant(f)
match(construct={dispatch}),adjust_args(need_device_ptr : c),append_args(a)
| 1
Error: Unclassifiable statement at (1)
I think it would be better to replace the first_p by, e.g.,
'error_p = true; break;' – and do the this diagnostic
after the for(;;) loop. This seems to yield a better diagnostic.
I am not sure I am getting that part. Is this what you are suggesting?
diff --git gcc/fortran/openmp.cc gcc/fortran/openmp.cc
index 9d255558dc9..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 %<match%> or %<adjust_args%> at %C");
- return MATCH_ERROR;
- }
- else
- break;
+ if (!has_match)
+ error_p = true;
+ break;
}
if (gfc_match (" (") != MATCH_YES)
@@ -6666,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)
+ {
+ gfc_error ("expected %<match%> or %<adjust_args%> at %C");
+ return MATCH_ERROR;
}
if (has_adjust_args && !has_match)
======
If so, it does yield a better diagnostic ("expected 'match' or
'adjust_args' at (1)") for this testcase; but it completely breaks other
cases where the rest of the line is non empty. For instance, with an
end-of-line comment:
29 | !$omp declare variant (f0) adjust_args (nothing: a) !
{ dg-error "an 'adjust_args' clause at .1. can only be specified if the
'dispatch' selector of the construct selector set appears in the 'match'
clause" }
| 1
* * *
diff --git a/gcc/testsuite/c-c++-common/gomp/adjust-args-5.c b/gcc/
testsuite/c-c++-common/gomp/adjust-args-5.c
new file mode 100644
index 00000000000..863b77458e4
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/adjust-args-5.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+
+/* Check that the OpenMP 6 syntax with commas after the directive
name and
+ between clauses is supported. */
The comment is wrong - For C/C++, that's 5.1 syntax.
Fixed.
diff --git a/gcc/testsuite/gfortran.dg/gomp/adjust-args-11.f90 b/gcc/
testsuite/gfortran.dg/gomp/adjust-args-11.f90
new file mode 100644
index 00000000000..3b26f1b0868
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/gomp/adjust-args-11.f90
@@ -0,0 +1,45 @@
+! { dg-do compile }
+
+! Check that the OpenMP 5.1 syntax with commas between clauses is
supported.
+! A comma after the directive name is introduced in 5.2, which is not
supported
+! yet.
OpenMP 5.1 is slightly misleading (it's 2.5 but new for declare variant
as it didn't have more than one clause before). I think I would remove
'5.1' (i.e. make it: "... that the OpenMP syntax with commas ...").
Removed '5.1' as suggested.
A side: It's supported by the parser, but you could vary the tests
by having no spaces around the comma vs. having a comma before, after,
or before and after the comma.
Added suggested variations around commas and spaces.
s/which is not supported yet/which currently is only partially supported/.
At least for 'declare variant' is is supported - as your testcase shows.
Rephrased as suggested.
* * *
--- a/gcc/testsuite/gfortran.dg/gomp/declare-variant-2.f90
+++ b/gcc/testsuite/gfortran.dg/gomp/declare-variant-2.f90
@@ -182,9 +182,6 @@ contains
- subroutine f75 ()
- !$omp declare variant (f1),match(construct={parallel}) ! { dg-
error "expected 'match' or 'adjust_args' at .1." }
- end subroutine
I think you could replace the removed test case by one or all snippets
from above. (Well, better not 'append_args' as that will be supported
soon, maybe something like 'unknown' or an existing but invalid clause
like 'nowait' or ...)
Added snippets as suggested.
Thanks,
--
PA
commit 4c328bdd3af9f1f358f32562a9a5a1f61dd1e319
Author: Paul-Antoine Arras <par...@baylibre.com>
Date: Mon Jan 6 18:38:11 2025 +0100
Accept commas between clauses in OpenMP declare variant
Add support to the Fortran parser for the OpenMP syntax that allows a comma
after the directive name and between clauses of declare variant. The C and C++
parsers already support this syntax so only a new test is added.
gcc/fortran/ChangeLog:
* openmp.cc (gfc_match_omp_declare_variant): Match comma after directive
name and between clauses.
gcc/testsuite/ChangeLog:
* gfortran.dg/gomp/declare-variant-2.f90: Remove error test for a comma
after the directive name. Add tests for other invalid syntaxes (extra
comma and invalid clause).
* c-c++-common/gomp/adjust-args-5.c: New test.
* gfortran.dg/gomp/adjust-args-11.f90: New test.
diff --git gcc/fortran/openmp.cc gcc/fortran/openmp.cc
index 79c0f1b2e62..9d255558dc9 100644
--- gcc/fortran/openmp.cc
+++ gcc/fortran/openmp.cc
@@ -6595,6 +6595,10 @@ gfc_match_omp_declare_variant (void)
for (;;)
{
+ gfc_gobble_whitespace ();
+ gfc_match_char (',');
+ gfc_gobble_whitespace ();
+
enum clause
{
match,
diff --git gcc/testsuite/c-c++-common/gomp/adjust-args-5.c gcc/testsuite/c-c++-common/gomp/adjust-args-5.c
new file mode 100644
index 00000000000..8aff73cc6e5
--- /dev/null
+++ gcc/testsuite/c-c++-common/gomp/adjust-args-5.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+
+/* Check that the OpenMP 5.1 syntax with commas after the directive name and
+ between clauses is supported. */
+
+int f (int a, void *b, float c[2]);
+
+#pragma omp declare variant (f), match (construct={dispatch}), adjust_args (nothing: a), adjust_args (need_device_ptr: b, c)
+int f0 (int a, void *b, float c[2]);
+
+int test () {
+ int a;
+ void *b;
+ float c[2];
+ struct {int a;} s;
+
+ #pragma omp dispatch, novariants(0), nocontext(1)
+ s.a = f0 (a, b, c);
+
+ return s.a;
+}
diff --git gcc/testsuite/gfortran.dg/gomp/adjust-args-11.f90 gcc/testsuite/gfortran.dg/gomp/adjust-args-11.f90
new file mode 100644
index 00000000000..d9fe7ca4cd7
--- /dev/null
+++ gcc/testsuite/gfortran.dg/gomp/adjust-args-11.f90
@@ -0,0 +1,45 @@
+! { dg-do compile }
+
+! Check that the OpenMP syntax with commas between clauses is supported.
+! A comma after the directive name is introduced in 5.2, which currently is only
+! partially supported.
+
+module main
+ use iso_c_binding, only: c_ptr
+ implicit none
+
+ type :: struct
+ integer :: a
+ real :: b
+ end type
+
+ interface
+ integer function f(a, b, c)
+ import c_ptr
+ integer, intent(in) :: a
+ type(c_ptr), intent(inout) :: b
+ type(c_ptr), intent(out) :: c(:)
+ end function
+ integer function f0(a, b, c)
+ import c_ptr
+ integer, intent(in) :: a
+ type(c_ptr), intent(inout) :: b
+ type(c_ptr), intent(out) :: c(:)
+ !$omp declare variant (f), match (construct={dispatch}) , &
+ !$omp& adjust_args (nothing: a) ,adjust_args (need_device_ptr: b),adjust_args (need_device_ptr: c)
+ end function
+ end interface
+
+contains
+subroutine test
+ integer :: a
+ type(c_ptr) :: b
+ type(c_ptr) :: c(2)
+ type(struct) :: s
+
+ !!$omp dispatch, nocontext(.false.), novariants(.false.) ! Not supported yet
+ !$omp dispatch nocontext(.false.), novariants(.false.)
+ s%a = f0 (a, b, c)
+
+end subroutine
+end module
diff --git gcc/testsuite/gfortran.dg/gomp/declare-variant-2.f90 gcc/testsuite/gfortran.dg/gomp/declare-variant-2.f90
index 62d2cb96fac..17b112f065b 100644
--- gcc/testsuite/gfortran.dg/gomp/declare-variant-2.f90
+++ gcc/testsuite/gfortran.dg/gomp/declare-variant-2.f90
@@ -182,8 +182,14 @@ contains
subroutine f74 ()
!$omp declare variant (f1) match(construct={requires}) ! { dg-warning "unknown selector 'requires' for context selector set 'construct' at .1." }
end subroutine
- subroutine f75 ()
- !$omp declare variant (f1),match(construct={parallel}) ! { dg-error "expected 'match' or 'adjust_args' at .1." }
+ subroutine f75a ()
+ !$omp declare variant(f1) ,,match(construct={dispatch}) adjust_args(need_device_ptr : c) ! { dg-error "expected 'match' or 'adjust_args' at .1." }
+ end subroutine
+ subroutine f75b ()
+ !$omp declare variant(f1) match(construct={dispatch}),,adjust_args(need_device_ptr : c) ! { dg-error "expected 'match' or 'adjust_args' at .1." }
+ end subroutine
+ subroutine f75c ()
+ !$omp declare variant(f1) match(construct={dispatch}),nowait(a) ! { dg-error "expected 'match' or 'adjust_args' at .1." }
end subroutine
subroutine f76 ()
!$omp declare variant (f1) match(implementation={atomic_default_mem_order("relaxed")}) ! { dg-error "expected identifier at .1." }
