[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

764

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

#include "llvm/Frontend/OpenMP/OMP.inc"
 
   void Enter(const parser::OpenMPConstruct &);
   void Enter(const parser::OpenMPLoopConstruct &);
   void Leave(const parser::OpenMPLoopConstruct &);
   void Enter(const parser::OmpEndLoopDirective &);
 
   void Enter(const parser::OpenMPBlockConstruct &);
   void Leave(const parser::OpenMPBlockConstruct &);
+  void Leave(const parser::OmpBeginBlockDirective &);
   void Enter(const parser::OmpEndBlockDirective &);
   void Leave(const parser::OmpEndBlockDirective &);
 
   void Enter(const parser::OpenMPSectionsConstruct &);
   void Leave(const parser::OpenMPSectionsConstruct &);
   void Enter(const parser::OmpEndSectionsDirective &);
   void Leave(const parser::OmpEndSectionsDirective &);
 
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);
   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 ChecksOnOrderedAsBlock();
   void CheckBarrierNesting(const parser::OpenMPSimpleStandaloneConstruct &x);
+  void ChecksOnOrderedAsStandalone();
   void CheckReductionArraySection(const parser::OmpObjectList &ompObjectList);
   void CheckIntentInPointerAndDefinable(
       const parser::OmpObjectList &, const llvm::omp::Clause);
   void CheckArraySection(const parser::ArrayElement &arrayElement,
       const parser::Name &name, const llvm::omp::Clause clause);
   void CheckMultipleAppearanceAcrossContext(
       const parser::OmpObjectList &ompObjectList);
   const parser::OmpObjectList *GetOmpObjectList(const parser::OmpClause &);

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::Leave(const parser::OpenMPBlockConstruct &) {
   if (GetDirectiveNest(TargetBlockOnlyTeams)) {
     ExitDirectiveNest(TargetBlockOnlyTeams);
   }
   if (GetContext().directive == llvm::omp::Directive::OMPD_target) {
     ExitDirectiveNest(TargetNest);
   }
   dirContext_.pop_back();
 }
 
+void OmpStructureChecker::ChecksOnOrderedAsBlock() {
+  if (FindClause(llvm::omp::Clause::OMPC_depend)) {
+    context_.Say(GetContext().clauseSource,
+        "DEPEND(*) clauses are not allowed when ORDERED construct is a block"
+        " construct with an ORDERED region"_err_en_US);
+  }
+}
+
+void OmpStructureChecker::Leave(const parser::OmpBeginBlockDirective &) {
+  switch (GetContext().directive) {
+  case llvm::omp::Directive::OMPD_ordered:
+    // [5.1] 2.19.9 Ordered Construct Restriction
+    ChecksOnOrderedAsBlock();
+    break;
+  default:
+    break;
+  }
+}
+
 void OmpStructureChecker::Enter(const parser::OpenMPSectionsConstruct &x) {
   const auto &beginSectionsDir{
       std::get<parser::OmpBeginSectionsDirective>(x.t)};
   const auto &endSectionsDir{std::get<parser::OmpEndSectionsDirective>(x.t)};
   const auto &beginDir{
       std::get<parser::OmpSectionsDirective>(beginSectionsDir.t)};
   const auto &endDir{std::get<parser::OmpSectionsDirective>(endSectionsDir.t)};
   CheckMatching<parser::OmpSectionsDirective>(beginDir, endDir);
if (IsCloselyNestedRegion(llvm::omp::nestedBarrierErrSet)) {
       context_.Say(parser::FindSourceLocation(x),
           "`BARRIER` region may not be closely nested inside of `WORKSHARING`, "
           "`LOOP`, `TASK`, `TASKLOOP`,"
           "`CRITICAL`, `ORDERED`, `ATOMIC` or `MASTER` region."_err_en_US);
     }
   }
 }
 
