[OpenMP] Patch for Support to loop bind clause : Checking Parent Region

[koops]

Summary

https://github.com/llvm/llvm-project/issues/64728
In https://reviews.llvm.org/D144634 : Support for Code Generation of loop bind clause,
before mapping a directive to the new directive there is a need to check if the parent directive/region is proper for the new mapped directive. e.g.
#pragma omp parallel for
for () {

#pragma omp loop bind(parallel)
for() {}

}.

After mapping "omp loop bind(parallel)" to "omp for" the above example becomes:
#pragma omp parallel for
for () {

#pragma omp for
for() {}

}.
However, it is illegal to have a "for" region within another "for" region. Thanks to David Pagan for pointing this out.

[ABataev]

clang/include/clang/Basic/OpenMPKinds.h

251 ↗

(On Diff #551446)

What if the outer regioun is sections?

[ABataev]

Add tests for the nesting of regions to check all possible combinations correctly.

[koops]

Using isOpenMPWorksharingDirective( ) for the "omp loop bind(parallel)" and "omp loop bind(teams)".
Added extra tests in loop_bind_messages.cpp.

[koops]

Can someone please review the changes that I uploaded last week?

[ABataev]

In D158266#4640416, @koops wrote:

Can someone please review the changes that I uploaded last week?

Still need to add extra tests to the nesting_of_regions.cpp test

[koops]

Adding extra test cases to loop_bind_messages.cpp.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

6181–6197

Can you move these checks to checkNestingOfRegions() function?

[koops]

Moving checks of Work Sharing Directive in mapLoopConstruct to checkNestingOfRegions().

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

6229

Remove this new line

6251–6252

Can we do this before checks in lines 6232-6235 so we can avoid double call of checkNestingOfRegions for bind?

[koops]

Avoiding executing "checkNestingOfRegions( )" multiple times.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

6181–6190

Need to drop these extra calls of checkNestingOfRegions(), all check must be handled in the single call. Can we do it?

[koops]

clang/lib/Sema/SemaOpenMP.cpp

6181–6190

1. Default binding in "#pragma omp loop" is handled in the beginning of mapLoopConstruct.

#pragma omp parallel
....
#pragma omp loop
...
indicates "omp loop" will be "omp loop bind(parallel)". So, I cannot call checkNestingOfRegions() before call to mapLoopConstruct()

2. In mapLoopConstruct the directive/construct of "omp loop" is changed to other directives like "omp for". So, it is not possible to call checkNestingOfRegions() after the call to mapLoopConstruct.

Because of the above 2 reasons the current code seems correct. Please let me know if you have any alternative in mind.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

6181–6190

Can you perform mapLoopConstruct earlier, before the very first checkNestingOfRegions()? Current code structure ovrcomplicates the code, makes it much harder to understand and to maintain

[koops]

Removing multiple calls to Sema::checkNestingOfRegions() in Sema::ActOnOpenMPExecutableDirective() .

[ABataev]

LG

[koops]

Can someone please backout the changes done with this commit? It is resulting in build failures on debian.

[ddpagan]

Did anything change after the patch was reverted?

[koops]

In D158266#4655433, @ddpagan wrote:

Did anything change after the patch was reverted?

I have not changed anything in the code yet. I am still investigating. The test where it is failing has nothing to do with my changes. I do not understand the failure presently.

[ddpagan]

Ok. Thanks for the clarification.

[ddpagan]

Is there a github PR associated with this? If yes, what is the PR? If not, one should be created that includes a fix for the teams loop/loop error issue. Please add me as a reviewer. Thanks.

clang/include/clang/Sema/Sema.h

private:
 
   /// All `omp assumes` we encountered so far.
   SmallVector<AssumptionAttr *, 4> OMPAssumeGlobal;
 
   /// OMPD_loop is mapped to OMPD_for, OMPD_distribute or OMPD_simd depending
   /// on the parameter of the bind clause. In the methods for the
   /// mapped directives, check the parameters of the lastprivate clause.
   bool checkLastPrivateForMappedDirectives(ArrayRef<OMPClause *> Clauses);
+
   /// Depending on the bind clause of OMPD_loop map the directive to new
   /// directives.
   ///    1) loop bind(parallel) --> OMPD_for
   ///    2) loop bind(teams) --> OMPD_distribute
   ///    3) loop bind(thread) --> OMPD_simd
   /// This is being handled in Sema instead of Codegen because of the need for
   /// rigorous semantic checking in the new mapped directives.
   bool mapLoopConstruct(llvm::SmallVector<OMPClause *> &ClausesWithoutBind,
                         ArrayRef<OMPClause *> Clauses,
-                        OpenMPBindClauseKind BindKind,
+                        OpenMPBindClauseKind &BindKind,
                         OpenMPDirectiveKind &Kind,
-                        OpenMPDirectiveKind &PrevMappedDirective);
+                        OpenMPDirectiveKind &PrevMappedDirective,
+                        SourceLocation StartLoc, SourceLocation EndLoc,
+                        const DeclarationNameInfo &DirName,
+                        OpenMPDirectiveKind CancelRegion);
 
 public:
   /// The declarator \p D defines a function in the scope \p S which is nested
   /// in an `omp begin/end declare variant` scope. In this method we create a
   /// declaration for \p D and rename \p D according to the OpenMP context
   /// selector of the surrounding scope. Return all base functions in \p Bases.
   void ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope(
       Scope *S, Declarator &D, MultiTemplateParamsArg TemplateParameterLists,

clang/lib/Sema/SemaOpenMP.cpp

if (Stack->getCurScope()) {
     // Allow some constructs (except teams and cancellation constructs) to be
     // orphaned (they could be used in functions, called from OpenMP regions
     // with the required preconditions).
     if (ParentRegion == OMPD_unknown &&
         !isOpenMPNestingTeamsDirective(CurrentRegion) &&
         CurrentRegion != OMPD_cancellation_point &&
         CurrentRegion != OMPD_cancel && CurrentRegion != OMPD_scan)
       return false;
+    // Checks needed for mapping "loop" construct. Please check mapLoopConstruct
+    // for a detailed explanation
+    if (SemaRef.LangOpts.OpenMP >= 50 && CurrentRegion == OMPD_loop &&
+        ((BindKind == OMPC_BIND_parallel) || (BindKind == OMPC_BIND_teams)) &&
+        (isOpenMPWorksharingDirective(ParentRegion) ||
+         ParentRegion == OMPD_loop)) {
+      int ErrorMsgNumber = (BindKind == OMPC_BIND_parallel) ? 1 : 4;
+      SemaRef.Diag(StartLoc, diag::err_omp_prohibited_region)
+          << true << getOpenMPDirectiveName(ParentRegion) << ErrorMsgNumber
+          << getOpenMPDirectiveName(CurrentRegion);
+      return true;
+    }
     if (CurrentRegion == OMPD_cancellation_point ||
         CurrentRegion == OMPD_cancel) {
       // OpenMP [2.16, Nesting of Regions]
       // A cancellation point construct for which construct-type-clause is
       // taskgroup must be nested inside a task construct. A cancellation
       // point construct for which construct-type-clause is not taskgroup must
       // be closely nested inside an OpenMP construct that matches the type
       // specified in construct-type-clause.
if (OMPClause *NewClause = S.ActOnOpenMPMapClause(
             /*IsMapTypeImplicit=*/true, SourceLocation(), SourceLocation(),
             SubExprs, OMPVarListLocTy()))
       Clauses.push_back(NewClause);
   }
 }
 
 bool Sema::mapLoopConstruct(llvm::SmallVector<OMPClause *> &ClausesWithoutBind,
                             ArrayRef<OMPClause *> Clauses,
-                            OpenMPBindClauseKind BindKind,
+                            OpenMPBindClauseKind &BindKind,
                             OpenMPDirectiveKind &Kind,
-                            OpenMPDirectiveKind &PrevMappedDirective) {
+                            OpenMPDirectiveKind &PrevMappedDirective,
+                            SourceLocation StartLoc, SourceLocation EndLoc,
+                            const DeclarationNameInfo &DirName,
+                            OpenMPDirectiveKind CancelRegion) {
 
   bool UseClausesWithoutBind = false;
 
   // Restricting to "#pragma omp loop bind"
   if (getLangOpts().OpenMP >= 50 && Kind == OMPD_loop) {
+
+    const OpenMPDirectiveKind ParentDirective = DSAStack->getParentDirective();
+
     if (BindKind == OMPC_BIND_unknown) {
       // Setting the enclosing teams or parallel construct for the loop
       // directive without bind clause.
       BindKind = OMPC_BIND_thread; // Default bind(thread) if binding is unknown
 
-      const OpenMPDirectiveKind ParentDirective =
-          DSAStack->getParentDirective();
       if (ParentDirective == OMPD_unknown) {
         Diag(DSAStack->getDefaultDSALocation(),
              diag::err_omp_bind_required_on_loop);
-      } else if (ParentDirective == OMPD_parallel ||
-                 ParentDirective == OMPD_target_parallel) {
+      } else if (isOpenMPParallelDirective(ParentDirective) &&
+                 !isOpenMPTeamsDirective(ParentDirective)) {
         BindKind = OMPC_BIND_parallel;
-      } else if (ParentDirective == OMPD_teams ||
-                 ParentDirective == OMPD_target_teams) {
+      } else if (isOpenMPNestingTeamsDirective(ParentDirective) ||
+                 (ParentDirective == OMPD_target_teams)) {
         BindKind = OMPC_BIND_teams;
       }
     } else {
-      // bind clause is present, so we should set flag indicating to only
-      // use the clauses that aren't the bind clause for the new directive that
-      // loop is lowered to.
+      // bind clause is present in loop directive. When the loop directive is
+      // changed to a new directive the bind clause is not used. So, we should
+      // set flag indicating to only use the clauses that aren't the
+      // bind clause.
       UseClausesWithoutBind = true;
     }
 
     for (OMPClause *C : Clauses) {
       // Spec restriction : bind(teams) and reduction not permitted.
       if (BindKind == OMPC_BIND_teams &&
           C->getClauseKind() == llvm::omp::Clause::OMPC_reduction)
         Diag(DSAStack->getDefaultDSALocation(),
              diag::err_omp_loop_reduction_clause);
 
       // A new Vector ClausesWithoutBind, which does not contain the bind
       // clause, for passing to new directive.
       if (C->getClauseKind() != llvm::omp::Clause::OMPC_bind)
         ClausesWithoutBind.push_back(C);
     }
 
     switch (BindKind) {
     case OMPC_BIND_parallel:
       Kind = OMPD_for;
       DSAStack->setCurrentDirective(OMPD_for);
       DSAStack->setMappedDirective(OMPD_loop);
       PrevMappedDirective = OMPD_loop;
       break;
     case OMPC_BIND_teams:
       Kind = OMPD_distribute;
       DSAStack->setCurrentDirective(OMPD_distribute);
       DSAStack->setMappedDirective(OMPD_loop);
       PrevMappedDirective = OMPD_loop;
       break;
     case OMPC_BIND_thread:
       Kind = OMPD_simd;
       DSAStack->setCurrentDirective(OMPD_simd);
       DSAStack->setMappedDirective(OMPD_loop);
       PrevMappedDirective = OMPD_loop;
       break;
     case OMPC_BIND_unknown:
       break;
     }
   } else if (PrevMappedDirective == OMPD_loop) {
     /// An initial pass after recognizing all the statements is done in the
     /// Parser when the directive OMPD_loop is mapped to OMPD_for,
     /// OMPD_distribute or OMPD_simd. A second transform pass with call from
     /// clang::TreeTransform::TransformOMPExecutableDirective() is done

 
 StmtResult Sema::ActOnOpenMPExecutableDirective(
     OpenMPDirectiveKind Kind, const DeclarationNameInfo &DirName,
     OpenMPDirectiveKind CancelRegion, ArrayRef<OMPClause *> Clauses,
     Stmt *AStmt, SourceLocation StartLoc, SourceLocation EndLoc,
     OpenMPDirectiveKind PrevMappedDirective) {
   StmtResult Res = StmtError();
   OpenMPBindClauseKind BindKind = OMPC_BIND_unknown;
+  llvm::SmallVector<OMPClause *> ClausesWithoutBind;
+  bool UseClausesWithoutBind = false;
+
   if (const OMPBindClause *BC =
           OMPExecutableDirective::getSingleClause<OMPBindClause>(Clauses))
     BindKind = BC->getBindKind();
+
+  // Variable used to note down the DirectiveKind because mapLoopConstruct may
+  // change "Kind" variable, due to mapping of "omp loop" to other directives.
+  OpenMPDirectiveKind DK = Kind;
+  if ((Kind == OMPD_loop) || (PrevMappedDirective == OMPD_loop)) {
+    UseClausesWithoutBind = mapLoopConstruct(
+        ClausesWithoutBind, Clauses, BindKind, Kind, PrevMappedDirective,
+        StartLoc, EndLoc, DirName, CancelRegion);
+    DK = OMPD_loop;
+  }
+
   // First check CancelRegion which is then used in checkNestingOfRegions.
   if (checkCancelRegion(*this, Kind, CancelRegion, StartLoc) ||
-      checkNestingOfRegions(*this, DSAStack, Kind, DirName, CancelRegion,
-                            BindKind, StartLoc))
+      checkNestingOfRegions(*this, DSAStack, DK, DirName, CancelRegion,
+                            BindKind, StartLoc)) {
     return StmtError();
+  }
 
   // Report affected OpenMP target offloading behavior when in HIP lang-mode.
   if (getLangOpts().HIP && (isOpenMPTargetExecutionDirective(Kind) ||
                             isOpenMPTargetDataManagementDirective(Kind)))
     Diag(StartLoc, diag::warn_hip_omp_target_directives);
 
-  llvm::SmallVector<OMPClause *> ClausesWithoutBind;
-  bool UseClausesWithoutBind = false;
-
-  UseClausesWithoutBind = mapLoopConstruct(ClausesWithoutBind, Clauses,
-                                           BindKind, Kind, PrevMappedDirective);
-
   llvm::SmallVector<OMPClause *, 8> ClausesWithImplicit;
   VarsWithInheritedDSAType VarsWithInheritedDSA;
   bool ErrorFound = false;
   if (getLangOpts().OpenMP >= 50 && UseClausesWithoutBind) {
     ClausesWithImplicit.append(ClausesWithoutBind.begin(),
                                ClausesWithoutBind.end());
   } else {
     ClausesWithImplicit.append(Clauses.begin(), Clauses.end());
   }

clang/test/OpenMP/loop_bind_messages.cpp

 #ifndef HEADER
 #define HEADER
 // RUN: %clang_cc1 -fopenmp -fopenmp-version=50 -verify %s
 
 #define NNN 50
 int aaa[NNN];
+int aaa2[NNN][NNN];
 
 void parallel_loop() {
   #pragma omp parallel
   {
      #pragma omp loop
      for (int j = 0 ; j < NNN ; j++) {
        aaa[j] = j*NNN;
      }
    }
 }
 
+void parallel_for_AND_loop_bind() {
+  #pragma omp parallel for
+  for (int i = 0 ; i < NNN ; i++) {
+    #pragma omp loop bind(parallel) // expected-error{{region cannot be closely nested inside 'parallel for' region; perhaps you forget to enclose 'omp loop' directive into a parallel region?}}
+    for (int j = 0 ; j < NNN ; j++) {
+      aaa2[i][j] = i+j;
+    }
+  }
+}
+
+void parallel_nowait() {
+  #pragma omp parallel
+  #pragma omp for nowait
+  for (int i = 0 ; i < NNN ; i++) {
+    #pragma omp loop bind(parallel) // expected-error{{region cannot be closely nested inside 'for' region; perhaps you forget to enclose 'omp loop' directive into a parallel region?}}
+    for (int j = 0 ; j < NNN ; j++) {
+      aaa2[i][j] = i+j;
+    }
+  }
+}
+
+void parallel_for_with_nothing() {
+  #pragma omp parallel for
+  for (int i = 0 ; i < NNN ; i++) {
+    #pragma omp nothing
+    #pragma omp loop // expected-error{{region cannot be closely nested inside 'parallel for' region; perhaps you forget to enclose 'omp loop' directive into a parallel region?}}
+    for (int j = 0 ; j < NNN ; j++) {
+      aaa2[i][j] = i+j;
+    }
+  }
+}
+
+void parallel_targetfor_with_loop_bind() {
+  #pragma omp target teams distribute parallel for 
+  for (int i = 0 ; i < NNN ; i++) {
+    #pragma omp loop bind(parallel) // expected-error{{region cannot be closely nested inside 'target teams distribute parallel for' region; perhaps you forget to enclose 'omp loop' directive into a parallel region?}}
+    for (int j = 0 ; j < NNN ; j++) {
+      aaa2[i][j] = i+j;
+    }
+  }
+}
+
+void parallel_targetparallel_with_loop() {
+  #pragma omp target parallel
+  for (int i = 0 ; i < NNN ; i++) {
+    #pragma omp loop bind(parallel)
+    for (int j = 0 ; j < NNN ; j++) {
+      aaa2[i][j] = i+j;
+    }
+  }
+}
+
+void loop_bind_AND_loop_bind() {
+  #pragma omp parallel for
+  for (int i = 0; i < 100; ++i) {
+    #pragma omp loop bind(parallel) // expected-error{{region cannot be closely nested inside 'parallel for' region; perhaps you forget to enclose 'omp loop' directive into a parallel region?}} 
+    for (int i = 0 ; i < NNN ; i++) {
+      #pragma omp loop bind(parallel) // expected-error{{region cannot be closely nested inside 'loop' region; perhaps you forget to enclose 'omp loop' directive into a parallel region?}} 
+      for (int j = 0 ; j < NNN ; j++) {
+        aaa[j] = j*NNN;
+      }
+    }
+  }
+}
+
+void parallel_with_sections_loop() {
+  #pragma omp parallel
+  {
+     #pragma omp sections
+     {
+        for (int i = 0 ; i < NNN ; i++) {
+          #pragma omp loop bind(parallel) // expected-error{{region cannot be closely nested inside 'sections' region; perhaps you forget to enclose 'omp loop' directive into a parallel region?}}
+          for (int j = 0 ; j < NNN ; j++) {
+            aaa2[i][j] = i+j;
+          }
+        }
+
+        #pragma omp section
+  {
+          aaa[NNN-1] = NNN;
+        }
+     }
+  }
+}
+
 void teams_loop() {
   int var1, var2;
 
   #pragma omp teams
   {
      #pragma omp loop bind(teams)
      for (int j = 0 ; j < NNN ; j++) {
        aaa[j] = j*NNN;
      }
 
      #pragma omp loop bind(teams) collapse(2) private(var1)
      for (int i = 0 ; i < 3 ; i++) {
        for (int j = 0 ; j < NNN ; j++) {
          var1 += aaa[j];
        }
      }
    }
 }
 
-void orphan_loop_with_bind() {
-  #pragma omp loop bind(parallel) 
+void teams_targetteams_with_loop() {
+  #pragma omp target teams
+  for (int i = 0 ; i < NNN ; i++) {
+    #pragma omp loop bind(teams)
     for (int j = 0 ; j < NNN ; j++) {
-    aaa[j] = j*NNN;
+      aaa2[i][j] = i+j;
+    }
   }
 }
 
-void orphan_loop_no_bind() {
-  #pragma omp loop  // expected-error{{expected 'bind' clause for 'loop' construct without an enclosing OpenMP construct}}
+void teams_targetfor_with_loop_bind() {
+  #pragma omp target teams distribute parallel for 
+  for (int i = 0 ; i < NNN ; i++) {
+    #pragma omp loop bind(teams) // expected-error{{region cannot be closely nested inside 'target teams distribute parallel for' region; perhaps you forget to enclose 'omp loop' directive into a teams region?}}
     for (int j = 0 ; j < NNN ; j++) {
-    aaa[j] = j*NNN;
+      aaa2[i][j] = i+j;
+    }
   }
 }
 
 void teams_loop_reduction() {
   int total = 0;
 
   #pragma omp teams
   {
      #pragma omp loop bind(teams)
      for (int j = 0 ; j < NNN ; j++) {
        aaa[j] = j*NNN;
      }
 
      #pragma omp loop bind(teams) reduction(+:total) // expected-error{{'reduction' clause not allowed with '#pragma omp loop bind(teams)'}}
      for (int j = 0 ; j < NNN ; j++) {
        total+=aaa[j];
      }
    }
 }
 
+void teams_loop_distribute() {
+  int total = 0;
+
+  #pragma omp teams num_teams(8) thread_limit(256)
+  #pragma omp distribute parallel for dist_schedule(static, 1024) \
+                                      schedule(static, 64)
+  for (int i = 0; i < NNN; i++) {
+    #pragma omp loop bind(teams) // expected-error{{'distribute parallel for' region; perhaps you forget to enclose 'omp loop' directive into a teams region?}}
+    for (int j = 0; j < NNN; j++) {
+      aaa2[i][j] = i+j;
+    }
+  }
+}
+
+void parallel_for_with_loop_teams_bind(){
+  #pragma omp parallel for
+  for (int i = 0; i < NNN; i++) {
+    #pragma omp loop bind(teams) // expected-error{{region cannot be closely nested inside 'parallel for' region; perhaps you forget to enclose 'omp loop' directive into a teams region?}}
+    for (int j = 0 ; j < NNN ; j++) {
+      aaa[i] = i+i*NNN;
+    }
+  }
+}
+
+void teams_with_loop_thread_bind(){
+  #pragma omp teams
+  for (int i = 0; i < NNN; i++) {
+    #pragma omp loop bind(thread)
+    for (int j = 0 ; j < NNN ; j++) {
+      aaa[i] = i+i*NNN;
+    }
+  }
+}
+
+void orphan_loop_no_bind() {
+  #pragma omp loop  // expected-error{{expected 'bind' clause for 'loop' construct without an enclosing OpenMP construct}}
+  for (int j = 0 ; j < NNN ; j++) {
+    aaa[j] = j*NNN;
+  }
+}
+
+void orphan_loop_parallel_bind() {
+  #pragma omp loop bind(parallel) 
+  for (int j = 0 ; j < NNN ; j++) {
+    aaa[j] = j*NNN;
+  }
+}
+
+void orphan_loop_teams_bind(){
+  #pragma omp loop bind(teams)
+  for (int i = 0; i < NNN; i++) {
+    aaa[i] = i+i*NNN;
+  }
+}
+
 int main(int argc, char *argv[]) {
   parallel_loop();
+  parallel_for_AND_loop_bind();
+  parallel_nowait();
+  parallel_for_with_nothing();
+  parallel_targetfor_with_loop_bind();
+  parallel_targetparallel_with_loop();
+  loop_bind_AND_loop_bind();
+  parallel_with_sections_loop();
   teams_loop();
-  orphan_loop_with_bind();
-  orphan_loop_no_bind();
+  teams_targetteams_with_loop();
+  teams_targetfor_with_loop_bind();
   teams_loop_reduction();
+  teams_loop_distribute();
+  parallel_for_with_loop_teams_bind();
+  teams_with_loop_thread_bind();
+  orphan_loop_no_bind();
+  orphan_loop_parallel_bind();
+  orphan_loop_teams_bind();
 }
 
 #endif

clang/test/PCH/pragma-loop.cpp

 #pragma omp loop bind(parallel)
     for (int i = 0; i < Length; i++) {
       List[i] = i;
     }
   }
 
   inline void run10(int *List, int Length) {
     int i = 0;
-#pragma omp loop bind(teams)
+    int j = 0;
+    #pragma omp teams
     for (int i = 0; i < Length; i++) {
-      List[i] = i;
+      #pragma omp loop bind(teams)
+      for (int j = 0; j < Length; j++) {
+        List[i] = i+j;
+      }
     }
   }
 
 };
 #else
 void test() {
   int List[100];