[Flang][OpenMP 4.5] Add semantic check for OpenMP ordered and collapse clause

[yhegde]

Summary

Semantic check for OpenMP 4.5 - 2.7.1 ordered and collapse clause

File:

1. resolve-directives.cpp CHECK(level == 0); is commented ; because semantic checks are invoked after this function is called, so the check will fail before checking the semantic errors.

Test cases:

1. omp-do-ordered.f90

2.omp-do-collapse.f90

[kiranchandramohan]

Thanks for this patch. A few comments inline.

flang/lib/Semantics/resolve-directives.cpp

45

OmpClause should ideally be inside the OmpAttributeVistor. Is there any specific reason it should be here?

flang/test/Semantics/omp-do-collapse.f90

5

Can you add the following tests?

1. A positive test.
2. A positive test with more nested loops than the collapse value.
3. A negative test where there are loops enclosing the omp do loop with collapse.

[clementval]

flang/lib/Semantics/resolve-directives.cpp

45

OmpClause should not be here. If this is intended to be shared with OpenACC it should be templated.

76

Same here for OmpClause in generic part of DirectiveAttributeVisitor

79

Same here for OmpClause in generic part of DirectiveAttributeVisitor

[yhegde]

1. More positive and negative test cases are added.
2. Code moved to OmpAttributeVisitor.

[kiranchandramohan]

A few questions. Otherwise LGTM.

flang/lib/Semantics/resolve-directives.cpp

787

Can this function return a pair of std::int64_t parser::OmpClause *?

811

Nit: Can this function return a pair? Question because the name of the function is Get*.

860

You can remove this commented code.

861–868

Nit: This function's name (PrivatizeAssociatedLoopIndex) does not suggest that this kind of check happens here. Is it possible to pull this out or do it at a different place? Any strong reasons why it should be here?

[yhegde]

1. From PrivatizeAssociatedLoopIndex function, the loop level checking code for ordered and collapse clause  is moved to CheckAssocLoopLevel function.

"Is it possible to pull this out or do it at a different place?"

Since CHECK(level == 0) is removed, the semantic checks can be done in check-omp-structure.cpp. Once again we can go thro the DoConstruct to get the loop level and can have respective checks in OMPC_collapse and OMPC_ordered. Is that what you are suggesting ?!! @kiranchandramohan

2. Clause names are changed to uppercase.

3. "Nit: Can this function return a pair? Question because the name of the function is Get*."

Not done.
It can return a pair of level and OmpClause (probably to avoid Set and GetAssociatedClause,) but to invoke this function from elsewhere, we require OmpClauseList as an input parameter , if my understanding is correct. @kiranchandramohan

[kiranchandramohan]

OK. LGTM.

flang/lib/Semantics/resolve-directives.cpp

829–830

Nit: remove this todo.

831

Nit: rename to PrivatizeLoopIndexAndCheckLoopLevel or a better name.

[kiranchandramohan]

@yhegde can this be submitted? If you require inputs from me please let me know.

[yhegde]

Thanks for reviewing.
Function name PrivatizeAssociatedLoopIndex is changed to PrivatizeAssociatedLoopIndexAndCheckLoopLevel.

[yhegde]

This comment was removed by yhegde.

flang/lib/Semantics/resolve-directives.cpp

831

Nit: rename to PrivatizeLoopIndexAndCheckLoopLevel or a better name.

Thank you very much @kiranchandramohan and @clementval for all the review comments.
Shall I change the name of the function to CheckClauseAssocLoopIndexandLoopLevel ?

Thank you once again.

[kiranchandramohan]

flang/lib/Semantics/resolve-directives.cpp

831

This is fine. Please go ahead and submit.

[clementval]

Fine for me too

flang/lib/Semantics/resolve-directives.cpp

