[OpenMP] Refactor the analysis in checkMapClauseBaseExpression using StmtVisitor class.

Summary: This step is the preparation of allowing lvalue in map/motion clause.

Reviewers: ABataev, jdoerfert

Reviewed By: ABataev

Subscribers: guansong, cfe-commits

Tags: #clang, #openmp

Differential Revision: https://reviews.llvm.org/D74970

clang/lib/Sema/SemaOpenMP.cpp

@@ -15431,95 +15431,95 @@ static bool checkArrayExpressionDoesNotReferToUnitySize(Sema &SemaRef,
   return ConstLength.getSExtValue() != 1;
 }
 
-// Return the expression of the base of the mappable expression or null if it
+// The base of elements of list in a map clause have to be either:
-// cannot be determined and do all the necessary checks to see if the expression
+//  - a reference to variable or field.
-// is valid as a standalone mappable expression. In the process, record all the
+//  - a member expression.
-// components of the expression.
+//  - an array expression.
-static const Expr *checkMapClauseExpressionBase(
+//
-    Sema &SemaRef, Expr *E,
+// E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
-    OMPClauseMappableExprCommon::MappableExprComponentList &CurComponents,
+// reference to 'r'.
-    OpenMPClauseKind CKind, bool NoDiagnose) {
+//
-  SourceLocation ELoc = E->getExprLoc();
+// If we have:
-  SourceRange ERange = E->getSourceRange();
+//
-
+// struct SS {
-  // The base of elements of list in a map clause have to be either:
+//   Bla S;
-  //  - a reference to variable or field.
+//   foo() {
-  //  - a member expression.
+//     #pragma omp target map (S.Arr[:12]);
-  //  - an array expression.
+//   }
-  //
+// }
-  // E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
+//
-  // reference to 'r'.
+// We want to retrieve the member expression 'this->S';
-  //
-  // If we have:
-  //
-  // struct SS {
-  //   Bla S;
-  //   foo() {
-  //     #pragma omp target map (S.Arr[:12]);
-  //   }
-  // }
-  //
-  // We want to retrieve the member expression 'this->S';
 
+// OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, p.2]
+//  If a list item is an array section, it must specify contiguous storage.
+//
+// For this restriction it is sufficient that we make sure only references
+// to variables or fields and array expressions, and that no array sections
+// exist except in the rightmost expression (unless they cover the whole
+// dimension of the array). E.g. these would be invalid:
+//
+//   r.ArrS[3:5].Arr[6:7]
+//
+//   r.ArrS[3:5].x
+//
+// but these would be valid:
+//   r.ArrS[3].Arr[6:7]
+//
+//   r.ArrS[3].x
+namespace {
+class MapBaseChecker final : public StmtVisitor<MapBaseChecker, bool> {
+  Sema &SemaRef;
+  OpenMPClauseKind CKind = OMPC_unknown;
+  OMPClauseMappableExprCommon::MappableExprComponentList &Components;
+  bool NoDiagnose = false;
   const Expr *RelevantExpr = nullptr;
-
-  // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, p.2]
-  //  If a list item is an array section, it must specify contiguous storage.
-  //
-  // For this restriction it is sufficient that we make sure only references
-  // to variables or fields and array expressions, and that no array sections
-  // exist except in the rightmost expression (unless they cover the whole
-  // dimension of the array). E.g. these would be invalid:
-  //
-  //   r.ArrS[3:5].Arr[6:7]
-  //
-  //   r.ArrS[3:5].x
-  //
-  // but these would be valid:
-  //   r.ArrS[3].Arr[6:7]
-  //
-  //   r.ArrS[3].x
-
   bool AllowUnitySizeArraySection = true;
   bool AllowWholeSizeArraySection = true;
+  SourceLocation ELoc;
+  SourceRange ERange;
 
-  while (!RelevantExpr) {
+  void emitErrorMsg() {
-    E = E->IgnoreParenImpCasts();
+    if (!NoDiagnose) {
-
+      // If nothing else worked, this is not a valid map clause expression.
-    if (auto *CurE = dyn_cast<DeclRefExpr>(E)) {
+      SemaRef.Diag(ELoc, diag::err_omp_expected_named_var_member_or_array_expression)
-      if (!isa<VarDecl>(CurE->getDecl()))
+        << ERange;
-        return nullptr;
+    }
-
+  }
-      RelevantExpr = CurE;
-
-      // If we got a reference to a declaration, we should not expect any array
-      // section before that.
-      AllowUnitySizeArraySection = false;
-      AllowWholeSizeArraySection = false;
 
+public:
+  bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+    if (!isa<VarDecl>(DRE->getDecl())) {
+      emitErrorMsg();
+      return false;
+    }
+    RelevantExpr = DRE;
     // Record the component.
-      CurComponents.emplace_back(CurE, CurE->getDecl());
+    Components.emplace_back(DRE, DRE->getDecl());
-    } else if (auto *CurE = dyn_cast<MemberExpr>(E)) {
+    return true;
-      Expr *BaseE = CurE->getBase()->IgnoreParenImpCasts();
+  }
+
+  bool VisitMemberExpr(MemberExpr *ME) {
+    Expr *E = ME;
+    Expr *BaseE = ME->getBase()->IgnoreParenCasts();
 
     if (isa<CXXThisExpr>(BaseE))
       // We found a base expression: this->Val.
-        RelevantExpr = CurE;
+      RelevantExpr = ME;
     else
       E = BaseE;
 
-      if (!isa<FieldDecl>(CurE->getMemberDecl())) {
+    if (!isa<FieldDecl>(ME->getMemberDecl())) {
       if (!NoDiagnose) {
         SemaRef.Diag(ELoc, diag::err_omp_expected_access_to_data_field)
-              << CurE->getSourceRange();
+          << ME->getSourceRange();
-          return nullptr;
+        return false;
       }
       if (RelevantExpr)
-          return nullptr;
+        return false;
-        continue;
+      return Visit(E);
     }
 
-      auto *FD = cast<FieldDecl>(CurE->getMemberDecl());
+    auto *FD = cast<FieldDecl>(ME->getMemberDecl());
 
     // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, C/C++, p.3]
     //  A bit-field cannot appear in a map clause.
@@ -15527,12 +15527,12 @@ static const Expr *checkMapClauseExpressionBase(
     if (FD->isBitField()) {
       if (!NoDiagnose) {
         SemaRef.Diag(ELoc, diag::err_omp_bit_fields_forbidden_in_clause)
-              << CurE->getSourceRange() << getOpenMPClauseName(CKind);
+          << ME->getSourceRange() << getOpenMPClauseName(CKind);
-          return nullptr;
+        return false;
       }
       if (RelevantExpr)
-          return nullptr;
+        return false;
-        continue;
+      return Visit(E);
     }
 
     // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, C++, p.1]
@@ -15547,10 +15547,10 @@ static const Expr *checkMapClauseExpressionBase(
     if (CurType->isUnionType()) {
       if (!NoDiagnose) {
         SemaRef.Diag(ELoc, diag::err_omp_union_type_not_allowed)
-              << CurE->getSourceRange();
+          << ME->getSourceRange();
-          return nullptr;
+        return false;
       }
-        continue;
+      return RelevantExpr || Visit(E);
     }
 
     // If we got a member expression, we should not expect any array section
@@ -15564,45 +15564,51 @@ static const Expr *checkMapClauseExpressionBase(
     AllowWholeSizeArraySection = false;
 
     // Record the component.
-      CurComponents.emplace_back(CurE, FD);
+    Components.emplace_back(ME, FD);
-    } else if (auto *CurE = dyn_cast<ArraySubscriptExpr>(E)) {
+    return RelevantExpr || Visit(E);
-      E = CurE->getBase()->IgnoreParenImpCasts();
+  }
+
+  bool VisitArraySubscriptExpr(ArraySubscriptExpr *AE) {
+    Expr *E = AE->getBase()->IgnoreParenImpCasts();
 
     if (!E->getType()->isAnyPointerType() && !E->getType()->isArrayType()) {
       if (!NoDiagnose) {
         SemaRef.Diag(ELoc, diag::err_omp_expected_base_var_name)
-              << 0 << CurE->getSourceRange();
+          << 0 << AE->getSourceRange();
-          return nullptr;
+        return false;
       }
-        continue;
+      return RelevantExpr || Visit(E);
     }
 
     // If we got an array subscript that express the whole dimension we
     // can have any array expressions before. If it only expressing part of
     // the dimension, we can only have unitary-size array expressions.
-      if (checkArrayExpressionDoesNotReferToWholeSize(SemaRef, CurE,
+    if (checkArrayExpressionDoesNotReferToWholeSize(SemaRef, AE,
                                                     E->getType()))
       AllowWholeSizeArraySection = false;
 
-      if (const auto *TE = dyn_cast<CXXThisExpr>(E)) {
+    if (const auto *TE = dyn_cast<CXXThisExpr>(E->IgnoreParenCasts())) {
       Expr::EvalResult Result;
-        if (CurE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext())) {
+      if (!AE->getIdx()->isValueDependent() &&
-          if (!Result.Val.getInt().isNullValue()) {
+          AE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext()) &&
-            SemaRef.Diag(CurE->getIdx()->getExprLoc(),
+          !Result.Val.getInt().isNullValue()) {
+        SemaRef.Diag(AE->getIdx()->getExprLoc(),
                      diag::err_omp_invalid_map_this_expr);
-            SemaRef.Diag(CurE->getIdx()->getExprLoc(),
+        SemaRef.Diag(AE->getIdx()->getExprLoc(),
                      diag::note_omp_invalid_subscript_on_this_ptr_map);
       }
-        }
       RelevantExpr = TE;
     }
 
     // Record the component - we don't have any declaration associated.
-      CurComponents.emplace_back(CurE, nullptr);
+    Components.emplace_back(AE, nullptr);
-    } else if (auto *CurE = dyn_cast<OMPArraySectionExpr>(E)) {
-      assert(!NoDiagnose && "Array sections cannot be implicitly mapped.");
-      E = CurE->getBase()->IgnoreParenImpCasts();
 
+    return RelevantExpr || Visit(E);
+  }
+
+  bool VisitOMPArraySectionExpr(OMPArraySectionExpr *OASE) {
+    assert(!NoDiagnose && "Array sections cannot be implicitly mapped.");
+    Expr *E = OASE->getBase()->IgnoreParenImpCasts();
     QualType CurType =
       OMPArraySectionExpr::getBaseOriginalType(E).getCanonicalType();
 
@@ -15616,14 +15622,14 @@ static const Expr *checkMapClauseExpressionBase(
 
     if (!IsPointer && !CurType->isArrayType()) {
       SemaRef.Diag(ELoc, diag::err_omp_expected_base_var_name)
-            << 0 << CurE->getSourceRange();
+        << 0 << OASE->getSourceRange();
-        return nullptr;
+      return false;
     }
 
     bool NotWhole =
-          checkArrayExpressionDoesNotReferToWholeSize(SemaRef, CurE, CurType);
+      checkArrayExpressionDoesNotReferToWholeSize(SemaRef, OASE, CurType);
     bool NotUnity =
-          checkArrayExpressionDoesNotReferToUnitySize(SemaRef, CurE, CurType);
+      checkArrayExpressionDoesNotReferToUnitySize(SemaRef, OASE, CurType);
 
     if (AllowWholeSizeArraySection) {
       // Any array section is currently allowed. Allowing a whole size array
@@ -15639,48 +15645,68 @@ static const Expr *checkMapClauseExpressionBase(
       // compatible with the properties of the current array section.
       SemaRef.Diag(
         ELoc, diag::err_array_section_does_not_specify_contiguous_storage)
-            << CurE->getSourceRange();
+        << OASE->getSourceRange();
-        return nullptr;
+      return false;
     }
 
     if (const auto *TE = dyn_cast<CXXThisExpr>(E)) {
       Expr::EvalResult ResultR;
       Expr::EvalResult ResultL;
-        if (CurE->getLength()->EvaluateAsInt(ResultR,
+      if (!OASE->getLength()->isValueDependent() &&
-                                             SemaRef.getASTContext())) {
+          OASE->getLength()->EvaluateAsInt(ResultR, SemaRef.getASTContext()) &&
-          if (!ResultR.Val.getInt().isOneValue()) {
+          !ResultR.Val.getInt().isOneValue()) {
-            SemaRef.Diag(CurE->getLength()->getExprLoc(),
+        SemaRef.Diag(OASE->getLength()->getExprLoc(),
                      diag::err_omp_invalid_map_this_expr);
-            SemaRef.Diag(CurE->getLength()->getExprLoc(),
+        SemaRef.Diag(OASE->getLength()->getExprLoc(),
                      diag::note_omp_invalid_length_on_this_ptr_mapping);
       }
-        }
+      if (OASE->getLowerBound() && !OASE->getLowerBound()->isValueDependent() &&
-        if (CurE->getLowerBound() && CurE->getLowerBound()->EvaluateAsInt(
+          OASE->getLowerBound()->EvaluateAsInt(ResultL,
-                                        ResultL, SemaRef.getASTContext())) {
+                                               SemaRef.getASTContext()) &&
-          if (!ResultL.Val.getInt().isNullValue()) {
+          !ResultL.Val.getInt().isNullValue()) {
-            SemaRef.Diag(CurE->getLowerBound()->getExprLoc(),
+        SemaRef.Diag(OASE->getLowerBound()->getExprLoc(),
                      diag::err_omp_invalid_map_this_expr);
-            SemaRef.Diag(CurE->getLowerBound()->getExprLoc(),
+        SemaRef.Diag(OASE->getLowerBound()->getExprLoc(),
                      diag::note_omp_invalid_lower_bound_on_this_ptr_mapping);
       }
-        }
       RelevantExpr = TE;
     }
 
     // Record the component - we don't have any declaration associated.
-      CurComponents.emplace_back(CurE, nullptr);
+    Components.emplace_back(OASE, nullptr);
-    } else {
+    return RelevantExpr || Visit(E);
-      if (!NoDiagnose) {
-        // If nothing else worked, this is not a valid map clause expression.
-        SemaRef.Diag(
-            ELoc, diag::err_omp_expected_named_var_member_or_array_expression)
-            << ERange;
   }
-      return nullptr;
+  bool VisitStmt(Stmt *) {
+    emitErrorMsg();
+    return false;
   }
-  }
+  const Expr *getFoundBase() const {
-
     return RelevantExpr;
+  }
+  explicit MapBaseChecker(
+      Sema &SemaRef, OpenMPClauseKind CKind,
+      OMPClauseMappableExprCommon::MappableExprComponentList &Components,
+      bool NoDiagnose, SourceLocation &ELoc, SourceRange &ERange)
+      : SemaRef(SemaRef), CKind(CKind), Components(Components),
+        NoDiagnose(NoDiagnose), ELoc(ELoc), ERange(ERange) {}
+};
+} // namespace
+
+/// Return the expression of the base of the mappable expression or null if it
+/// cannot be determined and do all the necessary checks to see if the expression
+/// is valid as a standalone mappable expression. In the process, record all the
+/// components of the expression.
+static const Expr *checkMapClauseExpressionBase(
+    Sema &SemaRef, Expr *E,
+    OMPClauseMappableExprCommon::MappableExprComponentList &CurComponents,
+    OpenMPClauseKind CKind, bool NoDiagnose) {
+  SourceLocation ELoc = E->getExprLoc();
+  SourceRange ERange = E->getSourceRange();
+  MapBaseChecker Checker(SemaRef, CKind, CurComponents, NoDiagnose, ELoc,
+                         ERange);
+  if (Checker.Visit(E->IgnoreParenImpCasts()))
+    return Checker.getFoundBase();
+  return nullptr;
 }
 
 // Return true if expression E associated with value VD has conflicts with other

clang/test/OpenMP/target_messages.cpp

@@ -50,6 +50,12 @@ class S {
       int b;
     #pragma omp target map(this[1]) // expected-note {{expected 'this' subscript expression on map clause to be 'this[0]'}} // expected-error {{invalid 'this' expression on 'map' clause}}
       int c;
+    #pragma omp target map(foo) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+      int d;
+    #pragma omp target map(zee) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+      int e;
+    #pragma omp target map(this->zee) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+      int f;
   }
 };
 
@@ -110,6 +116,14 @@ int main(int argc, char **argv) {
   #pragma omp target
   for (int n = 0; n < 100; ++n) {}
 
+  #pragma omp target map(foo) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+  {}
+
+  S s;
+
+  #pragma omp target map(s.zee) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+  {}
+
   return 0;
 }