+void OmpStructureChecker::ChecksOnOrderedAsStandalone() {
+  if (FindClause(llvm::omp::Clause::OMPC_threads) ||
+      FindClause(llvm::omp::Clause::OMPC_simd)) {
+    context_.Say(GetContext().clauseSource,
+        "THREADS, SIMD clauses are not allowed when ORDERED construct is a "
+        "standalone construct with no ORDERED region"_err_en_US);
+  }
+
+  bool isSinkPresent{false};
+  int dependSourceCount{0};
+  auto clauseAll = FindClauses(llvm::omp::Clause::OMPC_depend);
+  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)) {
+      dependSourceCount++;
+      if (isSinkPresent) {
+        context_.Say(itr->second->source,
+            "DEPEND(SOURCE) is not allowed when DEPEND(SINK: vec) is present "
+            "on ORDERED directive"_err_en_US);
+      }
+      if (dependSourceCount > 1) {
+        context_.Say(itr->second->source,
+            "At most one DEPEND(SOURCE) clause can appear on the ORDERED "
+            "directive"_err_en_US);
+      }
+    } else if (std::get_if<parser::OmpDependClause::Sink>(&dependClause.v.u)) {
+      isSinkPresent = true;
+      if (dependSourceCount > 0) {
+        context_.Say(itr->second->source,
+            "DEPEND(SINK: vec) is not allowed when DEPEND(SOURCE) is present "
+            "on ORDERED directive"_err_en_US);
+      }
+    } else {
+      context_.Say(itr->second->source,
+          "Only DEPEND(SOURCE) or DEPEND(SINK: vec) are allowed when ORDERED "
+          "construct is a standalone construct with no ORDERED "
+          "region"_err_en_US);
+    }
+  }
+}
+
 void OmpStructureChecker::Enter(
     const parser::OpenMPSimpleStandaloneConstruct &x) {
   const auto &dir{std::get<parser::OmpSimpleStandaloneDirective>(x.t)};
   PushContextAndClauseSets(dir.source, dir.v);
   CheckBarrierNesting(x);
 }
 
 void OmpStructureChecker::Leave(
     const parser::OpenMPSimpleStandaloneConstruct &) {
+  switch (GetContext().directive) {
+  case llvm::omp::Directive::OMPD_ordered:
+    // [5.1] 2.19.9 Ordered Construct Restriction
+    ChecksOnOrderedAsStandalone();
+    break;
+  default:
+    break;
+  }
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPFlushConstruct &x) {
   const auto &dir{std::get<parser::Verbatim>(x.t)};
   PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_flush);
 }
 

flang/lib/Semantics/resolve-directives.cpp

public:
 
   bool Pre(const parser::OpenMPBlockConstruct &);
   void Post(const parser::OpenMPBlockConstruct &);
 
   void Post(const parser::OmpBeginBlockDirective &) {
     GetContext().withinConstruct = true;
   }
 
+  bool Pre(const parser::OpenMPSimpleStandaloneConstruct &);
+  void Post(const parser::OpenMPSimpleStandaloneConstruct &) { PopContext(); }
+
   bool Pre(const parser::OpenMPLoopConstruct &);
   void Post(const parser::OpenMPLoopConstruct &) { PopContext(); }
   void Post(const parser::OmpBeginLoopDirective &) {
     GetContext().withinConstruct = true;
   }
   bool Pre(const parser::DoConstruct &);
 
   bool Pre(const parser::OpenMPSectionsConstruct &);
public:
   }
 
   bool Pre(const parser::OmpClause::Nontemporal &x) {
     const auto &nontemporalNameList{x.v};
     ResolveOmpNameList(nontemporalNameList, Symbol::Flag::OmpNontemporal);
     return false;
   }
 
+  bool Pre(const parser::OmpDependClause &x) {
+    if (const auto *dependSink{
+            std::get_if<parser::OmpDependClause::Sink>(&x.u)}) {
+      const auto &dependSinkVec{dependSink->v};
+      for (const auto &dependSinkElement : dependSinkVec) {
+        const auto &name{std::get<parser::Name>(dependSinkElement.t)};
+        ResolveName(&name);
+      }
+    }
+    return false;
+  }
+
   void Post(const parser::Name &);
 
   // Keep track of labels in the statements that causes jumps to target labels
   void Post(const parser::GotoStmt &gotoStmt) { CheckSourceLabel(gotoStmt.v); }
   void Post(const parser::ComputedGotoStmt &computedGotoStmt) {
     for (auto &label : std::get<std::list<parser::Label>>(computedGotoStmt.t)) {
       CheckSourceLabel(label);
     }
case llvm::omp::Directive::OMPD_target_parallel: {
     break;
   }
   default:
     break;
   }
   PopContext();
 }
 
