[Flang][OpenMP][OpenACC] Fix exit out of a region in OpenMP parallel construct.

[sameeranjoshi]

Summary

From below mentioned standard references
OpenACC 3.0 Standards document
840 • A program may not branch into or out of an OpenACC parallel construct

OpenMP 5.0 Standards document
A program that branches into or out of a parallel region is non-conforming.

This patch
Resolves the issue of exit out of a parallel region, other branching out issues like goto statements are not handled with this patch.
Moves code from D87906 to be reused by other OpenMP/OpenACC to check-directive-structure.h.
Adds support in OpenMP parallel construct and a test case to verify.

[sameeranjoshi]

Rebased on master and updated after D87906.

[clementval]

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

76

DirectiveStructureChecker:: is not necessary here and it's not added for other checks.

84

DirectiveStructureChecker:: is not necessary here and it's not added for other checks.

[sameeranjoshi]

Remove unnecessary DirectiveStructureChecker:: from check-acc-structure.cpp and check-omp-structure.cpp.

[clementval]

flang/test/Semantics/omp-clause-validity01.f90

169

Is the indentation of directive on purpose?

[sameeranjoshi]

flang/test/Semantics/omp-clause-validity01.f90

169

I was trying to use a tool to make formatting like clang-format named fprettify.
I will see how other code is.

[sameeranjoshi]

Fix indentation.

[clementval]

LGTM just a nit comment if you can update before landing this patch.

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

29

nit: Blank line before this block of code.

[kiranchandramohan]

Nit: I believe this only handles exit out of a region. It does not fully handle the branching (goto etc) out issue. Can you update the title and description to reflect this?

[awarzynski]

Sadly this patch is causing build failures on our Flang buildbots: http://lab.llvm.org:8011/#/builders/33/builds/386

