[flang][OpenMP] Check for occurrence of multiple list items in nontemporal clause for simd directive

[arnamoy10]

Summary

This patch implements the following semantic check:

A list-item cannot appear in more than one nontemporal clause.

[kiranchandramohan]

Thanks @arnamoy10 for this patch. A few comments.

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

176

Move this close to its use. Use braced initialization in all places (here, the line above, for loop variable etc).

184–192

Can you make this a lambda function that takes in a list of variables, with listvars declared inside the lambda and an argument (pass ALIGNED or NONTEMPORAL to it)?

189

Should this break be removed to capture all errors?
!$omp simd nontemporal(a,a,b,b)

[arnamoy10]

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

184–192

Thanks for the comment. I was creating a lambda as per your suggestion and passing alignedClauses/ nonTemporalClauses as the argument and another argument to denote whether we are processing ALIGNED or NONTEMPORAL.

However, in that lambda, how do I declare a generic variable, which will be defined as the following pseudocode based on the passed argument? It cannot be auto e.g. the following will not work.

auto &Clause;
if (arg == ALIGNED) 
  Clause = std::get<parser::OmpClause::Aligned>(itr->second->u)};
else
  Clause = std::get<parser::OmpClause::Aligned>(itr->second->u)};

What will be the type of Clause in this case?

[kiranchandramohan]

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

184–192

I think the lambda at that level might not work since templates are not properly supported for lambdas in C++17. I was thinking something like the following (templated functions) will work. It can be called as checkMultiple<parser::OmpClause::Aligned>(*itr, "ALIGNED"). But I see that there is some difference in the way the namelist is constructed. So there are additional checks or conditions required.

const auto &alignedNameList{std::get<std::list<parser::Name>>(alignedClause.v.t)};
const auto &nontempNameList{nontempClause.v};

  template<typename C, typename V>
void checkMultiple(const V &v, const std::string &clauseName) {

    semantics::UnorderedSymbolSet listVars;
  auto checkMultipleOcurrence = [&](const std::list<parser::Name>& nameList, const parser::CharBlock& item) {
    for (auto const &var : nameList) {
      if (listVars.count(*(var.symbol)) == 1) {
        context_.Say(item,
            "List item '%s' present at multiple %s clauses"_err_en_US,
            var.ToString(), clauseName);
        break;
      }
      listVars.insert(*(var.symbol));
    }
  };
    const auto &alignedClause{
        std::get<C>(v.second->u)};
    const auto &alignedNameList{
        std::get<std::list<parser::Name>>(alignedClause.v.t)};
    checkMultipleOcurrence(alignedNameList, v.second->source);

}

Instead, you can try the lambda checkMultipleOcurrence that is there in the function above. I think this is what I suggested.

[arnamoy10]

Addressing reviewers comments:

1. Use of lambda
2. Enhancing the check by not breaking from the loop
3. Adding a test case to catch the above case

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

184–192

