[Flang][OpenMP 4.5] Add semantic check for OpenMP shared and private clause

[yhegde]

Summary

Semantic check for OpenMP 4.5 - 2.15.3.2 and 2.15.3.3 shared and private clause

Test cases : omp-parallel-shared.f90

        omp-parallel-shared02.f90
        omp-parallel-shared03.f90
        omp-parallel-shared04.f90    
and  omp-parallel-private.f90
        omp-parallel-private02.f90
        omp-parallel-private03.f90
        omp-parallel-private04.f90

[clementval]

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

61

Since this is OpenMP specific, it should be in check-omp-structure and not here.

66

Same here. Should be moved to check-omp-structure.h

[clementval]

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

199

OmpObjectList is specific to OpenMP and doesn't fit in check-directive-structure.h

238

In the rest of Flang there is no blank line between each specific visitor so you might want to stick with the same coding style.

[tskeith]

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

212

This code just seems to be trying to determine if ompObject is a StructureComponent. If so, it could be simplified to:

if (parser::Unwrap<parser::StructureComponent>(ompObject)) {

[kiranchandramohan]

Thanks for this patch. Did you miss the array element check?

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

413

Nit: you can skip the standard section number here.

423

Nit: you can skip the standard section number here.

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

165 ↗

(On Diff #298335)

Nit: You can skip the section number here.

flang/test/Semantics/omp-parallel-private.f90

9

Nit: alignment

15–18

Is this loop required for this test?

22–25

Nit: Alignment.

flang/test/Semantics/omp-parallel-shared.f90

9

Nit: alignment

12–16

Is this loop required for this test?

[yhegde]

1. Array element check added along with test cases to test the same.

[yhegde]

clang format

[Prashanth]

This comment was removed by Prashanth.

[clementval]

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

437

Clause names are usually uppercase in the error message.

[kiranchandramohan]

LGTM.
Please wait for approval from @clementval after uppercasing the clause names.

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

424

Nit: Would a better name be CheckIsVarPartOfAnotherVar?

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

165 ↗

(On Diff #299106)

Nit: Can this be private?

[yhegde]

1. Function name changed to CheckIsVarPartOfAnotherVar
2. Clause names in uppercase.

[clementval]

Ok from my side. Maybe @kiranchandramohan wants to double check the latest changes.

[kiranchandramohan]

LGTM.

[kiranchandramohan]

@yhegde please submit. If you require any inputs then please let me know. This will be helpful for D91159.

[yhegde]

Thanks a lot for the review comments.
Requesting a quick review of this accepted patch. I had to revert the code part related to Shared and Private clause from the review https://reviews.llvm.org/D90324.

Test cases : omp-parallel-shared.01f90

        omp-parallel-shared02.f90
        omp-parallel-shared03.f90
        omp-parallel-shared04.f90    
and  omp-parallel-private.01f90
        omp-parallel-private02.f90
        omp-parallel-private03.f90
        omp-parallel-private04.f90

[yhegde]

Shall I assume there are no issues with this patch. ?

[kiranchandramohan]

No issues, please go ahead.

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

struct DirectiveContext {
     common::EnumSet<C, ClauseEnumSize> allowedExclusiveClauses{};
     common::EnumSet<C, ClauseEnumSize> requiredClauses{};
 
     const PC *clause{nullptr};
     std::multimap<C, const PC *> clauseInfo;
     std::list<C> actualClauses;
   };
 
+  // OpenMP 4.5 : 2.15.3.2 and 2.15.3.3
+  const parser::StructureComponent *CheckAssociatedVariant(
+      const parser::Name &name, const parser::DataRef &dataRef);
+
+  // OpenMP 4.5 : 2.15.3.2 and 2.15.3.3
+  void CheckObjectListStructure(const parser::OmpObjectList &x);
+
   // back() is the top of the stack
   DirectiveContext &GetContext() {
     CHECK(!dirContext_.empty());
     return dirContext_.back();
   }
 
   void SetContextClause(const PC &clause) {
     GetContext().clauseSource = clause.source;
protected:
   SemanticsContext &context_;
   std::vector<DirectiveContext> dirContext_; // used as a stack
   std::unordered_map<D, DirectiveClauses<C, ClauseEnumSize>>
       directiveClausesMap_;
 
   std::string ClauseSetToString(const common::EnumSet<C, ClauseEnumSize> set);
 };
 
+// OpenMP 4.5 : 2.15.3.2 and 2.15.3.3
+template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
+void DirectiveStructureChecker<D, C, PC, ClauseEnumSize>::
+    CheckObjectListStructure(const parser::OmpObjectList &objList) {
+
+  for (const auto &ompObject : objList.v) {
+    std::visit(
+        common::visitors{
+            [&](const parser::Designator &designator) {
+              if (const auto *dataRef{
+                      std::get_if<parser::DataRef>(&designator.u)}) {
+
+                const parser::Name *name{
+                    std::get_if<parser::Name>(&dataRef->u)};
+
+                if (const parser::StructureComponent *
+                    ssc{CheckAssociatedVariant(*name, *dataRef)}) {
+
+                  context_.Say(GetContext().clauseSource,
+                      "A variable that is part of another variable (as an "
+                      "array or structure element)"
+                      " cannot appear in a private or shared clause."_err_en_US);
+                }
+              }
+            },
+            [&](const parser::Name &name) {},
+
+        },
+
+        ompObject.u);
+  }
+}
+
+// OpenMP 4.5 : 2.15.3.2 and 2.15.3.3
+template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
+const parser::StructureComponent *
+DirectiveStructureChecker<D, C, PC, ClauseEnumSize>::CheckAssociatedVariant(
+    const parser::Name &name, const parser::DataRef &dataRef) {
+
+  return std::visit(
+      common::visitors{
+
+          [&](const parser::Name &name) -> const parser::StructureComponent * {
+            return nullptr;
+          },
+
+          [&](const common::Indirection<parser::StructureComponent> &sc)
+              -> const parser::StructureComponent * { return &sc.value(); },
+
+          [&](const common::Indirection<parser::ArrayElement> &ae)
+              -> const parser::StructureComponent * { return nullptr; },
+
+          [&](const common::Indirection<parser::CoindexedNamedObject> &ci)
+              -> const parser::StructureComponent * { return nullptr; },
+
+      },
+      dataRef.u);
+}
+
 // Check that only clauses included in the given set are present after the given
 // clause.
 template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
 void DirectiveStructureChecker<D, C, PC, ClauseEnumSize>::CheckOnlyAllowedAfter(
     C clause, common::EnumSet<C, ClauseEnumSize> set) {
   bool enforceCheck = false;
   for (auto cl : GetContext().actualClauses) {
     if (cl == clause) {

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

if (llvm::omp::doSimdSet.test(GetContext().directive)) {
           ContextDirectiveAsFortran());
     }
   }
 }
 void OmpStructureChecker::Enter(const parser::OmpClause::Priority &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_priority);
   RequiresPositiveParameter(llvm::omp::Clause::OMPC_priority, x.v);
 }
-void OmpStructureChecker::Enter(const parser::OmpClause::Private &) {
+void OmpStructureChecker::Enter(const parser::OmpClause::Private &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_private);
+
+  // OpenMP 4.5 : 2.15.3.3
+  CheckObjectListStructure(x.v);
 }
 void OmpStructureChecker::Enter(const parser::OmpClause::Safelen &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_safelen);
   RequiresConstantPositiveParameter(llvm::omp::Clause::OMPC_safelen, x.v);
 }
-void OmpStructureChecker::Enter(const parser::OmpClause::Shared &) {
+void OmpStructureChecker::Enter(const parser::OmpClause::Shared &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_shared);
+
+  // OpenMP 4.5 : 2.15.3.2
+  CheckObjectListStructure(x.v);
 }
 void OmpStructureChecker::Enter(const parser::OmpClause::Simdlen &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_simdlen);
   RequiresConstantPositiveParameter(llvm::omp::Clause::OMPC_simdlen, x.v);
 }
 void OmpStructureChecker::Enter(const parser::OmpClause::ThreadLimit &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_thread_limit);
   RequiresPositiveParameter(llvm::omp::Clause::OMPC_thread_limit, x.v);
 }
 void OmpStructureChecker::Enter(const parser::OmpClause::To &) {
   CheckAllowed(llvm::omp::Clause::OMPC_to);
 }
 void OmpStructureChecker::Enter(const parser::OmpClause::Link &) {
   CheckAllowed(llvm::omp::Clause::OMPC_link);
 }
 void OmpStructureChecker::Enter(const parser::OmpClause::Uniform &) {
   CheckAllowed(llvm::omp::Clause::OMPC_uniform);
 }
 void OmpStructureChecker::Enter(const parser::OmpClause::UseDevicePtr &) {
   CheckAllowed(llvm::omp::Clause::OMPC_use_device_ptr);
 }

