Hi Sandra,
this patch LGTM with some minor comments. Or rather:
I have a few minor comments that should be fixed right away
and a few larger items for which PRs should be filed.
See below.
Sandra Loosemore wrote:
gcc/fortran/ChangeLog
PR middle-end/112779
PR middle-end/113904
* decl.cc (gfc_match_end): Handle COMP_OMP_BEGIN_METADIRECTIVE and
COMP_OMP_METADIRECTIVE.
* dump-parse-tree.cc (show_omp_node): Handle EXEC_OMP_METADIRECTIVE.
(show_code_node): Likewise.
* gfortran.h (enum gfc_statement): Add ST_OMP_METADIRECTIVE,
ST_OMP_BEGIN_METADIRECTIVE, and ST_OMP_END_METADIRECTIVE.
(struct gfc_omp_clauses): Rename target_first_st_is_teams to
target_first_st_is_teams_or_meta.
(struct gfc_omp_variant): New.
(gfc_get_omp_variant): New.
(struct gfc_st_label): Add omp_region field.
(enum gfc_exec_op): Add EXEC_OMP_METADIRECTIVE.
(struct gfc_code): Add omp_variants fields.
(gfc_free_omp_variants): Declare.
(match_omp_directive): Declare.
(is_omp_declarative_stmt): Declare.
* io.cc (format_asterisk): Adjust initializer.
* match.h (gfc_match_omp_begin_metadirective): Declare.
(gfc_match_omp_metadirective): Declare.
* openmp.cc (gfc_match_omp_eos): Adjust to match context selectors.
(gfc_free_omp_variants): New.
(gfc_match_omp_clauses): Remove context_selector parameter and adjust
to use gfc_match_omp_eos instead.
(match_omp): Adjust call to gfc_match_omp_clauses.
(gfc_match_omp_context_selector): Add metadirective_p parameter and
adjust error-checking. Adjust matching of simd clauses.
(gfc_match_omp_context_selector_specification): Adjust parameters
so it can be used for metadirective as well as declare variant.
(match_omp_metadirective): New.
(gfc_match_omp_begin_metadirective): New.
(gfc_match_omp_metadirective): New.
(resolve_omp_metadirective): New.
(resolve_omp_target): Handle metadirectives.
(gfc_resolve_omp_directive): Handle EXEC_OMP_METADIRECTIVE.
* parse.cc (gfc_matching_omp_context_selector): New.
(gfc_in_omp_metadirective_body): New.
(gfc_omp_region_count): New.
(decode_omp_directive): Handle ST_OMP_BEGIN_METADIRECTIVE and
ST_OMP_METADIRECTIVE.
(match_omp_directive): New.
(case_omp_structured_block): Define.
(case_omp_do): Define.
(gfc_ascii_statement): Handle ST_OMP_BEGIN_METADIRECTIVE,
ST_OMP_END_METADIRECTIVE, and ST_OMP_METADIRECTIVE.
(accept_statement): Handle ST_OMP_METADIRECTIVE and
ST_OMP_BEGIN_METADIRECTIVE.
(gfc_omp_end_stmt): New, split from...
(parse_omp_do): ...here, and...
(parse_omp_structured_block): ...here. Handle metadirectives.
(parse_omp_metadirective_body): New.
(parse_executable): Handle metadirective. Use new case macros
defined above.
(gfc_parse_file): Initialize metadirective state.
(is_omp_declarative_stmt): New.
* parse.h (enum gfc_compile_state): Add COMP_OMP_METADIRECTIVE
and COMP_OMP_BEGIN_METADIRECTIVE.
(gfc_omp_end_stmt): Declare.
(gfc_matching_omp_context_selector): Declare.
(gfc_in_omp_metadirective_body): Declare.
(gfc_omp_metadirective_region_count): Declare.
* resolve.cc (gfc_resolve_code): Handle EXEC_OMP_METADIRECTIVE.
* st.cc (gfc_free_statement): Likewise.
* symbol.cc (compare_st_labels): Handle labels within a metadirective
body.
(gfc_get_st_label): Likewise.
* trans-decl.cc (gfc_get_label_decl): Encode the metadirective region
in the label_name.
* trans-openmp.cc (gfc_trans_omp_directive): Handle
EXEC_OMP_METADIRECTIVE.
(gfc_trans_omp_set_selector): New, split/adapted from code....
(gfc_trans_omp_declare_variant): ...here.
(gfc_trans_omp_metadirective): New.
* trans-stmt.h (gfc_trans_omp_metadirective): Declare.
* trans.cc (trans_code): Handle EXEC_OMP_METADIRECTIVE.
gcc/testsuite/ChangeLog
PR middle-end/112779
PR middle-end/113904
* gfortran.dg/gomp/metadirective-1.f90: New.
* gfortran.dg/gomp/metadirective-10.f90: New.
* gfortran.dg/gomp/metadirective-11.f90: New.
* gfortran.dg/gomp/metadirective-12.f90: New.
* gfortran.dg/gomp/metadirective-2.f90: New.
* gfortran.dg/gomp/metadirective-3.f90: New.
* gfortran.dg/gomp/metadirective-4.f90: New.
* gfortran.dg/gomp/metadirective-5.f90: New.
* gfortran.dg/gomp/metadirective-6.f90: New.
* gfortran.dg/gomp/metadirective-7.f90: New.
* gfortran.dg/gomp/metadirective-8.f90: New.
* gfortran.dg/gomp/metadirective-9.f90: New.
* gfortran.dg/gomp/metadirective-construct.f90: New.
* gfortran.dg/gomp/metadirective-no-score.f90: New.
* gfortran.dg/gomp/pure-1.f90: Test metadirective.
* gfortran.dg/gomp/pure-2.f90: Remove test for error on metadirective.
libgomp/ChangeLog
PR middle-end/112779
PR middle-end/113904
* testsuite/libgomp.fortran/metadirective-1.f90: New.
* testsuite/libgomp.fortran/metadirective-2.f90: New.
* testsuite/libgomp.fortran/metadirective-3.f90: New.
* testsuite/libgomp.fortran/metadirective-4.f90: New.
* testsuite/libgomp.fortran/metadirective-5.f90: New.
* testsuite/libgomp.fortran/metadirective-6.f90: New.
Co-Authored-By: Kwok Cheung Yeung <k...@codesourcery.com>
Co-Authored-By: Sandra Loosemore <san...@codesourcery.com>
Co-Authored-By: Tobias Burnus <tob...@codesourcery.com>
Co-Authored-By: Paul-Antoine Arras <p...@codesourcery.com>
Smaller items:
* uncommenting "metadirective" inside gfc_omp_directives.
I believe this has already been done locally.
* OpenMP permits (optional) commas as separators between clauses (and since 6.0
also between the directive name (and its arguments, if any) and the first
clause).
[C/C++ likewise, except that the latter is already valid since 5.0 (?).]
i.e.
+ /* Parse the context selectors. */
+ for (;;)
+ {
needs a
gfc_gobble_whitespace ();
gfc_match_char (',');
gfc_gobble_whitespace ();
cf. related
r15-6871-g2ea4801cf7a4eb Accept commas between clauses in OpenMP declare
variant
(same issue, different function)
I guess we want to have at least one testcase for this - or throw just a comma
in here and there in one of the existing tests.
* Change a few %C to %L + &…expr->where for better error location.
(multiple locations). For parser errors, %C itself is fine, but usually
%L is better when something has been successfully parsed but then should
fail for other reasons - be it the duplicate messages (where he locus before
parsing the clause name is better) - or when an expression has issues.
Namely for:
+ gfc_error ("property must be a constant "
it should use &otp->expr->where with %L
And for
+ gfc_error ("too many %<otherwise%> or %<default%> clauses "
+ "in %<metadirective%> at %C");
it should use &variant_locus with %L
+++ b/gcc/testsuite/gfortran.dg/gomp/metadirective-6.f90
...
+! The outer metadirective should be resolved at parse time, but is
+! currently resolved during Gimplification.
I am confused why the outer should be resolvable during parse time;
I see that the inner one can be resolved for each branch of the outer
one - by eliminating it from the default(nothing) branch.
Is this a left-over comment from static conditions?
[Not really critical, it's just a comment in a testcase;
I am just puzzled.]
* * *
--- a/gcc/testsuite/gfortran.dg/gomp/pure-2.f90
+++ b/gcc/testsuite/gfortran.dg/gomp/pure-2.f90
@@ -26,14 +26,6 @@ logical function func_interchange(n)
end do
end
-
-!pure logical function func_metadirective()
-logical function func_metadirective()
- implicit none
- !$omp metadirective ! { dg-error "Unclassifiable OpenMP directive" }
- func_metadirective = .false.
-end
I think removing this is fine, but I think we want to have a testcase like:
! not 'parallel' not pure -> invalid in 5.2; + in general invalid in 5.1
pure logical function func_metadirective()
implicit none
integer :: i, n
n = 0
!$omp metadirective when( ... : parallel do) ! { dg-error "OpenMP directive at (1)
is not pure and thus may not appear in a PURE procedure" }
do i = 1, 5
n = n + i
end do
end do
end
*****************************
Continuing with this topic, but I think all items below
are candidates for PRs (problem reports). Please file/update
them as required, unless you can fix them quickly
(as part of an updated patch - or follow-up patch)
* * *
Additionally, we need to have a PR for the following, can you file one?
OpenMP 5.2 and 6 state:
"A metadirective is pure if and only if all directive variants specified for it are
pure."
Thus, the following is valid:
pure logical function func_metadirective()
implicit none
integer :: i, n
n = 0
!$omp metadirective when( ... : unroll full)
do i = 1, 5
n = n + i
end do
end do
end
as 'unroll' is 'pure'. It currently prints:
Error: OpenMP directive at (1) is not pure and thus may not appear in a PURE
procedure
BTW: We claim that GCC support this 5.2 feature:
"Extended list of directives permitted in Fortran pure procedures | Y |"
in libgomp.texi
* * *
[This one feels wrong from numeric point of view ...
+ real, parameter :: PI_CONST = 3.14159
+ real, parameter :: E_CONST = 2.71828
[No action required - as that is *not* a real code,
but for real code, I'd use
real, parameter :: PI_CONST = 2.0*acos(0.0)
real, parameter :: E_CONST = exp(1.0)
[... not that it should matter with a large epsilon of 0.001.]
* * *
Another item of either fix it or file a PR:
fioo.f90:2:82:
2 | !$omp begin metadirective when(construct={parallel} : nothing)
otherwise(dispatch)
|
1
Error: variant directive used in OMP BEGIN METADIRECTIVE at (1) must have a
corresponding end directive
fioo.f90:4:23:
4 | !$omp end metadirective
| 1
Error: Unexpected !$OMP END METADIRECTIVE statement at (1)
That's for:
!---------------------
external f
!$omp begin metadirective when(construct={parallel} : nothing)
otherwise(dispatch)
call f()
!$omp end metadirective
end
!---------------------
* * *
Likewise, for:
------------------
integer :: x
!$omp atomic update
x = x + 1
!$omp atomic update
x = x + 1
!$omp end atomic
!$omp begin metadirective when(construct={parallel} : nothing) otherwise(atomic
update)
x = x + 1
!$omp end metadirective
end
------------------
8 | !$omp begin metadirective when(construct={parallel} : nothing)
otherwise(atomic update)
|
1
Error: variant directive used in OMP BEGIN METADIRECTIVE at (1) must have a
corresponding end directive
* * *
* Unless it is quickly fixable, we agreed on deferring the bogus message
"Error: ‘target’ construct with nested ‘teams’ construct contains directives
outside of the ‘teams’ construct"
to a new PR. That's for:
OpenMP_VV's tests/5.0/metadirective/test_metadirective_arch_is_nvidia.F90
tests/5.0/metadirective/test_metadirective_arch_is_nvidia.F90:42:84:
42 | !$omp target map(to:v1,v2) map(from:v3,target_device_num)
device(default_device)
|
^
Error: ‘target’ construct with nested ‘teams’ construct
contains directives outside of the ‘teams’ construct
Likewise with the OpenMP examples document:
program_control/sources/metadirective.1.f90:12:53:
12 | !$omp target map(to:v1,v2) map(from:v3) device(0)
| ^
Error: ‘target’ construct with nested ‘teams’ construct contains directives
outside of the ‘teams’ construct
* * *
* Having code between END BLOCK and !$omp end metadirective gives an ICE;
That is already tacked as https://gcc.gnu.org/PR107067
Probably, the PR should be updated to mention testcase? [It is
marked as dg-ice, i.e. even when not mentioning it will show up as
XPASS once fixed.]
And the PR could be also be mentioned in
gfortran.dg/gomp/metadirective-11.f90's
dg-ice message, I guess.
* * *
Thanks,
Tobias