Thank you. However, we could not fit the alignedClause and `alignedNameList' inside the helper because at compile time, the namelist construction could not be controlled. Still used a lambda, please check the updated patch.

[NimishMishra]

Added reviewers: @shraiysh, @kiranktp, @AMDChirag

[shraiysh]

Patch LGTM (once @kiranchandramohan's comments have been addressed ). Please run git-clang-format HEAD~ to format.
Also, please rebase with main, it has been long.

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

177

llvm::is_contained?

[arnamoy10]

@shraiysh Thanks for the comments. I have already addressed @kiranchandramohan 's comments as much as I could. If you are OK, can you please accept this patch, because Kiran might be busy. Thank you

[kiranchandramohan]

LGTM. Apologies for the long delay.

[shraiysh]

Looks good to me. Thank you!

[arnamoy10]

Rebase before land

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

#include "llvm/Frontend/OpenMP/OMP.inc"
   // Get the OpenMP Clause Kind for the corresponding Parser class
   template <typename A>
   llvm::omp::Clause GetClauseKindForParserClass(const A &) {
 #define GEN_FLANG_CLAUSE_PARSER_KIND_MAP
 #include "llvm/Frontend/OpenMP/OMP.inc"
   }
 
 private:
+  void CheckMultListItems();
   bool HasInvalidWorksharingNesting(
       const parser::CharBlock &, const OmpDirectiveSet &);
   bool IsCloselyNestedRegion(const OmpDirectiveSet &set);
   void HasInvalidTeamsNesting(
       const llvm::omp::Directive &dir, const parser::CharBlock &source);
   void HasInvalidDistributeNesting(const parser::OpenMPLoopConstruct &x);
   // specific clause related
   bool ScheduleModifierHasType(const parser::OmpScheduleClause &,

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

while (index != -1) {
         return false;
       }
       index--;
     }
   }
   return false;
 }
 
+void OmpStructureChecker::CheckMultListItems() {
+  semantics::UnorderedSymbolSet listVars;
+  auto checkMultipleOcurrence = [&](const std::list<parser::Name> &nameList,
+                                    const parser::CharBlock &item,
+                                    const std::string &clauseName) {
+    for (auto const &var : nameList) {
+      if (llvm::is_contained(listVars, *(var.symbol))) {
+        context_.Say(item,
+            "List item '%s' present at multiple %s clauses"_err_en_US,
+            var.ToString(), clauseName);
+      }
+      listVars.insert(*(var.symbol));
+    }
+  };
+
+  // Aligned clause
+  auto alignedClauses{FindClauses(llvm::omp::Clause::OMPC_aligned)};
+  for (auto itr = alignedClauses.first; itr != alignedClauses.second; ++itr) {
+    const auto &alignedClause{
+        std::get<parser::OmpClause::Aligned>(itr->second->u)};
+    const auto &alignedNameList{
+        std::get<std::list<parser::Name>>(alignedClause.v.t)};
+    checkMultipleOcurrence(alignedNameList, itr->second->source, "ALIGNED");
+  }
+
+  // Nontemporal clause
+  auto nonTemporalClauses{FindClauses(llvm::omp::Clause::OMPC_nontemporal)};
+  for (auto itr = nonTemporalClauses.first; itr != nonTemporalClauses.second;
+       ++itr) {
+    const auto &nontempClause{
+        std::get<parser::OmpClause::Nontemporal>(itr->second->u)};
+    const auto &nontempNameList{nontempClause.v};
+    checkMultipleOcurrence(nontempNameList, itr->second->source, "NONTEMPORAL");
+  }
+}
+
 bool OmpStructureChecker::HasInvalidWorksharingNesting(
     const parser::CharBlock &source, const OmpDirectiveSet &set) {
   // set contains all the invalid closely nested directives
   // for the given directive (`source` here)
   if (IsCloselyNestedRegion(set)) {
     context_.Say(source,
         "A worksharing region may not be closely nested inside a "
         "worksharing, explicit task, taskloop, critical, ordered, atomic, or "
if (auto *clause{FindClause(llvm::omp::Clause::OMPC_simdlen)}) {
               context_.Say(clause->source,
                   "The parameter of the SIMDLEN clause must be less than or "
                   "equal to the parameter of the SAFELEN clause"_err_en_US);
             }
           }
         }
       }
     }
-    // A list-item cannot appear in more than one aligned clause
-    semantics::UnorderedSymbolSet alignedVars;
-    auto clauseAll = FindClauses(llvm::omp::Clause::OMPC_aligned);
-    for (auto itr = clauseAll.first; itr != clauseAll.second; ++itr) {
-      const auto &alignedClause{
-          std::get<parser::OmpClause::Aligned>(itr->second->u)};
-      const auto &alignedNameList{
-          std::get<std::list<parser::Name>>(alignedClause.v.t)};
-      for (auto const &var : alignedNameList) {
-        if (alignedVars.count(*(var.symbol)) == 1) {
-          context_.Say(itr->second->source,
-              "List item '%s' present at multiple ALIGNED clauses"_err_en_US,
-              var.ToString());
-          break;
-        }
-        alignedVars.insert(*(var.symbol));
-      }
-    }
+    // Sema checks related to presence of multiple list items within the same
+    // clause
+    CheckMultListItems();
   } // 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});
   }
 

flang/test/Semantics/omp-simd-nontemporal.f90

+! RUN: %python %S/test_errors.py %s %flang -fopenmp
+
+! OpenMP Version 4.5
+! 2.8.1 simd Construct
+! Semantic error for correct test case
+
+program omp_simd
+  integer i, j, k
+  integer, allocatable :: a(:), b(:)
+
+  allocate(a(10))
+  allocate(b(10))
+
+  !ERROR: List item 'a' present at multiple NONTEMPORAL clauses
+  !$omp simd nontemporal(a, a)
+  do i = 1, 10
+    a(i) = i
+  end do
+  !$omp end simd
+
+  !ERROR: List item 'a' present at multiple NONTEMPORAL clauses
+  !ERROR: List item 'b' present at multiple NONTEMPORAL clauses
+  !$omp simd nontemporal(a,a) nontemporal(b) nontemporal(b)
+  do i = 1, 10
+    a(i) = i
+    b(i) = i
+  end do
+  !$omp end simd
+
+  !ERROR: List item 'a' present at multiple NONTEMPORAL clauses
+  !$omp simd nontemporal(a) nontemporal(a)
+  do i = 1, 10
+    a(i) = i
+  end do
+  !$omp end simd
+
+  !$omp simd nontemporal(a) nontemporal(b)
+  do i = 1, 10
+    a(i) = i
+    b(i) = i
+  end do
+  !$omp end simd
+
+  !ERROR: List item 'a' present at multiple NONTEMPORAL clauses
+  !$omp simd nontemporal(a) private(a) nontemporal(a)
+  do i = 1, 10
+    a(i) = i
+    b(i) = i
+  end do
+  !$omp end simd
+
+  !ERROR: List item 'a' present at multiple NONTEMPORAL clauses
+  !ERROR: List item 'b' present at multiple NONTEMPORAL clauses
+  !$omp simd nontemporal(a,a,b,b)
+  do i = 1, 10
+    a(i) = i
+    b(i) = i
+  end do
+  !$omp end simd
+
+  print *, a
+
+end program omp_simd