[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

4

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.

77

Same here for OmpClause in generic part of DirectiveAttributeVisitor

80

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

788

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

810

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

857–865

You can remove this commented code.

858–865

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

826–827

Nit: remove this todo.

828

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

828

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

828

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};
+    const parser::OmpClause *clause{nullptr};
   };
 
   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;
   }
+  void SetContextAssociatedClause(const parser::OmpClause &c) {
+    GetContext().clause = &c;
+  }
+  const parser::OmpClause *GetContextAssociatedClause() {
+    return GetContext().clause;
+  }
   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(
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};
   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;
       }
+      SetContextAssociatedClause(clause);
     }
     if (const auto *collapseClause{
             std::get_if<parser::OmpClause::Collapse>(&clause.u)}) {
       if (const auto v{EvaluateInt64(context_, collapseClause->v)}) {
         collapseLevel = *v;
       }
+      SetContextAssociatedClause(clause);
     }
   }
 
   if (orderedLevel && (!collapseLevel || orderedLevel >= collapseLevel)) {
     return orderedLevel;
   } else if (!orderedLevel && collapseLevel) {
     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 (auto *clause{GetContextAssociatedClause()}) {
+    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.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
+end program omp_doCollapse
+

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
+end program omp_doOrdered