[OpenMP] Generate better diagnostics for cancel and cancellation point

[Hahnfeld]

Summary

checkNestingOfRegions uses CancelRegion to determine whether cancel and
cancellation point are valid in the given nesting. This leads to unuseful
diagnostics if CancelRegion is invalid. The given test case has produced:

region cannot be closely nested inside 'parallel' region

As a solution, introduce checkCancelRegion and call it first to get the
expected error:

one of 'for', 'parallel', 'sections' or 'taskgroup' is expected

[ABataev]

Not sure that this is better because at first, we need to be sure that this nesting is allowed. Why do we need to perform some additional analysis if nesting is not allowed at all?

[Hahnfeld]

In D30135#681354, @ABataev wrote:

Not sure that this is better because at first, we need to be sure that this nesting is allowed. Why do we need to perform some additional analysis if nesting is not allowed at all?

CheckNestingOfRegions uses CancelRegion to determine whether cancel and cancellation point may be nested inside the parent construct. However with the current code, CancelRegion would only be checked afterwards.

#pragma omp parallel
  {
#pragma omp cancellation point unknown
  }

therefore produces region cannot be closely nested inside 'parallel' region. After this change, it says one of 'for', 'parallel', 'sections' or 'taskgroup' is expected as in the test case which is better IMO.

Should I try to improve the summary to explain that?

[ABataev]

I see. I think it is better to check the CancelRegion just before call of CheckNestingOfRegions() function. You need to extract checks for CancelRegion from ActOnOpenMPCancellationPointDirective() and ActOnOpenMPCancelDirective() functions into a standalone function and call it before CheckNestingOfRegions().

[Hahnfeld]

new static function CheckCancelRegion

[ABataev]

lib/Sema/SemaOpenMP.cpp

1959 ↗