DirContext(const parser::CharBlock &source, T d, Scope &s)
         : directiveSource{source}, directive{d}, scope{s} {}
     parser::CharBlock directiveSource;
     T directive;
     Scope &scope;
     Symbol::Flag defaultDSA{Symbol::Flag::AccShared}; // TODOACC
     std::map<const Symbol *, Symbol::Flag> objectWithDSA;
     bool withinConstruct{false};
     std::int64_t associatedLoopLevel{0};
   };
 
   DirContext &GetContext() {
     CHECK(!dirContext_.empty());
     return dirContext_.back();
   }
   void PushContext(const parser::CharBlock &source, T dir) {
     dirContext_.emplace_back(source, dir, context_.FindScope(source));
   }
protected:
   }
   bool IsObjectWithDSA(const Symbol &symbol) {
     auto it{GetContext().objectWithDSA.find(&symbol)};
     return it != GetContext().objectWithDSA.end();
   }
   void SetContextAssociatedLoopLevel(std::int64_t level) {
     GetContext().associatedLoopLevel = level;
   }
   Symbol &MakeAssocSymbol(const SourceName &name, Symbol &prev, Scope &scope) {
     const auto pair{scope.try_emplace(name, Attrs{}, HostAssocDetails{prev})};
     return *pair.first->second;
   }
   Symbol &MakeAssocSymbol(const SourceName &name, Symbol &prev) {
     return MakeAssocSymbol(name, prev, currScope());
   }
   static const parser::Name *GetDesignatorNameIfDataRef(
       const parser::Designator &designator) {
     const auto *dataRef{std::get_if<parser::DataRef>(&designator.u)};
     return dataRef ? std::get_if<parser::Name>(&dataRef->u) : nullptr;
   }
public:
   }
   bool Pre(const parser::OmpClause::Lastprivate &x) {
     ResolveOmpObjectList(x.v, Symbol::Flag::OmpLastPrivate);
     return false;
   }
 
   void Post(const parser::Name &);
 
+  const parser::OmpClause *associatedClause{nullptr};
+  void SetAssociatedClause(const parser::OmpClause &c) {
+    associatedClause = &c;
+  }
+  const parser::OmpClause *GetAssociatedClause() { return associatedClause; }
+
 private:
   std::int64_t GetAssociatedLoopLevelFromClauses(const parser::OmpClauseList &);
 
   static constexpr Symbol::Flags dataSharingAttributeFlags{
       Symbol::Flag::OmpShared, Symbol::Flag::OmpPrivate,
       Symbol::Flag::OmpFirstPrivate, Symbol::Flag::OmpLastPrivate,
       Symbol::Flag::OmpReduction, Symbol::Flag::OmpLinear};
 
