[PATCH v6 4/6] OpenMP: Fortran support for metadirectives and dynamic selectors
Sandra Loosemore
sloosemore@baylibre.com
Wed Jan 29 22:25:24 GMT 2025
On 1/26/25 16:53, Tobias Burnus wrote:
> 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 <kcy@codesourcery.com>
>> Co-Authored-By: Sandra Loosemore <sandra@codesourcery.com>
>> Co-Authored-By: Tobias Burnus <tobias@codesourcery.com>
>> Co-Authored-By: Paul-Antoine Arras <pa@codesourcery.com>
>
> Smaller items:
>
> * uncommenting "metadirective" inside gfc_omp_directives.
> I believe this has already been done locally.
Yes, fixed but not previously posted.
> * 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.
Fixed now. As discussed offline, some care needs to be taken to ensure
there isn't a dangling comma at the end.
> * 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
Likewise ready to go.
>> +++ 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.]
I'm puzzled too, now that I look at it. This testcase was present in
the original OG12 implementation with the same comment and I don't know
the history behind it, but removing the comment seems best.
> --- 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
So I did this and discovered that the pure/metadirective handling is
completely borked, as you went on to say:
> *****************************
> 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
I eventually found this was a fairly easy fix in decode_omp_directive;
just categorize metadirective as pure, and if any of its variants are
not pure they will be separately diagnosed by that function during
parsing of the metadirective variants, without any need for any other
special handling.
> [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.]
Fixed.
>
> 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
This was the result of bit-rot as these other directives were
implemented long after the Fortran support for metadirectives was
originally written (for the OG12 branch), and I wasn't tracking closely
enough when refreshing the series. Things are updated now to know about
these new directives.
> * 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
I filed PR118694 for this.
This one is going to be hard to fix, or at least I don't have any good
ideas on how to fix it. Cases that require late resolution without
dynamic selectors might be hacked around to recognize and skip over an
intervening metadirective between the "target" and "teams", but it
doesn't seem like the code for evaluating dynamic selectors can
generally be hoisted outside of the outer "target".
> * 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.
I'd already been working on this one and have a patch that's almost
ready, but it still needs a bit of tinkering/testing so I'm going to
hold off and commit it separately.
I'll push the attached patch tomorrow, along with the remaining piece of
the series to update the implementation status table in the libgomp
manual, unless you want more time to do another round of review first.
Also need to update the GCC 15 release notes!
-Sandra
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-OpenMP-Fortran-support-for-metadirectives-and-dynami.patch
Type: text/x-patch
Size: 108424 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/fortran/attachments/20250129/bd9a1d35/attachment-0001.bin>
More information about the Fortran
mailing list