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++.

* * *

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.

* * *

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.

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 ...").

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.


s/which is not supported yet/which currently is only partially supported/.

At least for 'declare variant' is is supported - as your testcase shows.

* * *

--- 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 ...)

Tobias

Reply via email to