FAILED: tools/flang/lib/Semantics/CMakeFiles/obj.FortranSemantics.dir/check-acc-structure.cpp.o 
/usr/bin/clang++-10  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/flang/lib/Semantics -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/include -Itools/flang/include -Iinclude -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/include -isystem /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/../mlir/include -isystem tools/mlir/include -isystem tools/clang/include -isystem /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/../clang/include -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Werror -Wno-deprecated-copy -Wno-string-conversion -Wno-unused-command-line-argument -Wstring-conversion           -Wcovered-switch-default -Wno-nested-anon-types -O3     -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/flang/lib/Semantics/CMakeFiles/obj.FortranSemantics.dir/check-acc-structure.cpp.o -MF tools/flang/lib/Semantics/CMakeFiles/obj.FortranSemantics.dir/check-acc-structure.cpp.o.d -o tools/flang/lib/Semantics/CMakeFiles/obj.FortranSemantics.dir/check-acc-structure.cpp.o -c /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-acc-structure.cpp
In file included from /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-acc-structure.cpp:8:
In file included from /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-acc-structure.h:17:
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-directive-structure.h:39:9: error: field 'currentDirective_' will be initialized after field 'upperCaseDirName_' [-Werror,-Wreorder-ctor]
        currentDirective_{directive}, upperCaseDirName_{
        ^
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-directive-structure.h:273:25: note: in instantiation of member function 'Fortran::semantics::NoBranchingEnforce<llvm::acc::Directive>::NoBranchingEnforce' requested here
  NoBranchingEnforce<D> noBranchingEnforce{
                        ^
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-acc-structure.cpp:89:5: note: in instantiation of member function 'Fortran::semantics::DirectiveStructureChecker<llvm::acc::Directive, llvm::acc::Clause, Fortran::parser::AccClause, 45>::CheckNoBranching' requested here
    CheckNoBranching(block, GetContext().directive, blockDir.source);
    ^

[awarzynski]

I've also noticed other patches causing build failures. Fixing this one felt rather straightforward so I took the liberty of submitting a fix: https://github.com/llvm/llvm-project/commit/b7d1271a01b72a079ea016a9d4e31a73091d5f0e. I hope that that's fine.

[clementval]

In D88655#2364244, @awarzynski wrote:

I've also noticed other patches causing build failures. Fixing this one felt rather straightforward so I took the liberty of submitting a fix: https://github.com/llvm/llvm-project/commit/b7d1271a01b72a079ea016a9d4e31a73091d5f0e. I hope that that's fine.

Thanks for fixing this @awarzynski

[sameeranjoshi]

In D88655#2364244, @awarzynski wrote:

I've also noticed other patches causing build failures. Fixing this one felt rather straightforward so I took the liberty of submitting a fix: https://github.com/llvm/llvm-project/commit/b7d1271a01b72a079ea016a9d4e31a73091d5f0e. I hope that that's fine.

Thanks a lot @awarzynski I would add to my local builds -DFLANG_ENABLE_WERROR=ON from now onwards.

[kiranchandramohan]

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

132

A stop statement is allowed inside a pararallel region in OpenMP. Possibly in OpenACC as well.

Page 40 line 6. https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf

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

#include "llvm/Frontend/OpenACC/ACC.cpp.inc"
   void Enter(const parser::AccClause::Vector &);
   void Enter(const parser::AccClause::VectorLength &);
   void Enter(const parser::AccClause::Wait &);
   void Enter(const parser::AccClause::Worker &);
   void Enter(const parser::AccClause::Write &);
 
 private:
 
-  void CheckNoBranching(const parser::Block &block,
-      const llvm::acc::Directive directive,
-      const parser::CharBlock &directiveSource) const;
-
   llvm::StringRef getClauseName(llvm::acc::Clause clause) override;
   llvm::StringRef getDirectiveName(llvm::acc::Directive directive) override;
 };
 
 } // namespace Fortran::semantics
 
 #endif // FORTRAN_SEMANTICS_CHECK_ACC_STRUCTURE_H_

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

 
 static constexpr inline AccClauseSet updateOnlyAllowedAfterDeviceTypeClauses{
     llvm::acc::Clause::ACCC_async, llvm::acc::Clause::ACCC_wait};
 
 static constexpr inline AccClauseSet routineOnlyAllowedAfterDeviceTypeClauses{
     llvm::acc::Clause::ACCC_bind, llvm::acc::Clause::ACCC_gang,
     llvm::acc::Clause::ACCC_vector, llvm::acc::Clause::ACCC_worker};
 
