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

[peixin]

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 <[e-mail]>

[kiranchandramohan]

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

[kiranktp]

In D108512#2959209, @kiranchandramohan wrote:

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.

[peixin]

@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]

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]

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.

[kiranchandramohan]

In D108512#2966936, @peixin wrote:

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]

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

[kiranchandramohan]

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?

[peixin]

Rebase this patch.

[peixin]

@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?

[kiranchandramohan]

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.

[peixin]

@kiranchandramohan Can I land this patch now?

[kiranchandramohan]

In D108512#3005355, @peixin wrote:

@kiranchandramohan Can I land this patch now?

Yes.

flang/lib/Semantics/check-omp-structure.h

private:
   void CheckCycleConstraints(const parser::OpenMPLoopConstruct &x);
   void CheckDistLinear(const parser::OpenMPLoopConstruct &x);
   void CheckSIMDNest(const parser::OpenMPConstruct &x);
   void CheckTargetNest(const parser::OpenMPConstruct &x);
   void CheckCancellationNest(
       const parser::CharBlock &source, const parser::OmpCancelType::Type &type);
   std::int64_t GetOrdCollapseLevel(const parser::OpenMPLoopConstruct &x);
   void CheckIfDoOrderedClause(const parser::OmpBlockDirective &blkDirectiv);
+  void CheckAllowedOnceClause(const llvm::omp::Clause);
   bool CheckReductionOperators(const parser::OmpClause::Reduction &);
   bool CheckIntrinsicOperator(
       const parser::DefinedOperator::IntrinsicOperator &);
   void CheckReductionTypeList(const parser::OmpClause::Reduction &);
   void CheckMasterNesting(const parser::OpenMPBlockConstruct &x);
   void CheckBarrierNesting(const parser::OpenMPSimpleStandaloneConstruct &x);
   void CheckReductionArraySection(const parser::OmpObjectList &ompObjectList);
   void CheckIntentInPointerAndDefinable(

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

if (blkDirective.v == llvm::omp::OMPD_ordered) {
     else if (IsCloselyNestedRegion(llvm::omp::nestedOrderedErrSet)) {
       context_.Say(blkDirective.source,
           "`ORDERED` region may not be closely nested inside of "
           "`CRITICAL`, `ORDERED`, explicit `TASK` or `TASKLOOP` region."_err_en_US);
     }
   }
 }
 
+void OmpStructureChecker::CheckAllowedOnceClause(
+    const llvm::omp::Clause clause) {
+  auto clauseAll = FindClauses(clause);
+  unsigned clauseCnt = 0;
+  for (auto itr = clauseAll.first; itr != clauseAll.second; ++itr) {
+    clauseCnt++;
+    if (clauseCnt > 1) {
+      context_.Say(itr->second->source,
+          "At most one %s clause can appear on the %s directive"_err_en_US,
+          parser::ToUpperCaseLetters(getClauseName(clause).str()),
+          ContextDirectiveAsFortran());
+      break;
+    }
+  }
+}
+
 void OmpStructureChecker::Leave(const parser::OpenMPBlockConstruct &) {
   if (GetDirectiveNest(TargetBlockOnlyTeams)) {
     ExitDirectiveNest(TargetBlockOnlyTeams);
   }
   if (GetContext().directive == llvm::omp::Directive::OMPD_target) {
     ExitDirectiveNest(TargetNest);
   }
   dirContext_.pop_back();
void OmpStructureChecker::Leave(const parser::OmpClauseList &) {
   } // SIMD
 
   // 2.7.3 Single Construct Restriction
   if (GetContext().directive == llvm::omp::Directive::OMPD_end_single) {
     CheckNotAllowedIfClause(
         llvm::omp::Clause::OMPC_copyprivate, {llvm::omp::Clause::OMPC_nowait});
   }
 
+  // 2.19.9 Ordered Construct Restriction
+  if (GetContext().directive == llvm::omp::Directive::OMPD_ordered) {
+    // At most one threads clause can appear on an ordered construct.
+    CheckAllowedOnceClause(llvm::omp::Clause::OMPC_threads);
+    // At most one simd clause can appear on an ordered construct.
+    CheckAllowedOnceClause(llvm::omp::Clause::OMPC_simd);
+    // At most one depend(source) clause can appear on an ordered construct.
+    auto clauseAll = FindClauses(llvm::omp::Clause::OMPC_depend);
+    unsigned dependSourceCnt = 0;
+    for (auto itr = clauseAll.first; itr != clauseAll.second; ++itr) {
+      const auto &dependClause{
+          std::get<parser::OmpClause::Depend>(itr->second->u)};
+      if (std::get_if<parser::OmpDependClause::Source>(&dependClause.v.u)) {
+        dependSourceCnt++;
+        if (dependSourceCnt > 1) {
+          context_.Say(itr->second->source,
+              "At most one DEPEND(SOURCE) clause can appear on the ORDERED "
+              "directive"_err_en_US);
+          break;
+        }
+      }
+    }
+  }
+
   CheckRequireAtLeastOneOf();
 }
 
 void OmpStructureChecker::Enter(const parser::OmpClause &x) {
   SetContextClause(x);
 }
 
 // Following clauses do not have a separate node in parse-tree.h.

flang/test/Semantics/omp-ordered01.f90

+! RUN: %S/test_errors.sh %s %t %flang_fc1 -fopenmp
+! REQUIRES: shell
+
+! OpenMP Version 5.1
+! Check OpenMP construct validity for the following directives:
+! 2.19.9 Ordered Construct
+
+program main
+  integer :: i, N = 10
+  real :: a, arrayA(10), arrayB(10)
+  real, external :: foo, bar
+
+  !$omp do ordered
+  do i = 1, N
+    !ERROR: At most one THREADS clause can appear on the ORDERED directive
+    !$omp ordered threads threads
+    arrayA(i) = i
+    !$omp end ordered
+  end do
+  !$omp end do
+
+  !$omp simd
+  do i = 1, N
+    !ERROR: At most one SIMD clause can appear on the ORDERED directive
+    !$omp ordered simd simd
+    arrayA(i) = i
+    !$omp end ordered
+  end do
+  !$omp end simd
+
+  !$omp do simd ordered
+  do i = 1, N
+    !ERROR: At most one SIMD clause can appear on the ORDERED directive
+    !$omp ordered simd simd
+    arrayA(i) = i
+    !$omp end ordered
+  end do
+  !$omp end do simd
+
+  !$omp do ordered(1)
+  do i = 1, N
+    arrayA(i) = foo(i)
+    !ERROR: At most one DEPEND(SOURCE) clause can appear on the ORDERED directive
+    !$omp ordered depend(source) depend(source)
+    arrayB(i) = bar(arrayA(i))
+  end do
+  !$omp end do
+
+end program main