[flang][OpenMP] Add semantic check for occurrence of multiple list items in aligned clause for simd directive

[arnamoy10]

Summary

Add semantic check for occurrence of multiple list items in aligned clause for simd directive. Also adds a test case

[kiranchandramohan]

Thanks @arnamoy10 for this patch.

Did you try the other approach that I mentioned using FindClause? I am guessing FindClause will only return a single clause and hence cannot be used directly. Would adding another function (if a similar one does not exist) which returns multiple clauses (using equal_range) work? This way we do not need to use the clauseList.

[arnamoy10]

In D97964#2604649, @kiranchandramohan wrote:

Thanks @arnamoy10 for this patch.

Did you try the other approach that I mentioned using FindClause? I am guessing FindClause will only return a single clause and hence cannot be used directly. Would adding another function (if a similar one does not exist) which returns multiple clauses (using equal_range) work? This way we do not need to use the clauseList.

Exactly. FindClause did not work for me for the reason mentioned above. I had this idea of using equal_range as well (and a new function). Thanks for confirming. Let me work on that and submit an updated patch.

[clementval]

This comment was removed by clementval.

[arnamoy10]

Updated code through the use of newly introduced FindClauseMult() function, which returns an iterator from the multimap, in case where multiple occurrence of a clause happens within a directive.

[clementval]

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

209

Is the name following a convention? What about FindClauses instead?

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

620

Clang-format to be run on the patch

[kiranchandramohan]

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

610

I think alignedVars{} is preferred.

619

Would it be better use the symbol corresponding to the name? And use a SymbolSet to store the symbols associated with the variables?

@tskeith In general what is preferable, comparing names of variables or symbols associated with them?

630

Can this go into the condition portion of the for loop from which it is breaking?

[tskeith]

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

610

The default initialization for a set is empty, so just std::set<std::string> alignedVars; is best.

619

You can have different symbols with the same name, so these aren't equivalent. In this case, I agree that SymbolSet seems appropriate.

Even when you want a set of names, it's better to save them as parser::CharBlock rather than making new std::string objects.

625

Why not just return here to simplify the logic?

[kiranchandramohan]

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

619

Thanks @tskeith.

@arnamoy10 if you switch to using symbols, then add a test which has a private clause in between the align clauses.

align(x) private(x) align(x)

[arnamoy10]

Addressing reviewers' comments:

1. Use of SymbolSet instead of searching by variable name.
2. Change function name to FindClauses()
3. One more test scenario to check aligned(a) private(a) aligned(a)
4. Simplified loop logic in case of Sema error

[kiranchandramohan]

LGTM. Please address all lint/clang-format errors. Please wait for @tskeith or @clementval to approve.

Thanks for your work here. Hope you can take this forward in the flow.

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

209

Nit: Naming the type here is probably better. Is there an issue?

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

12

Nit: Please fix the ordering warning above.

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

8

Nit: can you add a test with one more variable?

[clementval]

LGTM. Please address small comments.

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

209

+1 to @kiranchandramohan comment.

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

619

Please fix the clang-format warning.

628

Remove blank line.

[arnamoy10]

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

209

Do you mean replace auto?

[clementval]

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

209

Yes.

[arnamoy10]

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

8

@kiranchandramohan We already have a test with two variables at Line 28, do we need one more?

[arnamoy10]

1. Using type info in the FindClauses() function instead of auto
2. (hopefully) clang-formatted

[kiranchandramohan]

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

8

I meant an error with two variables repeating.

[arnamoy10]

