This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Add semantic checks for ordered construct
ClosedPublic

Authored by peixin on Aug 21 2021, 9:53 AM.

Details

Summary

This patch implements the following semantic checks according to
OpenMP Version 5.1 Ordered construct restriction:

At most one threads clause can appear on an ordered construct; At most
one simd clause can appear on an ordered construct; At most one
depend(source) clause can appear on an ordered construct; Either
depend(sink:vec) clauses or depend(source) clauses may appear on an
ordered construct, but not both.

This patch also implements the following semantic checks according to
the syntax and descriptions in OpenMP Version 5.1 Ordered construct:

The dependence types of sink or source are only allowed on an ordered
construct. The depend(*) clauses are not allowed when ordered construct
is a block construct with an ordered region. The threads or simd clauses
are not allowed when the ordered construct is a standalone construct
with no ordered region.

Co-authored-by: Sameeran Joshi <sameeranjayant.joshi@amd.com>

Diff Detail

Event Timeline

peixin created this revision.Aug 21 2021, 9:53 AM
peixin requested review of this revision.Aug 21 2021, 9:53 AM
kiranchandramohan requested changes to this revision.Aug 22 2021, 2:20 PM
kiranchandramohan added a subscriber: kiranktp.

There is a similar patch (https://reviews.llvm.org/D94087) from @sameeranjoshi (AMD) which implements these constraints. Please go through the discussion in that patch. Also,

@kiranktp is it OK if @peixin proceed with this patch. He can use the D94087 as a reference and take it to completion. Peixin can add @sameeranjoshi as a co-author.

flang/lib/Semantics/check-omp-structure.cpp
762

You can use the allowedOnce specification in OMP.td to achieve this. Change allowedClauses to allowedOnceClauses for Ordered.
https://github.com/llvm/llvm-project/blob/79b55e5038324e61a3abf4e6a9a949c473edd858/llvm/include/llvm/Frontend/OpenMP/OMP.td#L491

This revision now requires changes to proceed.Aug 22 2021, 2:20 PM

There is a similar patch (https://reviews.llvm.org/D94087) from @sameeranjoshi (AMD) which implements these constraints. Please go through the discussion in that patch. Also,

@kiranktp is it OK if @peixin proceed with this patch. He can use the D94087 as a reference and take it to completion. Peixin can add @sameeranjoshi as a co-author.

Should be fine @kiranchandramohan. @peixin can take this to completion.

@kiranchandramohan @kiranktp Thanks a lot. I will proceed with this patch to take it to completion and add @sameeranjoshi as a co-author.
In addition, do I need to add D94087 as a reference in commit message?

peixin updated this revision to Diff 368841.Aug 26 2021, 4:23 AM
peixin retitled this revision from [flang][OpenMP] Add semantic check for ordered construct to [flang][OpenMP] Add semantic checks for ordered construct.
peixin edited the summary of this revision. (Show Details)

This patch is based on D94087.
@sameeranjoshi I did some changes from your implementation. Please let me know if you think it is not OK.

I assume that the codes for parsing depend clause in openmp-parsers.cpp and parse-tree.h are not consistent. Is the following fix OK or should we leave it?

diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 5ce6a110c1e6..060c8b4a3060 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3439,9 +3439,9 @@ struct OmpDependSinkVec {
   std::tuple<Name, std::optional<OmpDependSinkVecLength>> t;
 };

-// 2.13.9 depend-type -> IN | OUT | INOUT | SOURCE | SINK
+// 2.13.9 depend-type -> IN | OUT | INOUT
 struct OmpDependenceType {
-  ENUM_CLASS(Type, In, Out, Inout, Source, Sink)
+  ENUM_CLASS(Type, In, Out, Inout)
   WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Type);
 };

Regarding to the last comment, I think the iteration of clause lit is necessary since multiple semantic checks may be needed such as depend(source) depend(sink: undecvar - 1) depend(source).

peixin updated this revision to Diff 370492.Sep 3 2021, 12:24 AM

Replace GetContext.clauseSource() with itr->second->source since GetContext.clauseSource() points the last clause in the clauselist while itr->second->source points the clause being checked. This fix makes the informative error pointing to the right code.

This patch is based on D94087.
@sameeranjoshi I did some changes from your implementation. Please let me know if you think it is not OK.

I assume that the codes for parsing depend clause in openmp-parsers.cpp and parse-tree.h are not consistent. Is the following fix OK or should we leave it?

diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 5ce6a110c1e6..060c8b4a3060 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3439,9 +3439,9 @@ struct OmpDependSinkVec {
   std::tuple<Name, std::optional<OmpDependSinkVecLength>> t;
 };

-// 2.13.9 depend-type -> IN | OUT | INOUT | SOURCE | SINK
+// 2.13.9 depend-type -> IN | OUT | INOUT
 struct OmpDependenceType {
-  ENUM_CLASS(Type, In, Out, Inout, Source, Sink)
+  ENUM_CLASS(Type, In, Out, Inout)
   WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Type);
 };

Please remove SOURCE and SINK if it is not required in OmpDependenceType.

Regarding to the last comment, I think the iteration of clause lit is necessary since multiple semantic checks may be needed such as depend(source) depend(sink: undecvar - 1) depend(source).

Which comment are you referring to here?

peixin updated this revision to Diff 370601.Sep 3 2021, 9:25 AM

@kiranchandramohan Thanks for your review. Delete SOURCE and SINK in dependence type in parser.

The comment what I am referring to is the last inline comment @clementval added as follows:

flang/lib/Semantics/check-omp-structure.cpp
891
What is the final reason to not use FindClause? It would be more efficient than iterate over the clause list once more.

It's in the function OmpStructureChecker::ChecksOnOrderedAsStandalone.

LGTM. A question and a comment.

Thanks for this work. Please submit with @sameeranjoshi as co-author. Please wait a day to see whether there are any other comments.

test/Semantics/omp-ordered01.f90
2 ↗(On Diff #370601)

Nit: Remove the requires and switch to test_errors.py when you submit.

51 ↗(On Diff #370601)

Nit: Is this required?
Can you use a defined variable in the depend(sink) clause?

This revision is now accepted and ready to land.Sep 14 2021, 9:47 AM
peixin updated this revision to Diff 372650.Sep 15 2021, 1:27 AM

Rebase this patch.

@kiranchandramohan Thanks for your review.

Thanks for this work. Please submit with @sameeranjoshi as co-author. Please wait a day to see whether there are any other comments.

I have added @sameeranjoshi as co-author in summary description and will do as you suggest.

Nit: Remove the requires and switch to test_errors.py when you submit.

Done

Nit: Is this required?

Can you use a defined variable in the depend(sink) clause?

Thanks for the notice. This is not required. I have deleted this check. The right way is to check if the variable is the worksharing-loop iteration variable and the loop iterator can be undeclared with implicit integer. I will do it together with other checks for depend(sink: iterator +/- constant) later. Is this ok to you?

Nit: Is this required?

Can you use a defined variable in the depend(sink) clause?

Thanks for the notice. This is not required. I have deleted this check. The right way is to check if the variable is the worksharing-loop iteration variable and the loop iterator can be undeclared with implicit integer. I will do it together with other checks for depend(sink: iterator +/- constant) later. Is this ok to you?

OK with me.

@kiranchandramohan Can I land this patch now?

@kiranchandramohan Can I land this patch now?

Yes.

This revision was automatically updated to reflect the committed changes.