if (const auto &iv{GetLoopIndex(x)}; iv.symbol) {
       } else {
         // TODO: conflict checks with explicitly determined DSA
       }
     }
   }
   return true;
 }
 
 std::int64_t OmpAttributeVisitor::GetAssociatedLoopLevelFromClauses(
     const parser::OmpClauseList &x) {
   std::int64_t orderedLevel{0};
   std::int64_t collapseLevel{0};
+  const parser::OmpClause *ordClause{nullptr};
+  const parser::OmpClause *collClause{nullptr};
   for (const auto &clause : x.v) {
     if (const auto *orderedClause{
             std::get_if<parser::OmpClause::Ordered>(&clause.u)}) {
       if (const auto v{EvaluateInt64(context_, orderedClause->v)}) {
         orderedLevel = *v;
       }
+      ordClause = &clause;
     }
     if (const auto *collapseClause{
             std::get_if<parser::OmpClause::Collapse>(&clause.u)}) {
       if (const auto v{EvaluateInt64(context_, collapseClause->v)}) {
         collapseLevel = *v;
       }
+      collClause = &clause;
     }
   }
 
   if (orderedLevel && (!collapseLevel || orderedLevel >= collapseLevel)) {
+    SetAssociatedClause(*ordClause);
     return orderedLevel;
   } else if (!orderedLevel && collapseLevel) {
+    SetAssociatedClause(*collClause);
     return collapseLevel;
   } // orderedLevel < collapseLevel is an error handled in structural checks
   return 1; // default is outermost loop
 }
 
 // 2.15.1.1 Data-sharing Attribute Rules - Predetermined
 //   - The loop iteration variable(s) in the associated do-loop(s) of a do,
 //     parallel do, taskloop, or distribute construct is (are) private.
 //   - The loop iteration variable in the associated do-loop of a simd construct
 //     with just one associated do-loop is linear with a linear-step that is the
 //     increment of the associated do-loop.
 //   - The loop iteration variables in the associated do-loops of a simd
 //     construct with multiple associated do-loops are lastprivate.
 //
 // TODO: revisit after semantics checks are completed for do-loop association of
 //       collapse and ordered
 void OmpAttributeVisitor::PrivatizeAssociatedLoopIndex(
     const parser::OpenMPLoopConstruct &x) {
   std::int64_t level{GetContext().associatedLoopLevel};
   if (level <= 0) {
     return;
   }
   Symbol::Flag ivDSA;
   if (!llvm::omp::simdSet.test(GetContext().directive)) {
     ivDSA = Symbol::Flag::OmpPrivate;
if (auto *symbol{ResolveOmp(iv, ivDSA, currScope())}) {
       iv.symbol = symbol; // adjust the symbol within region
       AddToContextObjectWithDSA(*symbol, ivDSA);
     }
 
     const auto &block{std::get<parser::Block>(loop->t)};
     const auto it{block.begin()};
     loop = it != block.end() ? GetDoConstructIf(*it) : nullptr;
   }
-  CHECK(level == 0);
+  // CHECK(level == 0);
+  if (const auto *clause{GetAssociatedClause()}) {
+    if (level != 0) {
+      context_.Say(clause->source,
+          "The value of the parameter in the collapse or ordered clause must"
+          " not be larger than the number of nested loops"
+          " following the construct."_err_en_US);
+    }
+  }
 }
 
 bool OmpAttributeVisitor::Pre(const parser::OpenMPSectionsConstruct &x) {
   const auto &beginSectionsDir{
       std::get<parser::OmpBeginSectionsDirective>(x.t)};
   const auto &beginDir{
       std::get<parser::OmpSectionsDirective>(beginSectionsDir.t)};
   switch (beginDir.v) {

flang/test/Semantics/omp-do-collapse-postivecases.f90

+!RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.7.1 Collapse Clause Positive cases
+
+!DEF: /omp_docollapse MainProgram
+program omp_docollapse
+ !DEF: /omp_docollapse/i ObjectEntity INTEGER(4)
+ !DEF: /omp_docollapse/j ObjectEntity INTEGER(4)
+ !DEF: /omp_docollapse/k ObjectEntity INTEGER(4)
+ integer i, j, k
+!$omp do  collapse(2)
+ !DEF: /omp_docollapse/Block1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ do i=1,10
+  !DEF: /omp_docollapse/Block1/j (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+  do j=1,10
+   !REF: /omp_docollapse/k
+   do k=1,10
+    print *, "hello"
+   end do
+  end do
+ end do
+!$omp end do
+
+ !REF: /omp_docollapse/i
+ do i=1,10
+!$omp do  collapse(2)
+  !DEF: /omp_docollapse/Block1/j (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+  do j=1,10
+   !DEF: /omp_docollapse/Block1/k (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+   do k=1,10
+    print *, "hello"
+   end do
+  end do
+!$omp end do
+ end do
+end program omp_docollapse

flang/test/Semantics/omp-do-collapse.f90

+!RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.7.1 Collapse Clause
+program omp_doCollapse
+  integer:: i,j
+  !ERROR: The value of the parameter in the collapse or ordered clause must not be larger than the number of nested loops following the construct.
+  !$omp do collapse(3)
+  do i = 1,10
+    do j = 1, 10
+      print *, "hello"
+    end do
+  end do
+  !$omp end do
+
+  do i = 1,10
+    do j = 1, 10
+      !ERROR: The value of the parameter in the collapse or ordered clause must not be larger than the number of nested loops following the construct.
+      !$omp do collapse(2)
+      do k = 1, 10
+        print *, "hello"
+      end do
+      !$omp end do
+    end do
+  end do
+end program omp_doCollapse
+

flang/test/Semantics/omp-do-ordered-positivecases.f90

+!RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.7.1 Ordered Clause positive cases.
+
+!DEF: /omp_doordered MainProgram
+program omp_doordered
+ !DEF: /omp_doordered/i ObjectEntity INTEGER(4)
+ !DEF: /omp_doordered/j ObjectEntity INTEGER(4)
+ integer i, j
+!$omp do  ordered(2)
+ !DEF: /omp_doordered/Block1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ do i=1,10
+  !DEF: /omp_doordered/Block1/j (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+  do j=1,10
+   print *, "hello"
+  end do
+ end do
+!$omp end do
+ !REF: /omp_doordered/i
+ do i=1,10
+  !REF: /omp_doordered/j
+  do j=1,10
+!$omp do  ordered(1)
+   !DEF: /omp_doordered/Block2/k (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+   do k=1,10
+    print *, "hello"
+   end do
+!$omp end do
+  end do
+ end do
+!$omp do  ordered(1)
+ !DEF: /omp_doordered/Block3/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ do i=1,10
+!$omp ordered
+  !REF: /omp_doordered/j
+  do j=1,10
+   print *, "hello"
+  end do
+!$omp end ordered
+ end do
+!$omp end do
+!$omp do  collapse(1) ordered(2)
+ !DEF: /omp_doordered/Block4/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ do i=1,10
+  !DEF: /omp_doordered/Block4/j (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+  do j=1,10
+   print *, "hello"
+  end do
+ end do
+!$omp end do
+!$omp parallel  num_threads(4)
+!$omp do  ordered(1) collapse(1)
+ !DEF: /omp_doordered/Block5/Block1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ do i=1,10
+!$omp ordered
+  !DEF: /omp_doordered/Block5/j (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+  do j=1,10
+   print *, "hello"
+  end do
+!$omp end ordered
+ end do
+!$omp end parallel
+end program omp_doordered

flang/test/Semantics/omp-do-ordered.f90

+!RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.7.1 Ordered Clause
+
+program omp_doOrdered
+  integer:: i,j
+  !ERROR: The value of the parameter in the collapse or ordered clause must not be larger than the number of nested loops following the construct.
+  !$omp do ordered(3)
+  do i = 1,10
+    do j = 1, 10
+      print *, "hello"
+    end do
+  end do
+  !$omp end do
+
+  do i = 1,10
+    do j = 1, 10
+      !ERROR: The value of the parameter in the collapse or ordered clause must not be larger than the number of nested loops following the construct.
+      !$omp do ordered(2)
+      do k = 1, 10
+        print *, "hello"
+      end do
+      !$omp end do
+    end do
+  end do
+
+  !ERROR: The value of the parameter in the collapse or ordered clause must not be larger than the number of nested loops following the construct.
+  !$omp do ordered(2)
+  do i = 1,10
+    !$omp ordered
+    do j = 1, 10
+       print *, "hello"
+    end do
+    !$omp end ordered
+  end do
+  !$omp end do
+
+  !ERROR: The value of the parameter in the collapse or ordered clause must not be larger than the number of nested loops following the construct.
+  !$omp do collapse(1) ordered(3)
+  do i = 1,10
+    do j = 1, 10
+       print *, "hello"
+    end do
+  end do
+  !$omp end do
+
+  !$omp parallel num_threads(4)
+  !ERROR: The value of the parameter in the collapse or ordered clause must not be larger than the number of nested loops following the construct.
+  !$omp do ordered(2) collapse(1)
+  do i = 1,10
+    !$omp ordered
+    do j = 1, 10
+       print *, "hello"
+    end do
+    !$omp end ordered
+  end do
+  !$omp end parallel
+end program omp_doOrdered