Add test for multiple aligned clauses with multiple variables repeated.

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

 class DirectiveStructureChecker : public virtual BaseChecker {
 protected:
   DirectiveStructureChecker(SemanticsContext &context,
       std::unordered_map<D, DirectiveClauses<C, ClauseEnumSize>>
           directiveClausesMap)
       : context_{context}, directiveClausesMap_(directiveClausesMap) {}
   virtual ~DirectiveStructureChecker() {}
 
+  using ClauseMapTy = std::multimap<C, const PC *>;
   struct DirectiveContext {
     DirectiveContext(parser::CharBlock source, D d)
         : directiveSource{source}, directive{d} {}
 
     parser::CharBlock directiveSource{nullptr};
     parser::CharBlock clauseSource{nullptr};
     D directive;
     common::EnumSet<C, ClauseEnumSize> allowedClauses{};
     common::EnumSet<C, ClauseEnumSize> allowedOnceClauses{};
     common::EnumSet<C, ClauseEnumSize> allowedExclusiveClauses{};
     common::EnumSet<C, ClauseEnumSize> requiredClauses{};
 
     const PC *clause{nullptr};
-    std::multimap<C, const PC *> clauseInfo;
+    ClauseMapTy clauseInfo;
     std::list<C> actualClauses;
     Symbol *loopIV{nullptr};
   };
 
   void SetLoopIv(Symbol *symbol) { GetContext().loopIV = symbol; }
 
   // back() is the top of the stack
   DirectiveContext &GetContext() {
protected:
   const PC *FindClause(C type) {
     auto it{GetContext().clauseInfo.find(type)};
     if (it != GetContext().clauseInfo.end()) {
       return it->second;
     }
     return nullptr;
   }
 
+  std::pair<typename ClauseMapTy::iterator, typename ClauseMapTy::iterator>
+  FindClauses(C type) {
+    auto it{GetContext().clauseInfo.equal_range(type)};
+    return it;
+  }
+
   void PushContext(const parser::CharBlock &source, D dir) {
     dirContext_.emplace_back(source, dir);
   }
 
   DirectiveContext *GetEnclosingContextWithDir(D dir) {
     CHECK(!dirContext_.empty());
     auto it{dirContext_.rbegin()};
     while (++it != dirContext_.rend()) {

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

 //===-- lib/Semantics/check-omp-structure.cpp -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "check-omp-structure.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/tools.h"
 #include <algorithm>
 
 namespace Fortran::semantics {
 
 // Use when clause falls under 'struct OmpClause' in 'parse-tree.h'.
 #define CHECK_SIMPLE_CLAUSE(X, Y) \
   void OmpStructureChecker::Enter(const parser::OmpClause::X &) { \
     CheckAllowed(llvm::omp::Clause::Y); \
   }
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);
             }
           }
         }
       }
     }
-    // TODO: A list-item cannot appear in more than one aligned clause
+    // A list-item cannot appear in more than one aligned clause
+    semantics::SymbolSet 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));
+      }
+    }
   } // 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});
   }
 
   CheckRequireAtLeastOneOf();
 }
 
 void OmpStructureChecker::Enter(const parser::OmpClause &x) {

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

+! RUN: %S/test_errors.sh %s %t %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 ALIGNED clauses
+  !$omp simd aligned(a, a)
+  do i = 1, 10
+    a(i) = i
+  end do
+  !$omp end simd
+
+  !ERROR: List item 'a' present at multiple ALIGNED clauses
+  !ERROR: List item 'b' present at multiple ALIGNED clauses
+  !$omp simd aligned(a,a) aligned(b) aligned(b)
+  do i = 1, 10
+    a(i) = i
+    b(i) = i
+  end do
+  !$omp end simd
+
+  !ERROR: List item 'a' present at multiple ALIGNED clauses
+  !$omp simd aligned(a) aligned(a)
+  do i = 1, 10
+    a(i) = i
+  end do
+  !$omp end simd
+
+  !$omp simd aligned(a) aligned(b)
+  do i = 1, 10
+    a(i) = i
+    b(i) = i
+  end do
+  !$omp end simd
+
+  !ERROR: List item 'a' present at multiple ALIGNED clauses
+  !$omp simd aligned(a) private(a) aligned(a)
+  do i = 1, 10
+    a(i) = i
+    b(i) = i
+  end do
+  !$omp end simd
+
+  print *, a
+
+end program omp_simd