[Flang] [OpenMP] [Semantics] Add missing semantic check for MAP clause.

[raghavendhra]

Summary

Added support for following semantic check for MAP clause.

• A list item cannot appear in both a map clause and a data-sharing attribute clause on the same target construct.

[raghavendhra]

Rebasing the patch

[NimishMishra]

flang/lib/Semantics/resolve-directives.cpp

532

Question: are we sure that the default ompFlag value should be OmpMapToFrom? Because I see a re-assignment in case Type::Tofrom:.

556

Redundant break? Rather, if the OmpMapType cannot be anything other than the 6 types listed here, then is this a redundant default that can be removed?

569

Question: Is this information used somewhere? As far as I understand, the semantic check resides in checkExclusivelists.

1955

Is this cast necessary for the function call? OmpFlagToClauseName function can also reside as a function in OmpAttributeVisitor imo. It doesn't seem like this function is providing anything specific to the class Symbol. Is it necessary to make this function encapsulated in the Symbol class?

[raghavendhra]

Thanks for your review comments Nimish!

flang/lib/Semantics/resolve-directives.cpp

532

map-type is optional and will take a default value of tofrom

As per the OpenMP 5.1 standard (Pg 353 Line # 2)

• If a map-type is not specified, the map-type defaults to tofrom.

So, Line 531 is used for no map-type modifier present case.

556

Sure, I will remove this in my next patch.

569

You are correct, this code snippet is used to set appropriate ompFlag related to MAP clause which will be used in checkExclusiveLists to compare against PRIVATE and FIRSTPRIVATE lists

1955

The major reason I defined in Symbol is because all the OmpFlags are defined in Symbol class. I can give a try to move it to OmpAttributeVisitor if needed.

[NimishMishra]

LGTM, with a couple of nits (noted below). Please wait for some more time in case anyone has any comments.

flang/lib/Semantics/resolve-directives.cpp

556

Sure thanks!

1955

That is ok. The OmpFlags can reside in Symbol class. The conversion to clause name can be shifted to OmpAttributeVisitor. Maybe we can take a second opinion on this.

[shraiysh]

Please make sure that there are testcases for all the checks that have checkExclusiveLists.

Not approving because I am not super familiar with this part of the code, but LGTM.

flang/lib/Semantics/resolve-directives.cpp

556

Small nit - instead of removing it, maybe put an assert(false) here. If for some reason the code goes here (like introduction of another flag in the future) it should error out.

[raghavendhra]

Addressed review comments. Changed MAP clause semantic check to run through permutation and
combination of different MAP types and data sharing attribute clauses on
checkExclusiveLists(). Rebased the patch.

[raghavendhra]

Rebasing patch

[clementval]

Looks like this patch triggers a new failure https://lab.llvm.org/buildbot/#/builders/175/builds/35729

[raghavendhra]

In D158807#4641353, @clementval wrote:

Looks like this patch triggers a new failure https://lab.llvm.org/buildbot/#/builders/175/builds/35729

Thanks for catching this Valentin. Sorry about this build failure and it has been fixed now with https://github.com/llvm/llvm-project/commit/872aa457ba90a90d2005293eebf1775b76a78b59

flang/include/flang/Semantics/symbol.h

ENUM_CLASS(Flag,
       AccDeclare,
       // OpenACC data-movement attribute
       AccDevice, AccHost, AccSelf,
       // OpenACC miscellaneous flags
       AccCommonBlock, AccThreadPrivate, AccReduction, AccNone, AccPreDetermined,
       // OpenMP data-sharing attribute
       OmpShared, OmpPrivate, OmpLinear, OmpFirstPrivate, OmpLastPrivate,
       // OpenMP data-mapping attribute
-      OmpMapTo, OmpMapFrom, OmpMapAlloc, OmpMapRelease, OmpMapDelete,
-      OmpUseDevicePtr, OmpUseDeviceAddr,
+      OmpMapTo, OmpMapFrom, OmpMapToFrom, OmpMapAlloc, OmpMapRelease,
+      OmpMapDelete, OmpUseDevicePtr, OmpUseDeviceAddr,
       // OpenMP data-copying attribute
       OmpCopyIn, OmpCopyPrivate,
       // OpenMP miscellaneous flags
       OmpCommonBlock, OmpReduction, OmpAligned, OmpNontemporal, OmpAllocate,
       OmpDeclarativeAllocateDirective, OmpExecutableAllocateDirective,
       OmpDeclareSimd, OmpDeclareTarget, OmpThreadprivate, OmpDeclareReduction,
       OmpFlushed, OmpCriticalLock, OmpIfSpecified, OmpNone, OmpPreDetermined);
   using Flags = common::EnumSet<Flag, Flag_enumSize>;
public:
   const Scope *scope() const { return scope_; }
   void set_scope(Scope *scope) { scope_ = scope; }
   std::size_t size() const { return size_; }
   void set_size(std::size_t size) { size_ = size; }
   std::size_t offset() const { return offset_; }
   void set_offset(std::size_t offset) { offset_ = offset; }
   // Give the symbol a name with a different source location but same chars.
   void ReplaceName(const SourceName &);
+  std::string OmpFlagToClauseName(Flag ompFlag);
 
   // Does symbol have this type of details?
   template <typename D> bool has() const {
     return std::holds_alternative<D>(details_);
   }
 
   // Return a non-owning pointer to details if it is type D, else nullptr.
   template <typename D> D *detailsIf() { return std::get_if<D>(&details_); }

flang/lib/Semantics/resolve-directives.cpp

public:
   void Post(const parser::AltReturnSpec &altReturnSpec) {
     CheckSourceLabel(altReturnSpec.v);
   }
   void Post(const parser::ErrLabel &errLabel) { CheckSourceLabel(errLabel.v); }
   void Post(const parser::EndLabel &endLabel) { CheckSourceLabel(endLabel.v); }
   void Post(const parser::EorLabel &eorLabel) { CheckSourceLabel(eorLabel.v); }
 
   void Post(const parser::OmpMapClause &x) {
+    Symbol::Flag ompFlag = Symbol::Flag::OmpMapToFrom;
+    if (const auto &maptype{std::get<std::optional<parser::OmpMapType>>(x.t)}) {
+      using Type = parser::OmpMapType::Type;
+      const Type &type{std::get<Type>(maptype->t)};
+      switch (type) {
+      case Type::To:
+        ompFlag = Symbol::Flag::OmpMapTo;
+        break;
+      case Type::From:
+        ompFlag = Symbol::Flag::OmpMapFrom;
+        break;
+      case Type::Tofrom:
+        ompFlag = Symbol::Flag::OmpMapToFrom;
+        break;
+      case Type::Alloc:
+        ompFlag = Symbol::Flag::OmpMapAlloc;
+        break;
+      case Type::Release:
+        ompFlag = Symbol::Flag::OmpMapRelease;
+        break;
+      case Type::Delete:
+        ompFlag = Symbol::Flag::OmpMapDelete;
+        break;
+      default:
+        assert(false && "Unsupported map-type");
+        break;
+      }
+    }
     const auto &ompObjList{std::get<parser::OmpObjectList>(x.t)};
     for (const auto &ompObj : ompObjList.v) {
       common::visit(
           common::visitors{
               [&](const parser::Designator &designator) {
                 if (const auto *name{
                         semantics::getDesignatorNameIfDataRef(designator)}) {
+                  if (name->symbol) {
+                    name->symbol->set(ompFlag);
+                    AddToContextObjectWithDSA(*name->symbol, ompFlag);
+                  }
                   if (name->symbol &&
                       semantics::IsAssumedSizeArray(*name->symbol)) {
                     context_.Say(designator.source,
                         "Assumed-size whole arrays may not appear on the %s "
                         "clause"_err_en_US,
                         "MAP");
                   }
                 }

 void OmpAttributeVisitor::ResolveOmpObject(
     const parser::OmpObject &ompObject, Symbol::Flag ompFlag) {
   common::visit(
       common::visitors{
           [&](const parser::Designator &designator) {
             if (const auto *name{
                     semantics::getDesignatorNameIfDataRef(designator)}) {
               if (auto *symbol{ResolveOmp(*name, ompFlag, currScope())}) {
+                auto checkExclusivelists =
+                    [&](const Symbol *symbol1, Symbol::Flag firstOmpFlag,
+                        Symbol *symbol2, Symbol::Flag secondOmpFlag) {
+                      if ((symbol1->test(firstOmpFlag) &&
+                              symbol2->test(secondOmpFlag)) ||
+                          (symbol1->test(secondOmpFlag) &&
+                              symbol2->test(firstOmpFlag))) {
+                        context_.Say(designator.source,
+                            "Variable '%s' may not "
+                            "appear on both %s and %s "
+                            "clauses on a %s construct"_err_en_US,
+                            symbol2->name(),
+                            const_cast<Symbol *>(symbol1)->OmpFlagToClauseName(
+                                firstOmpFlag),
+                            symbol2->OmpFlagToClauseName(secondOmpFlag),
+                            parser::ToUpperCaseLetters(
+                                llvm::omp::getOpenMPDirectiveName(
+                                    GetContext().directive)
+                                    .str()));
+                      }
+                    };
                 if (dataCopyingAttributeFlags.test(ompFlag)) {
                   CheckDataCopyingClause(*name, *symbol, ompFlag);
                 } else {
                   AddToContextObjectWithDSA(*symbol, ompFlag);
                   if (dataSharingAttributeFlags.test(ompFlag)) {
                     CheckMultipleAppearances(*name, *symbol, ompFlag);
                   }
                   if (privateDataSharingAttributeFlags.test(ompFlag)) {
common::visit(
                   context_.Say(designator.source, // 2.15.3
                       "List items must be declared in the same scoping unit "
                       "in which the %s directive appears"_err_en_US,
                       parser::ToUpperCaseLetters(
                           llvm::omp::getOpenMPDirectiveName(
                               GetContext().directive)
                               .str()));
                 }
-                if ((GetContext().directive ==
-                        llvm::omp::Directive::OMPD_target_data) &&
-                    (((ompFlag == Symbol::Flag::OmpUseDevicePtr) &&
-                         symbol->test(Symbol::Flag::OmpUseDeviceAddr)) ||
-                        ((ompFlag == Symbol::Flag::OmpUseDeviceAddr) &&
-                            symbol->test(Symbol::Flag::OmpUseDevicePtr)))) {
-                  context_.Say(designator.source,
-                      "Variable '%s' may not "
-                      "appear on both USE_DEVICE_PTR and USE_DEVICE_ADDR "
-                      "clauses on a TARGET DATA construct"_err_en_US,
-                      symbol->name());
+                if (GetContext().directive ==
+                    llvm::omp::Directive::OMPD_target_data) {
+                  checkExclusivelists(symbol, Symbol::Flag::OmpUseDevicePtr,
+                      symbol, Symbol::Flag::OmpUseDeviceAddr);
+                }
+                if (llvm::omp::allDistributeSet.test(GetContext().directive)) {
+                  checkExclusivelists(symbol, Symbol::Flag::OmpFirstPrivate,
+                      symbol, Symbol::Flag::OmpLastPrivate);
+                }
+                if (llvm::omp::allTargetSet.test(GetContext().directive)) {
+                  const auto *hostAssocSym{symbol};
+                  if (const auto *details{
+                          symbol->detailsIf<HostAssocDetails>()}) {
+                    hostAssocSym = &details->symbol();
+                  }
+                  Symbol::Flag dataMappingAttributeFlags[] = {
+                      Symbol::Flag::OmpMapTo, Symbol::Flag::OmpMapFrom,
+                      Symbol::Flag::OmpMapToFrom, Symbol::Flag::OmpMapAlloc,
+                      Symbol::Flag::OmpMapRelease, Symbol::Flag::OmpMapDelete};
+
+                  Symbol::Flag dataSharingAttributeFlags[] = {
+                      Symbol::Flag::OmpPrivate, Symbol::Flag::OmpFirstPrivate,
+                      Symbol::Flag::OmpLastPrivate, Symbol::Flag::OmpShared,
+                      Symbol::Flag::OmpLinear};
+
+                  for (Symbol::Flag ompFlag1 : dataMappingAttributeFlags) {
+                    for (Symbol::Flag ompFlag2 : dataSharingAttributeFlags) {
+                      checkExclusivelists(
+                          hostAssocSym, ompFlag1, symbol, ompFlag2);
+                    }
                   }
-                if (llvm::omp::allDistributeSet.test(GetContext().directive) &&
-                    (((ompFlag == Symbol::Flag::OmpFirstPrivate) &&
-                         symbol->test(Symbol::Flag::OmpLastPrivate)) ||
-                        ((ompFlag == Symbol::Flag::OmpLastPrivate) &&
-                            symbol->test(Symbol::Flag::OmpFirstPrivate)))) {
-                  context_.Say(designator.source,
-                      "Variable '%s' may not "
-                      "appear on both FIRSTPRIVATE and LASTPRIVATE "
-                      "clauses on a DISTRIBUTE construct"_err_en_US,
-                      symbol->name());
                 }
               }
             } else {
               // Array sections to be changed to substrings as needed
               if (AnalyzeExpr(context_, designator)) {
                 if (std::holds_alternative<parser::Substring>(designator.u)) {
                   context_.Say(designator.source,
                       "Substrings are not allowed on OpenMP "

flang/lib/Semantics/symbol.cpp

SourceName GenericKind::AsFortran(common::DefinedIo x) {
   return {name, std::strlen(name)};
 }
 
 bool GenericKind::Is(GenericKind::OtherKind x) const {
   const OtherKind *y{std::get_if<OtherKind>(&u)};
   return y && *y == x;
 }
 
+std::string Symbol::OmpFlagToClauseName(Symbol::Flag ompFlag) {
+  std::string clauseName;
+  switch (ompFlag) {
+  case Symbol::Flag::OmpShared:
+    clauseName = "SHARED";
+    break;
+  case Symbol::Flag::OmpPrivate:
+    clauseName = "PRIVATE";
+    break;
+  case Symbol::Flag::OmpLinear:
+    clauseName = "LINEAR";
+    break;
+  case Symbol::Flag::OmpFirstPrivate:
+    clauseName = "FIRSTPRIVATE";
+    break;
+  case Symbol::Flag::OmpLastPrivate:
+    clauseName = "LASTPRIVATE";
+    break;
+  case Symbol::Flag::OmpMapTo:
+  case Symbol::Flag::OmpMapFrom:
+  case Symbol::Flag::OmpMapToFrom:
+  case Symbol::Flag::OmpMapAlloc:
+  case Symbol::Flag::OmpMapRelease:
+  case Symbol::Flag::OmpMapDelete:
+    clauseName = "MAP";
+    break;
+  case Symbol::Flag::OmpUseDevicePtr:
+    clauseName = "USE_DEVICE_PTR";
+    break;
+  case Symbol::Flag::OmpUseDeviceAddr:
+    clauseName = "USE_DEVICE_ADDR";
+    break;
+  case Symbol::Flag::OmpCopyIn:
+    clauseName = "COPYIN";
+    break;
+  case Symbol::Flag::OmpCopyPrivate:
+    clauseName = "COPYPRIVATE";
+    break;
+  default:
+    clauseName = "";
+    break;
+  }
+  return clauseName;
+}
+
 bool SymbolOffsetCompare::operator()(
     const SymbolRef &x, const SymbolRef &y) const {
   const Symbol *xCommon{FindCommonBlockContaining(*x)};
   const Symbol *yCommon{FindCommonBlockContaining(*y)};
   if (xCommon) {
     if (yCommon) {
       const SymbolSourcePositionCompare sourceCmp;
       if (sourceCmp(*xCommon, *yCommon)) {

flang/test/Semantics/OpenMP/firstprivate01.f90

program omp_firstprivate
   !$omp end distribute
   !$omp end teams
 
   !$omp teams distribute firstprivate(a) lastprivate(b)
   do i = 1, 10
     a(i) = a(i) + b(i) - i
   end do
   !$omp end teams distribute
-  !ERROR: Variable 'b' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a DISTRIBUTE construct
+  !ERROR: Variable 'b' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a TEAMS DISTRIBUTE construct
   !$omp teams distribute firstprivate(a,b) lastprivate(b)
   do i = 1, 10
     a(i) = a(i) + b(i) - i
   end do
   !$omp end teams distribute
-  !ERROR: Variable 'a' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a DISTRIBUTE construct
-  !ERROR: Variable 'b' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a DISTRIBUTE construct
+  !ERROR: Variable 'a' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a TEAMS DISTRIBUTE construct
+  !ERROR: Variable 'b' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a TEAMS DISTRIBUTE construct
   !$omp teams distribute firstprivate(a,b) lastprivate(a,b)
   do i = 1, 10
     a(i) = a(i) + b(i) - i
   end do
   !$omp end teams distribute
-  !ERROR: Variable 'b' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a DISTRIBUTE construct
+  !ERROR: Variable 'b' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a TEAMS DISTRIBUTE construct
   !$omp teams distribute lastprivate(a,b) firstprivate(b)
   do i = 1, 10
     a(i) = a(i) + b(i) - i
   end do
   !$omp end teams distribute
-  !ERROR: Variable 'b' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a DISTRIBUTE construct
-  !ERROR: Variable 'a' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a DISTRIBUTE construct
+  !ERROR: Variable 'b' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a TEAMS DISTRIBUTE construct
+  !ERROR: Variable 'a' may not appear on both FIRSTPRIVATE and LASTPRIVATE clauses on a TEAMS DISTRIBUTE construct
   !$omp teams distribute lastprivate(a,b) firstprivate(b,a)
   do i = 1, 10
     a(i) = a(i) + b(i) - i
   end do
   !$omp end teams distribute
   !$omp end target
 
   print *, a, b

flang/test/Semantics/OpenMP/symbol08.f90

do i=1,5
   end do
  end do
 !$omp end taskloop
 end subroutine test_taskloop
 
 ! Rule a); OpenMP 4.5 Examples teams.2.f90
 ! TODO: reduction; data-mapping attributes
 !DEF: /dotprod (Subroutine) Subprogram
-!DEF: /dotprod/b ObjectEntity REAL(4)
-!DEF: /dotprod/c ObjectEntity REAL(4)
+!DEF: /dotprod/b (OmpMapTo) ObjectEntity REAL(4)
+!DEF: /dotprod/c (OmpMapTo) ObjectEntity REAL(4)
 !DEF: /dotprod/n ObjectEntity INTEGER(4)
 !DEF: /dotprod/block_size ObjectEntity INTEGER(4)
 !DEF: /dotprod/num_teams ObjectEntity INTEGER(4)
 !DEF: /dotprod/block_threads ObjectEntity INTEGER(4)
 subroutine dotprod (b, c, n, block_size, num_teams, block_threads)
  implicit none
  !REF: /dotprod/n
  integer n
  !REF: /dotprod/b
  !REF: /dotprod/n
  !REF: /dotprod/c
- !DEF: /dotprod/sum ObjectEntity REAL(4)
+ !DEF: /dotprod/sum (OmpMapToFrom) ObjectEntity REAL(4)
  real b(n), c(n), sum
  !REF: /dotprod/block_size
  !REF: /dotprod/num_teams
  !REF: /dotprod/block_threads
  !DEF: /dotprod/i ObjectEntity INTEGER(4)
  !DEF: /dotprod/i0 ObjectEntity INTEGER(4)
  integer block_size, num_teams, block_threads, i, i0
  !REF: /dotprod/sum

flang/test/Semantics/OpenMP/target01.f90

+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
+! OpenMP Version 4.5
+
+integer :: x
+!ERROR: Variable 'x' may not appear on both MAP and PRIVATE clauses on a TARGET construct
+!$omp target map(x) private(x)
+x = x + 1
+!$omp end target
+
+end

flang/test/Semantics/OpenMP/target02.f90

+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
+! OpenMP Version 4.5
+
+program p
+integer :: y
+!ERROR: Variable 'y' may not appear on both MAP and FIRSTPRIVATE clauses on a TARGET construct
+!$omp target map(y) firstprivate(y)
+y = y + 1
+!$omp end target
+!ERROR: Variable 'y' may not appear on both MAP and FIRSTPRIVATE clauses on a TARGET SIMD construct
+!$omp target simd map(y) firstprivate(y)
+do i=1,1
+  y = y + 1
+end do
+!$omp end target simd
+
+end program p