+bool OmpAttributeVisitor::Pre(
+    const parser::OpenMPSimpleStandaloneConstruct &x) {
+  const auto &standaloneDir{
+      std::get<parser::OmpSimpleStandaloneDirective>(x.t)};
+  switch (standaloneDir.v) {
+  case llvm::omp::Directive::OMPD_barrier:
+  case llvm::omp::Directive::OMPD_ordered:
+  case llvm::omp::Directive::OMPD_target_enter_data:
+  case llvm::omp::Directive::OMPD_target_exit_data:
+  case llvm::omp::Directive::OMPD_target_update:
+  case llvm::omp::Directive::OMPD_taskwait:
+  case llvm::omp::Directive::OMPD_taskyield:
+    PushContext(standaloneDir.source, standaloneDir.v);
+    break;
+  default:
+    break;
+  }
+  ClearDataSharingAttributeObjects();
+  return true;
+}
+
 bool OmpAttributeVisitor::Pre(const parser::OpenMPLoopConstruct &x) {
   const auto &beginLoopDir{std::get<parser::OmpBeginLoopDirective>(x.t)};
   const auto &beginDir{std::get<parser::OmpLoopDirective>(beginLoopDir.t)};
   const auto &clauseList{std::get<parser::OmpClauseList>(beginLoopDir.t)};
   switch (beginDir.v) {
   case llvm::omp::Directive::OMPD_distribute:
   case llvm::omp::Directive::OMPD_distribute_parallel_do:
   case llvm::omp::Directive::OMPD_distribute_parallel_do_simd:

flang/test/Semantics/omp-clause-validity01.f90

! Standalone Directives (basic)
   !$omp taskyield
   !$omp barrier
   !$omp taskwait
   !$omp taskwait depend(source)
   ! !$omp taskwait depend(sink:i-1)
   ! !$omp target enter data map(to:arrayA) map(alloc:arrayB)
   ! !$omp target update from(arrayA) to(arrayB)
   ! !$omp target exit data map(from:arrayA) map(delete:arrayB)
-  !$omp ordered depend(source)
-  ! !$omp ordered depend(sink:i-1)
   !$omp flush (c)
   !$omp flush acq_rel
   !$omp flush release
   !$omp flush acquire
   !ERROR: If memory-order-clause is RELEASE, ACQUIRE, or ACQ_REL, list items must not be specified on the FLUSH directive
   !$omp flush release (c)
   !ERROR: SEQ_CST clause is not allowed on the FLUSH directive
   !$omp flush seq_cst

flang/test/Semantics/omp-ordered01.f90

+! RUN: %python %S/test_errors.py %s %flang -fopenmp
+! 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), arrayC(10)
+  real, external :: foo, bar, baz
+
+  !$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 = 2, N
+    !ERROR: Only DEPEND(SOURCE) or DEPEND(SINK: vec) are allowed when ORDERED construct is a standalone construct with no ORDERED region
+    !ERROR: At most one DEPEND(SOURCE) clause can appear on the ORDERED directive
+    !$omp ordered depend(source) depend(inout: arrayA) depend(source)
+    arrayA(i) = foo(i)
+    !ERROR: DEPEND(SOURCE) is not allowed when DEPEND(SINK: vec) is present on ORDERED directive
+    !ERROR: DEPEND(SOURCE) is not allowed when DEPEND(SINK: vec) is present on ORDERED directive
+    !ERROR: At most one DEPEND(SOURCE) clause can appear on the ORDERED directive
+    !$omp ordered depend(sink: i - 1) depend(source) depend(source)
+    arrayB(i) = bar(arrayA(i), arrayB(i-1))
+    !ERROR: Only DEPEND(SOURCE) or DEPEND(SINK: vec) are allowed when ORDERED construct is a standalone construct with no ORDERED region
+    !ERROR: Only DEPEND(SOURCE) or DEPEND(SINK: vec) are allowed when ORDERED construct is a standalone construct with no ORDERED region
+    !$omp ordered depend(out: arrayC) depend(in: arrayB)
+    arrayC(i) = baz(arrayB(i-1))
+  end do
+  !$omp end do
+
+  !$omp do ordered(1)
+  do i = 2, N
+    !ERROR: DEPEND(*) clauses are not allowed when ORDERED construct is a block construct with an ORDERED region
+    !$omp ordered depend(source)
+    arrayA(i) = foo(i)
+    !$omp end ordered
+    !ERROR: DEPEND(*) clauses are not allowed when ORDERED construct is a block construct with an ORDERED region
+    !$omp ordered depend(sink: i - 1)
+    arrayB(i) = bar(arrayA(i), arrayB(i-1))
+    !$omp end ordered
+  end do
+  !$omp end do
+
+contains
+  subroutine work1()
+    !ERROR: THREADS, SIMD clauses are not allowed when ORDERED construct is a standalone construct with no ORDERED region
+    !$omp ordered simd
+  end subroutine work1
+
+  subroutine work2()
+    !ERROR: THREADS, SIMD clauses are not allowed when ORDERED construct is a standalone construct with no ORDERED region
+    !$omp ordered threads
+  end subroutine work2
+
+end program main

llvm/include/llvm/Frontend/OpenMP/OMP.td

let allowedOnceClauses = [
     VersionedClause<OMPC_Release, 50>,
     // TODO This should ne `none` instead. Comment carried over from
     // OMPKinds.def.
     VersionedClause<OMPC_Flush>
   ];
 }
 def OMP_Ordered : Directive<"ordered"> {
   let allowedClauses = [
-    VersionedClause<OMPC_Threads>,
-    VersionedClause<OMPC_Simd>,
     VersionedClause<OMPC_Depend>
   ];
+  let allowedOnceClauses = [
+    VersionedClause<OMPC_Threads>,
+    VersionedClause<OMPC_Simd>
+  ];
 }
 def OMP_Atomic : Directive<"atomic"> {
   let allowedClauses = [
     VersionedClause<OMPC_Read>,
     VersionedClause<OMPC_Write>,
     VersionedClause<OMPC_Update>,
     VersionedClause<OMPC_Capture>,
   ];