(On Diff #89099)

Should be checkCancelRegion

1964–1966 ↗

(On Diff #89099)

No need for braces

1968–1973 ↗

(On Diff #89099)

It is better to convert this to return false and make error message and return true statement unconditional

[Hahnfeld]

lib/Sema/SemaOpenMP.cpp

1959 ↗

(On Diff #89099)

It's also CheckNestingOfRegions, but can do

1968–1973 ↗

(On Diff #89099)

I wanted to keep the style in CheckNestingOfRegions: That way it's easier to add more checks later on if needed like additional restrictions on CancelRegion

[ABataev]

lib/Sema/SemaOpenMP.cpp

1959 ↗

(On Diff #89099)

Old formatting, this should be fixed

1968–1973 ↗

(On Diff #89099)

I just meant that it's better to make it look like this:

if (CancelRegion == OMPD_parallel || CancelRegion == OMPD_for ||
    CancelRegion == OMPD_sections || CancelRegion == OMPD_taskgroup)
  return false;

SemaRef.Diag(StartLoc, diag::err_omp_wrong_cancel_region)
  << getOpenMPDirectiveName(CancelRegion);
return true;

[Hahnfeld]

lib/Sema/SemaOpenMP.cpp

1968–1973 ↗

(On Diff #89099)

I understood what you want it to look like. However, that style makes it impossible to add additional diagnostics to this function

[ABataev]

lib/Sema/SemaOpenMP.cpp

1968–1973 ↗

(On Diff #89099)

Let's think about it later, if(!) some changes will be required

[Hahnfeld]

Address review comment's and apply new naming style to checkNestingOfRegions

[ABataev]

LG

cfe/trunk/lib/Sema/SemaOpenMP.cpp

if (isParallelOrTaskRegion(DSAStack->getCurrentDirective())) {
         }
       }
     }
     SR = ActOnCapturedRegionEnd(SR.get());
   }
   return SR;
 }
 
-static bool CheckNestingOfRegions(Sema &SemaRef, DSAStackTy *Stack,
+static bool checkCancelRegion(Sema &SemaRef, OpenMPDirectiveKind CurrentRegion,
+                              OpenMPDirectiveKind CancelRegion,
+                              SourceLocation StartLoc) {
+  // CancelRegion is only needed for cancel and cancellation_point.
+  if (CurrentRegion != OMPD_cancel && CurrentRegion != OMPD_cancellation_point)
+    return false;
+
+  if (CancelRegion == OMPD_parallel || CancelRegion == OMPD_for ||
+      CancelRegion == OMPD_sections || CancelRegion == OMPD_taskgroup)
+    return false;
+
+  SemaRef.Diag(StartLoc, diag::err_omp_wrong_cancel_region)
+      << getOpenMPDirectiveName(CancelRegion);
+  return true;
+}
+
+static bool checkNestingOfRegions(Sema &SemaRef, DSAStackTy *Stack,
                                   OpenMPDirectiveKind CurrentRegion,
                                   const DeclarationNameInfo &CurrentName,
                                   OpenMPDirectiveKind CancelRegion,
                                   SourceLocation StartLoc) {
   if (Stack->getCurScope()) {
     auto ParentRegion = Stack->getParentDirective();
     auto OffendingRegion = ParentRegion;
     bool NestingProhibited = false;
static bool checkIfClauses(Sema &S, OpenMPDirectiveKind Kind,
   return ErrorFound;
 }
 
 StmtResult Sema::ActOnOpenMPExecutableDirective(
     OpenMPDirectiveKind Kind, const DeclarationNameInfo &DirName,
     OpenMPDirectiveKind CancelRegion, ArrayRef<OMPClause *> Clauses,
     Stmt *AStmt, SourceLocation StartLoc, SourceLocation EndLoc) {
   StmtResult Res = StmtError();
-  if (CheckNestingOfRegions(*this, DSAStack, Kind, DirName, CancelRegion,
+  // First check CancelRegion which is then used in checkNestingOfRegions.
+  if (checkCancelRegion(*this, Kind, CancelRegion, StartLoc) ||
+      checkNestingOfRegions(*this, DSAStack, Kind, DirName, CancelRegion,
                             StartLoc))
     return StmtError();
 
   llvm::SmallVector<OMPClause *, 8> ClausesWithImplicit;
   llvm::DenseMap<ValueDecl *, Expr *> VarsWithInheritedDSA;
   bool ErrorFound = false;
   ClausesWithImplicit.append(Clauses.begin(), Clauses.end());
   if (AStmt) {
StmtResult Sema::ActOnOpenMPTeamsDirective(ArrayRef<OMPClause *> Clauses,
 
   return OMPTeamsDirective::Create(Context, StartLoc, EndLoc, Clauses, AStmt);
 }
 
 StmtResult
 Sema::ActOnOpenMPCancellationPointDirective(SourceLocation StartLoc,
                                             SourceLocation EndLoc,
                                             OpenMPDirectiveKind CancelRegion) {
-  if (CancelRegion != OMPD_parallel && CancelRegion != OMPD_for &&
-      CancelRegion != OMPD_sections && CancelRegion != OMPD_taskgroup) {
-    Diag(StartLoc, diag::err_omp_wrong_cancel_region)
-        << getOpenMPDirectiveName(CancelRegion);
-    return StmtError();
-  }
   if (DSAStack->isParentNowaitRegion()) {
     Diag(StartLoc, diag::err_omp_parent_cancel_region_nowait) << 0;
     return StmtError();
   }
   if (DSAStack->isParentOrderedRegion()) {
     Diag(StartLoc, diag::err_omp_parent_cancel_region_ordered) << 0;
     return StmtError();
   }
   return OMPCancellationPointDirective::Create(Context, StartLoc, EndLoc,
                                                CancelRegion);
 }
 
 StmtResult Sema::ActOnOpenMPCancelDirective(ArrayRef<OMPClause *> Clauses,
                                             SourceLocation StartLoc,
                                             SourceLocation EndLoc,
                                             OpenMPDirectiveKind CancelRegion) {
-  if (CancelRegion != OMPD_parallel && CancelRegion != OMPD_for &&
-      CancelRegion != OMPD_sections && CancelRegion != OMPD_taskgroup) {
-    Diag(StartLoc, diag::err_omp_wrong_cancel_region)
-        << getOpenMPDirectiveName(CancelRegion);
-    return StmtError();
-  }
   if (DSAStack->isParentNowaitRegion()) {
     Diag(StartLoc, diag::err_omp_parent_cancel_region_nowait) << 1;
     return StmtError();
   }
   if (DSAStack->isParentOrderedRegion()) {
     Diag(StartLoc, diag::err_omp_parent_cancel_region_ordered) << 1;
     return StmtError();
   }

cfe/trunk/test/OpenMP/cancel_messages.cpp

 // RUN: %clang_cc1 -verify -fopenmp -ferror-limit 100 %s
 
 int main(int argc, char **argv) {
 #pragma omp cancellation       // expected-error {{expected an OpenMP directive}}
 #pragma omp cancel // expected-error {{one of 'for', 'parallel', 'sections' or 'taskgroup' is expected}}
   ;
+#pragma omp parallel
+  {
+#pragma omp cancel // expected-error {{one of 'for', 'parallel', 'sections' or 'taskgroup' is expected}}
+  }
 #pragma omp cancel parallel untied // expected-error {{unexpected OpenMP clause 'untied' in directive '#pragma omp cancel'}}
 #pragma omp cancel unknown         // expected-error {{one of 'for', 'parallel', 'sections' or 'taskgroup' is expected}}
+#pragma omp parallel
+  {
+#pragma omp cancel unknown         // expected-error {{one of 'for', 'parallel', 'sections' or 'taskgroup' is expected}}
+  }
 #pragma omp cancel sections(       // expected-warning {{extra tokens at the end of '#pragma omp cancel' are ignored}}
 #pragma omp cancel for, )          // expected-warning {{extra tokens at the end of '#pragma omp cancel' are ignored}}
 #pragma omp cancel taskgroup()     // expected-warning {{extra tokens at the end of '#pragma omp cancel' are ignored}}
 #pragma omp cancel parallel, if    // expected-warning {{extra tokens at the end of '#pragma omp cancel' are ignored}}
   if (argc)
 #pragma omp cancel for // expected-error {{'#pragma omp cancel' cannot be an immediate substatement}}
     if (argc) {
 #pragma omp taskgroup

cfe/trunk/test/OpenMP/cancellation_point_messages.cpp

 // RUN: %clang_cc1 -verify -fopenmp -ferror-limit 100 %s
 
 int main(int argc, char **argv) {
 #pragma omp cancellation       // expected-error {{expected an OpenMP directive}}
 #pragma omp cancellation point // expected-error {{one of 'for', 'parallel', 'sections' or 'taskgroup' is expected}}
   ;
+#pragma omp parallel
+  {
+#pragma omp cancellation point // expected-error {{one of 'for', 'parallel', 'sections' or 'taskgroup' is expected}}
+  }
 #pragma omp cancellation point parallel untied // expected-error {{unexpected OpenMP clause 'untied' in directive '#pragma omp cancellation point'}}
 #pragma omp cancellation point unknown         // expected-error {{one of 'for', 'parallel', 'sections' or 'taskgroup' is expected}}
+#pragma omp parallel
+  {
+#pragma omp cancellation point unknown         // expected-error {{one of 'for', 'parallel', 'sections' or 'taskgroup' is expected}}
+  }
 #pragma omp cancellation point sections(       // expected-warning {{extra tokens at the end of '#pragma omp cancellation point' are ignored}}
 #pragma omp cancellation point for, )          // expected-warning {{extra tokens at the end of '#pragma omp cancellation point' are ignored}}
 #pragma omp cancellation point taskgroup()     // expected-warning {{extra tokens at the end of '#pragma omp cancellation point' are ignored}}
 #pragma omp cancellation point parallel, if    // expected-warning {{extra tokens at the end of '#pragma omp cancellation point' are ignored}}
   if (argc)
 #pragma omp cancellation point for // expected-error {{'#pragma omp cancellation point' cannot be an immediate substatement}}
     if (argc) {
 #pragma omp taskgroup