Accept commas between clauses in OpenMP declare variant
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
Commit Message
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.
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.
* c-c++-common/gomp/adjust-args-5.c: New test.
* gfortran.dg/gomp/adjust-args-11.f90: New test.
---
gcc/fortran/openmp.cc | 4 ++
.../c-c++-common/gomp/adjust-args-5.c | 21 +++++++++
.../gfortran.dg/gomp/adjust-args-11.f90 | 45 +++++++++++++++++++
.../gfortran.dg/gomp/declare-variant-2.f90 | 3 --
4 files changed, 70 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/gomp/adjust-args-5.c
create mode 100644 gcc/testsuite/gfortran.dg/gomp/adjust-args-11.f90
Comments
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
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,
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 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)
if (gfc_match_omp_eos () != MATCH_YES)
> + 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)
if (error || (!has_match && !has_adjust_args))
as the missing 'match' is handled more explicitly by the next error.
> + {
> + gfc_error ("expected %<match%> or %<adjust_args%> at %C");
> + return MATCH_ERROR;
> }
* * *
The rest looks good to me.
Thanks,
Tobias
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 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)
>
>
> if (gfc_match_omp_eos () != MATCH_YES)
>
Yes, exactly what I was missing!
>> + 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)
>
>
> if (error || (!has_match && !has_adjust_args))
>
> as the missing 'match' is handled more explicitly by the next error.
Right.
>> + {
>> + gfc_error ("expected %<match%> or %<adjust_args%> at %C");
>> + return MATCH_ERROR;
>> }
>
> * * *
>
> The rest looks good to me.
>
> Thanks,
>
> Tobias
>
>
Applied, tested and committed.
Thanks,
@@ -6595,6 +6595,10 @@ gfc_match_omp_declare_variant (void)
for (;;)
{
+ gfc_gobble_whitespace ();
+ gfc_match_char (',');
+ gfc_gobble_whitespace ();
+
enum clause
{
match,
new file mode 100644
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+
+/* Check that the OpenMP 6 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;
+}
new file mode 100644
@@ -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.
+
+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, 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
@@ -182,9 +182,6 @@ 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." }
- end subroutine
subroutine f76 ()
!$omp declare variant (f1) match(implementation={atomic_default_mem_order("relaxed")}) ! { dg-error "expected identifier at .1." }
end subroutine