flang/test/Semantics/omp-parallel-private.f90

+!RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.15.3.3 parallel private Clause
+program omp_parallel_private
+      integer :: i, j, a(10), b(10), c(10)
+      integer :: k = 10
+      type my_type
+              integer :: array(10)
+      end type my_type
+
+      type(my_type) :: my_var
+
+
+      do i = 1, 10
+      a(i) = i
+      b(i) = i * i
+      end do
+
+!ERROR: A variable that is part of another variable (as an array or structure element) cannot appear in a private or shared clause.
+      !$omp parallel private(my_var%array)
+      do i = 1, 10
+      c(i) = a(i) + b(i) + k
+      my_var%array(i) = k
+      end do
+      !$omp end parallel
+
+end program omp_parallel_private

flang/test/Semantics/omp-parallel-shared.f90

+!RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.15.3.2 parallel shared Clause
+program omp_parallel_shared
+  integer :: i, j, a(10), b(10), c(10)
+  integer :: k = 10
+  type my_type
+  integer :: array(10)
+  end type my_type
+
+ type(my_type) :: my_var
+  do i = 1, 10
+     a(i) = i
+     b(i) = i * i
+  end do
+!ERROR: A variable that is part of another variable (as an array or structure element) cannot appear in a private or shared clause.
+  !$omp parallel shared(my_var%array)
+    do i = 1, 10
+     c(i) = a(i) + b(i) +  k
+    my_var%array(i) = k
+   end do
+  !$omp end parallel
+
+end program omp_parallel_shared