-class NoBranchingEnforce {
-public:
-  NoBranchingEnforce(SemanticsContext &context,
-      parser::CharBlock sourcePosition, llvm::acc::Directive directive)
-      : context_{context}, sourcePosition_{sourcePosition}, currentDirective_{
-                                                                directive} {}
-  template <typename T> bool Pre(const T &) { return true; }
-  template <typename T> void Post(const T &) {}
-
-  template <typename T> bool Pre(const parser::Statement<T> &statement) {
-    currentStatementSourcePosition_ = statement.source;
-    return true;
-  }
-
-  void Post(const parser::ReturnStmt &) { EmitBranchOutError("RETURN"); }
-  void Post(const parser::ExitStmt &exitStmt) {
-    if (const auto &exitName{exitStmt.v}) {
-      CheckConstructNameBranching("EXIT", exitName.value());
-    }
-  }
-  void Post(const parser::StopStmt &) { EmitBranchOutError("STOP"); }
-
-private:
-  parser::MessageFormattedText GetEnclosingMsg() const {
-    return {"Enclosing %s construct"_en_US,
-        parser::ToUpperCaseLetters(
-            llvm::acc::getOpenACCDirectiveName(currentDirective_).str())};
-  }
-
-  void EmitBranchOutError(const char *stmt) const {
-    context_
-        .Say(currentStatementSourcePosition_,
-            "%s statement is not allowed in a %s construct"_err_en_US, stmt,
-            parser::ToUpperCaseLetters(
-                llvm::acc::getOpenACCDirectiveName(currentDirective_).str()))
-        .Attach(sourcePosition_, GetEnclosingMsg());
-  }
-
-  void EmitBranchOutErrorWithName(
-      const char *stmt, const parser::Name &toName) const {
-    const std::string branchingToName{toName.ToString()};
-    const auto upperCaseConstructName{parser::ToUpperCaseLetters(
-        llvm::acc::getOpenACCDirectiveName(currentDirective_).str())};
-    context_
-        .Say(currentStatementSourcePosition_,
-            "%s to construct '%s' outside of %s construct is not allowed"_err_en_US,
-            stmt, branchingToName, upperCaseConstructName)
-        .Attach(sourcePosition_, GetEnclosingMsg());
-  }
-
-  // Current semantic checker is not following OpenACC constructs as they are
-  // not Fortran constructs. Hence the ConstructStack doesn't capture OpenACC
-  // constructs. Apply an inverse way to figure out if a construct-name is
-  // branching out of an OpenACC construct. The control flow goes out of an
-  // OpenACC construct, if a construct-name from statement is found in
-  // ConstructStack.
-  void CheckConstructNameBranching(
-      const char *stmt, const parser::Name &stmtName) {
-    const ConstructStack &stack{context_.constructStack()};
-    for (auto iter{stack.cend()}; iter-- != stack.cbegin();) {
-      const ConstructNode &construct{*iter};
-      const auto &constructName{MaybeGetNodeName(construct)};
-      if (constructName) {
-        if (stmtName.source == constructName->source) {
-          EmitBranchOutErrorWithName(stmt, stmtName);
-          return;
-        }
-      }
-    }
-  }
-
-  SemanticsContext &context_;
-  parser::CharBlock currentStatementSourcePosition_;
-  parser::CharBlock sourcePosition_;
-  llvm::acc::Directive currentDirective_;
-};
-
 void AccStructureChecker::Enter(const parser::AccClause &x) {
   SetContextClause(x);
 }
 
 void AccStructureChecker::Leave(const parser::AccClauseList &) {}
 
 void AccStructureChecker::Enter(const parser::OpenACCBlockConstruct &x) {
   const auto &beginBlockDir{std::get<parser::AccBeginBlockDirective>(x.t)};
void AccStructureChecker::Leave(const parser::OpenACCBlockConstruct &x) {
   case llvm::acc::Directive::ACCD_kernels:
   case llvm::acc::Directive::ACCD_parallel:
     // Restriction - 880-881 (KERNELS)
     // Restriction - 843-844 (PARALLEL)
     CheckOnlyAllowedAfter(llvm::acc::Clause::ACCC_device_type,
         parallelAndKernelsOnlyAllowedAfterDeviceTypeClauses);
     // Restriction - 877 (KERNELS)
     // Restriction - 840 (PARALLEL)
-    CheckNoBranching(block, GetContext().directive, blockDir.source);
+    DirectiveStructureChecker::CheckNoBranching(
+        block, GetContext().directive, blockDir.source);
     break;
   case llvm::acc::Directive::ACCD_serial:
     // Restriction - 919
     CheckOnlyAllowedAfter(llvm::acc::Clause::ACCC_device_type,
         serialOnlyAllowedAfterDeviceTypeClauses);
     // Restriction - 916
-    CheckNoBranching(block, llvm::acc::Directive::ACCD_serial, blockDir.source);
+    DirectiveStructureChecker::CheckNoBranching(
+        block, llvm::acc::Directive::ACCD_serial, blockDir.source);
     break;
   case llvm::acc::Directive::ACCD_data:
     // Restriction - 1117-1118
     CheckRequireAtLeastOneOf();
     break;
   case llvm::acc::Directive::ACCD_host_data:
     // Restriction - 1578
     CheckRequireAtLeastOneOf();
     break;
   default:
     break;
   }
   dirContext_.pop_back();
 }
 
-void AccStructureChecker::CheckNoBranching(const parser::Block &block,
-    const llvm::acc::Directive directive,
-    const parser::CharBlock &directiveSource) const {
-  NoBranchingEnforce noBranchingEnforce{context_, directiveSource, directive};
-  parser::Walk(block, noBranchingEnforce);
-}
-
 void AccStructureChecker::Enter(
     const parser::OpenACCStandaloneDeclarativeConstruct &x) {
   const auto &declarativeDir{std::get<parser::AccDeclarativeDirective>(x.t)};
   PushContextAndClauseSets(declarativeDir.source, declarativeDir.v);
 }
 
 void AccStructureChecker::Leave(
     const parser::OpenACCStandaloneDeclarativeConstruct &) {

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

 namespace Fortran::semantics {
 
 template <typename C, std::size_t ClauseEnumSize> struct DirectiveClauses {
   const common::EnumSet<C, ClauseEnumSize> allowed;
   const common::EnumSet<C, ClauseEnumSize> allowedOnce;
   const common::EnumSet<C, ClauseEnumSize> allowedExclusive;
   const common::EnumSet<C, ClauseEnumSize> requiredOneOf;
 };
+// Generic branching checker for invalid branching out of OpenMP/OpenACC
+// directive.
+// typename D is the directive enumeration.
+template <typename D> class NoBranchingEnforce {
+public:
+  NoBranchingEnforce(SemanticsContext &context,
+      parser::CharBlock sourcePosition, D directive,
+      std::string &&upperCaseDirName)
+      : context_{context}, sourcePosition_{sourcePosition},
+        currentDirective_{directive}, upperCaseDirName_{
+                                          std::move(upperCaseDirName)} {}
+  template <typename T> bool Pre(const T &) { return true; }
+  template <typename T> void Post(const T &) {}
+
+  template <typename T> bool Pre(const parser::Statement<T> &statement) {
+    currentStatementSourcePosition_ = statement.source;
+    return true;
+  }
+
+  void Post(const parser::ReturnStmt &) { EmitBranchOutError("RETURN"); }
+  void Post(const parser::ExitStmt &exitStmt) {
+    if (const auto &exitName{exitStmt.v}) {
+      CheckConstructNameBranching("EXIT", exitName.value());
+    }
+  }
+  void Post(const parser::StopStmt &) { EmitBranchOutError("STOP"); }
+
+private:
+  parser::MessageFormattedText GetEnclosingMsg() const {
+    return {"Enclosing %s construct"_en_US, upperCaseDirName_};
+  }
+
+  void EmitBranchOutError(const char *stmt) const {
+    context_
+        .Say(currentStatementSourcePosition_,
+            "%s statement is not allowed in a %s construct"_err_en_US, stmt,
+            upperCaseDirName_)
+        .Attach(sourcePosition_, GetEnclosingMsg());
+  }
+
+  void EmitBranchOutErrorWithName(
+      const char *stmt, const parser::Name &toName) const {
+    const std::string branchingToName{toName.ToString()};
+    context_
+        .Say(currentStatementSourcePosition_,
+            "%s to construct '%s' outside of %s construct is not allowed"_err_en_US,
+            stmt, branchingToName, upperCaseDirName_)
+        .Attach(sourcePosition_, GetEnclosingMsg());
+  }
+
+  // Current semantic checker is not following OpenACC/OpenMP constructs as they
+  // are not Fortran constructs. Hence the ConstructStack doesn't capture
+  // OpenACC/OpenMP constructs. Apply an inverse way to figure out if a
+  // construct-name is branching out of an OpenACC/OpenMP construct. The control
+  // flow goes out of an OpenACC/OpenMP construct, if a construct-name from
+  // statement is found in ConstructStack.
+  void CheckConstructNameBranching(
+      const char *stmt, const parser::Name &stmtName) {
+    const ConstructStack &stack{context_.constructStack()};
+    for (auto iter{stack.cend()}; iter-- != stack.cbegin();) {
+      const ConstructNode &construct{*iter};
+      const auto &constructName{MaybeGetNodeName(construct)};
+      if (constructName) {
+        if (stmtName.source == constructName->source) {
+          EmitBranchOutErrorWithName(stmt, stmtName);
+          return;
+        }
+      }
+    }
+  }
+
+  SemanticsContext &context_;
+  parser::CharBlock currentStatementSourcePosition_;
+  parser::CharBlock sourcePosition_;
+  std::string upperCaseDirName_;
+  D currentDirective_;
+};
 
 // Generic structure checker for directives/clauses language such as OpenMP
 // and OpenACC.
 // typename D is the directive enumeration.
 // tyepname C is the clause enumeration.
 // typename PC is the parser class defined in parse-tree.h for the clauses.
 template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
 class DirectiveStructureChecker : public virtual BaseChecker {
struct DirectiveContext {
 
     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;
     std::list<C> actualClauses;
   };
 
   // back() is the top of the stack
   DirectiveContext &GetContext() {
     CHECK(!dirContext_.empty());
protected:
 
   template <typename B> void CheckMatching(const B &beginDir, const B &endDir) {
     const auto &begin{beginDir.v};
     const auto &end{endDir.v};
     if (begin != end) {
       SayNotMatching(beginDir.source, endDir.source);
     }
   }
+  void CheckNoBranching(const parser::Block &block, D directive,
+      const parser::CharBlock &directiveSource);
 
   // Check that only clauses in set are after the specific clauses.
   void CheckOnlyAllowedAfter(C clause, common::EnumSet<C, ClauseEnumSize> set);
 
   void CheckRequired(C clause);
 
   void CheckRequireAtLeastOneOf();
 
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);
 };
 
+template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
+void DirectiveStructureChecker<D, C, PC, ClauseEnumSize>::CheckNoBranching(
+    const parser::Block &block, D directive,
+    const parser::CharBlock &directiveSource) {
+  NoBranchingEnforce<D> noBranchingEnforce{
+      context_, directiveSource, directive, ContextDirectiveAsFortran()};
+  parser::Walk(block, noBranchingEnforce);
+}
+
 // 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

void OmpStructureChecker::Enter(const parser::OmpEndLoopDirective &x) {
   }
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
   const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(x.t)};
   const auto &endBlockDir{std::get<parser::OmpEndBlockDirective>(x.t)};
   const auto &beginDir{std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
   const auto &endDir{std::get<parser::OmpBlockDirective>(endBlockDir.t)};
+  const parser::Block &block{std::get<parser::Block>(x.t)};
+
   CheckMatching<parser::OmpBlockDirective>(beginDir, endDir);
 
   PushContextAndClauseSets(beginDir.source, beginDir.v);
+
+  switch (beginDir.v) {
+  case llvm::omp::OMPD_parallel:
+    DirectiveStructureChecker::CheckNoBranching(
+        block, llvm::omp::OMPD_parallel, beginDir.source);
+  default:
+    break;
+  }
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPBlockConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPSectionsConstruct &x) {
   const auto &beginSectionsDir{

flang/test/Semantics/omp-clause-validity01.f90

!                        allocate-clause
   !$omp end parallel
 
   !$omp parallel
   do i = 1, N
   enddo
   !ERROR: Unmatched END TARGET directive
   !$omp end target
 
+  ! OMP 5.0 - 2.6 Restriction point 1
+  outofparallel: do k =1, 10
+  !$omp parallel
+    !$omp do
+    outer: do i=0, 10
+      inner: do j=1, 10
+        exit
+        exit outer
+        !ERROR: EXIT to construct 'outofparallel' outside of PARALLEL construct is not allowed
+        exit outofparallel
+      end do inner
+    end do outer
+    !$end omp do
+  !$omp end parallel
+  end do outofparallel
+
 ! 2.7.1  do-clause -> private-clause |
 !                     firstprivate-clause |
 !                     lastprivate-clause |
 !                     linear-clause |
 !                     reduction-clause |
 !                     schedule-clause |
 !                     collapse-clause |
 !                     ordered-clause