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

[Ethan Stewart via Phabricator via cfe-commits]

estewart08 added inline comments.
Herald added subscribers: sstefan1, yaxunl.
Herald added a project: All.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15450
   }
+  bool VisitCXXThisExpr(CXXThisExpr *CTE) { return true; }
+  bool VisitStmt(Stmt *) {

cchen wrote:
> ABataev wrote:
> > Do you really need this function?
> Removed the function.
Was this function intended to be removed? As far as I can tell it was not and 
it seems to be the source of an issue I am having:

expected addressable lvalue in 'map' clause


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd66d25f83824: [OpenMP] Refactor the analysis in 
checkMapClauseBaseExpression using… (authored by cchen, committed by ABataev).

Changed prior to commit:
  https://reviews.llvm.org/D74970?vs=245995=246214#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_messages.cpp

Index: clang/test/OpenMP/target_messages.cpp
===
--- clang/test/OpenMP/target_messages.cpp
+++ clang/test/OpenMP/target_messages.cpp
@@ -50,6 +50,12 @@
   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 @@
   #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;
 }
 
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15431,256 +15431,282 @@
   return ConstLength.getSExtValue() != 1;
 }
 
-// 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 , Expr *E,
-OMPClauseMappableExprCommon::MappableExprComponentList ,
-OpenMPClauseKind CKind, bool NoDiagnose) {
-  SourceLocation ELoc = E->getExprLoc();
-  SourceRange ERange = E->getSourceRange();
-
-  // The base of elements of list in a map clause have to be either:
-  //  - a reference to variable or field.
-  //  - a member expression.
-  //  - an array expression.
-  //
-  // E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
-  // reference to 'r'.
-  //
-  // If we have:
-  //
-  // struct SS {
-  //   Bla S;
-  //   foo() {
-  // #pragma omp target map (S.Arr[:12]);
-  //   }
-  // }
-  //
-  // We want to retrieve the member expression 'this->S';
+// The base of elements of list in a map clause have to be either:
+//  - a reference to variable or field.
+//  - a member expression.
+//  - an array expression.
+//
+// E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
+// reference to 'r'.
+//
+// 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 {
+  Sema 
+  OpenMPClauseKind CKind = OMPC_unknown;
+  OMPClauseMappableExprCommon::MappableExprComponentList 
+  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:
-  //   

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Sure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added a comment.

@ABataev, can you land it for me when you have time? Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chi Chun Chen via Phabricator via cfe-commits]

cchen updated this revision to Diff 245995.
cchen added a comment.

Update test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_messages.cpp

Index: clang/test/OpenMP/target_messages.cpp
===
--- clang/test/OpenMP/target_messages.cpp
+++ clang/test/OpenMP/target_messages.cpp
@@ -50,6 +50,12 @@
   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 @@
   #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;
 }
 #endif
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15197,256 +15197,282 @@
   return ConstLength.getSExtValue() != 1;
 }
 
-// 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 , Expr *E,
-OMPClauseMappableExprCommon::MappableExprComponentList ,
-OpenMPClauseKind CKind, bool NoDiagnose) {
-  SourceLocation ELoc = E->getExprLoc();
-  SourceRange ERange = E->getSourceRange();
-
-  // The base of elements of list in a map clause have to be either:
-  //  - a reference to variable or field.
-  //  - a member expression.
-  //  - an array expression.
-  //
-  // E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
-  // reference to 'r'.
-  //
-  // If we have:
-  //
-  // struct SS {
-  //   Bla S;
-  //   foo() {
-  // #pragma omp target map (S.Arr[:12]);
-  //   }
-  // }
-  //
-  // We want to retrieve the member expression 'this->S';
+// The base of elements of list in a map clause have to be either:
+//  - a reference to variable or field.
+//  - a member expression.
+//  - an array expression.
+//
+// E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
+// reference to 'r'.
+//
+// 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 {
+  Sema 
+  OpenMPClauseKind CKind = OMPC_unknown;
+  OMPClauseMappableExprCommon::MappableExprComponentList 
+  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) {
-E = 

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D74970#1887337 , @cchen wrote:

> Fix test and sorry for my carelessness


No problem. But I don't see the changes in the test.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:15473
+ ERange);
+  if (Checker.Visit(E->IgnoreParenImpCasts())) {
+return Checker.getFoundBase();

Remove braces here, the substatement is single line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chi Chun Chen via Phabricator via cfe-commits]

cchen updated this revision to Diff 245983.
cchen added a comment.

Fix test and sorry for my carelessness


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_messages.cpp

Index: clang/test/OpenMP/target_messages.cpp
===
--- clang/test/OpenMP/target_messages.cpp
+++ clang/test/OpenMP/target_messages.cpp
@@ -50,6 +50,12 @@
   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}}
+  foo();
+#pragma omp target map(zee()) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+  zee();
+#pragma omp target map(this->zee()) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+  this->zee();
   }
 };
 
@@ -110,6 +116,14 @@
   #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}}
+  foo();
+
+  S s;
+
+  #pragma omp target map(s.zee()) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+  s.zee();
+
   return 0;
 }
 #endif
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15197,256 +15197,283 @@
   return ConstLength.getSExtValue() != 1;
 }
 
-// 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 , Expr *E,
-OMPClauseMappableExprCommon::MappableExprComponentList ,
-OpenMPClauseKind CKind, bool NoDiagnose) {
-  SourceLocation ELoc = E->getExprLoc();
-  SourceRange ERange = E->getSourceRange();
-
-  // The base of elements of list in a map clause have to be either:
-  //  - a reference to variable or field.
-  //  - a member expression.
-  //  - an array expression.
-  //
-  // E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
-  // reference to 'r'.
-  //
-  // If we have:
-  //
-  // struct SS {
-  //   Bla S;
-  //   foo() {
-  // #pragma omp target map (S.Arr[:12]);
-  //   }
-  // }
-  //
-  // We want to retrieve the member expression 'this->S';
+// The base of elements of list in a map clause have to be either:
+//  - a reference to variable or field.
+//  - a member expression.
+//  - an array expression.
+//
+// E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
+// reference to 'r'.
+//
+// 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 {
+  Sema 
+  OpenMPClauseKind CKind = OMPC_unknown;
+  OMPClauseMappableExprCommon::MappableExprComponentList 
+  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;
+  

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15358-15360
+  if (!AE->getIdx()->isValueDependent())
+if (AE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext())) {
   if (!Result.Val.getInt().isNullValue()) {

Simplify this nested ifs too, please.



Comment at: clang/test/OpenMP/target_messages.cpp:53-57
+#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}}

You mapped `CallExpr`s here, not `DeclRef`s



Comment at: clang/test/OpenMP/target_messages.cpp:119-125
+  #pragma omp target map(foo()) // expected-error {{expected expression 
containing only member accesses and/or array sections based on named variables}}
+  foo();
+
+  S s;
+
+  #pragma omp target map(s.zee()) // expected-error {{expected expression 
containing only member accesses and/or array sections based on named variables}}
+  foo();

same here, mapped CallExprs


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chi Chun Chen via Phabricator via cfe-commits]

cchen updated this revision to Diff 245978.
cchen added a comment.

Add test for mapping function


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_messages.cpp

Index: clang/test/OpenMP/target_messages.cpp
===
--- clang/test/OpenMP/target_messages.cpp
+++ clang/test/OpenMP/target_messages.cpp
@@ -50,6 +50,12 @@
   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 @@
   #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}}
+  foo();
+
+  S s;
+
+  #pragma omp target map(s.zee()) // expected-error {{expected expression containing only member accesses and/or array sections based on named variables}}
+  foo();
+
   return 0;
 }
 #endif
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15197,256 +15197,284 @@
   return ConstLength.getSExtValue() != 1;
 }
 
-// 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 , Expr *E,
-OMPClauseMappableExprCommon::MappableExprComponentList ,
-OpenMPClauseKind CKind, bool NoDiagnose) {
-  SourceLocation ELoc = E->getExprLoc();
-  SourceRange ERange = E->getSourceRange();
-
-  // The base of elements of list in a map clause have to be either:
-  //  - a reference to variable or field.
-  //  - a member expression.
-  //  - an array expression.
-  //
-  // E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
-  // reference to 'r'.
-  //
-  // If we have:
-  //
-  // struct SS {
-  //   Bla S;
-  //   foo() {
-  // #pragma omp target map (S.Arr[:12]);
-  //   }
-  // }
-  //
-  // We want to retrieve the member expression 'this->S';
+// The base of elements of list in a map clause have to be either:
+//  - a reference to variable or field.
+//  - a member expression.
+//  - an array expression.
+//
+// E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
+// reference to 'r'.
+//
+// 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 {
+  Sema 
+  OpenMPClauseKind CKind = OMPC_unknown;
+  OMPClauseMappableExprCommon::MappableExprComponentList 
+  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;
 
-  

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

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added a comment.

In D74970#1887252 , @ABataev wrote:

> In D74970#1887246 , @cchen wrote:
>
> > In D74970#1887193 , @ABataev wrote:
> >
> > > Did you try to run the tests? I would suggest adding a test, at least, 
> > > where a function is mapped. We should see the error message here. Seems 
> > > to me, we don't have this one.
> >
> >
> > We already have test for `err_omp_invalid_map_this_expr`, 
> > `note_omp_invalid_subscript_on_this_ptr_map`, etc.. in target_message.cpp 
> > line 44 without checking for value dependence. Do you mean that I should 
> > add a test for the check of value dependence? In that case, we don't print 
> > any messages.
>
>
> No. But previously, if we tried to map a function, not a variable, it would 
> be skipped silently. With this patch, this behavior will change. I suggest 
> adding a test with mapping a function to check that error message is emitted. 
> Something like:
>
>   int foo();
>   #pragma omp target map(foo) // <- must be an error here
>
>
> And I just asked if you tried to run the tests with this patch. Did the 
> result change or it is the same?


Got it, I'll add test for function mapping. I've run the test and this patch 
passed all the test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D74970#1887246 , @cchen wrote:

> In D74970#1887193 , @ABataev wrote:
>
> > Did you try to run the tests? I would suggest adding a test, at least, 
> > where a function is mapped. We should see the error message here. Seems to 
> > me, we don't have this one.
>
>
> We already have test for `err_omp_invalid_map_this_expr`, 
> `note_omp_invalid_subscript_on_this_ptr_map`, etc.. in target_message.cpp 
> line 44 without checking for value dependence. Do you mean that I should add 
> a test for the check of value dependence? In that case, we don't print any 
> messages.


No. But previously, if we tried to map a function, not a variable, it would be 
skipped silently. With this patch, this behavior will change. I suggest adding 
a test with mapping a function to check that error message is emitted. 
Something like:

  int foo();
  #pragma omp target map(foo) // <- must be an error here

And I just asked if you tried to run the tests with this patch. Did the result 
change or it is the same?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added a comment.

In D74970#1887193 , @ABataev wrote:

> Did you try to run the tests? I would suggest adding a test, at least, where 
> a function is mapped. We should see the error message here. Seems to me, we 
> don't have this one.


We already have test for `err_omp_invalid_map_this_expr`, 
`note_omp_invalid_subscript_on_this_ptr_map`, etc.. in target_message.cpp line 
44 without checking for value dependence. Do you mean that I should add a test 
for the check of value dependence? In that case, we don't print any messages.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Did you try to run the tests? I would suggest adding a test, at least, where a 
function is mapped. We should see the error message here. Seems to me, we don't 
have this one.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:15422-15423
+  Expr::EvalResult ResultL;
+  if (!OASE->getLength()->isValueDependent()) {
+if (OASE->getLength()->EvaluateAsInt(ResultR,
  SemaRef.getASTContext())) {

Just a nit. Try to reduce the structural complexity here by using logical 
exressions, `if (!OASE->getLength()->isValueDependent() && 
OASE->getLength()->EvaluateAsInt(...) && !ResultR.Val.getInt().isOneValue())`. 
Also, you can 



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15433-15436
+  if (OASE->getLowerBound() && !OASE->getLowerBound()->isValueDependent()) 
{
+if (OASE->getLowerBound()->EvaluateAsInt(
+ ResultL, SemaRef.getASTContext())) {
   if (!ResultL.Val.getInt().isNullValue()) {

Same here, try to reduce complexity


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chi Chun Chen via Phabricator via cfe-commits]

cchen updated this revision to Diff 245955.
cchen added a comment.

Fix by feedback


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970

Files:
  clang/lib/Sema/SemaOpenMP.cpp

Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15197,256 +15197,290 @@
   return ConstLength.getSExtValue() != 1;
 }
 
-// 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 , Expr *E,
-OMPClauseMappableExprCommon::MappableExprComponentList ,
-OpenMPClauseKind CKind, bool NoDiagnose) {
-  SourceLocation ELoc = E->getExprLoc();
-  SourceRange ERange = E->getSourceRange();
-
-  // The base of elements of list in a map clause have to be either:
-  //  - a reference to variable or field.
-  //  - a member expression.
-  //  - an array expression.
-  //
-  // E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
-  // reference to 'r'.
-  //
-  // If we have:
-  //
-  // struct SS {
-  //   Bla S;
-  //   foo() {
-  // #pragma omp target map (S.Arr[:12]);
-  //   }
-  // }
-  //
-  // We want to retrieve the member expression 'this->S';
+// The base of elements of list in a map clause have to be either:
+//  - a reference to variable or field.
+//  - a member expression.
+//  - an array expression.
+//
+// E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
+// reference to 'r'.
+//
+// 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 {
+  Sema 
+  OpenMPClauseKind CKind = OMPC_unknown;
+  OMPClauseMappableExprCommon::MappableExprComponentList 
+  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) {
-E = E->IgnoreParenImpCasts();
-
-if (auto *CurE = dyn_cast(E)) {
-  if (!isa(CurE->getDecl()))
-return nullptr;
-
-  RelevantExpr = CurE;
+  void emitErrorMsg() {
+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;
+}
+  }
 
-  // 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(DRE->getDecl())) {
+  emitErrorMsg();
+  return false;
+}
+RelevantExpr = DRE;
+// Record the component.
+Components.emplace_back(DRE, DRE->getDecl());
+return true;
+  }
 
-  // Record the component.
-  CurComponents.emplace_back(CurE, CurE->getDecl());
-} else if (auto *CurE = dyn_cast(E)) {
-  Expr *BaseE = CurE->getBase()->IgnoreParenImpCasts();
+  bool VisitMemberExpr(MemberExpr *ME) {
+Expr *E = ME;
+Expr *BaseE = ME->getBase()->IgnoreParenCasts();
 
-  if (isa(BaseE))
-// We found a base expression: this->Val.
-RelevantExpr = CurE;
-  else
-E = BaseE;
+if (isa(BaseE))
+  // We found a base expression: this->Val.
+  RelevantExpr = ME;
+else
+  

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

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked 11 inline comments as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15450
   }
+  bool VisitCXXThisExpr(CXXThisExpr *CTE) { return true; }
+  bool VisitStmt(Stmt *) {

ABataev wrote:
> Do you really need this function?
Removed the function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15319
   }
+  return Visit(E);
+}

`return RelevantExpr || Visit(E);`?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15346
   }
+  return Visit(E);
+}

`return RelevantExpr || Visit(E);`?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15358-15359
+  Expr::EvalResult Result;
+  if (AE->isValueDependent())
+return false;
+  if (AE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext())) {

1. Need to check `AE->getIdx()`, not `AE`.
2. Why return `false` here? I would say, just skip the check.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15374
 
-  QualType CurType =
-  OMPArraySectionExpr::getBaseOriginalType(E).getCanonicalType();
+return E && Visit(E);
+  }

`return RelevantExpr || Visit(E);`?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15423-15424
+  Expr::EvalResult ResultL;
+  if (OASE->isValueDependent())
+return false;
+  if (OASE->getLength()->EvaluateAsInt(ResultR,

1. Need to check `OASE->getLength()`, not `OASE`.
2. Why return `false` here? I would say, just skip the check.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15434
   }
-
-  // Record the component - we don't have any declaration associated.
-  CurComponents.emplace_back(CurE, nullptr);
-} else {
-  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;
+  if (OASE->getLowerBound() && OASE->getLowerBound()->EvaluateAsInt(
+  ResultL, SemaRef.getASTContext())) {

Need a check for value-dependent `OASE->getLowerBound()`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15448
+Components.emplace_back(OASE, nullptr);
+return RelevantExpr || Visit(E->IgnoreParenImpCasts());
   }

Just `Visit(E)`;



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15450
   }
+  bool VisitCXXThisExpr(CXXThisExpr *CTE) { return true; }
+  bool VisitStmt(Stmt *) {

Do you really need this function?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15467-15470
+// 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.

Use `\\\` style of comment here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chi Chun Chen via Phabricator via cfe-commits]

cchen updated this revision to Diff 245912.
cchen added a comment.

Fix based on feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970

Files:
  clang/lib/Sema/SemaOpenMP.cpp

Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15197,256 +15197,289 @@
   return ConstLength.getSExtValue() != 1;
 }
 
-// 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 , Expr *E,
-OMPClauseMappableExprCommon::MappableExprComponentList ,
-OpenMPClauseKind CKind, bool NoDiagnose) {
-  SourceLocation ELoc = E->getExprLoc();
-  SourceRange ERange = E->getSourceRange();
-
-  // The base of elements of list in a map clause have to be either:
-  //  - a reference to variable or field.
-  //  - a member expression.
-  //  - an array expression.
-  //
-  // E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
-  // reference to 'r'.
-  //
-  // If we have:
-  //
-  // struct SS {
-  //   Bla S;
-  //   foo() {
-  // #pragma omp target map (S.Arr[:12]);
-  //   }
-  // }
-  //
-  // We want to retrieve the member expression 'this->S';
+// The base of elements of list in a map clause have to be either:
+//  - a reference to variable or field.
+//  - a member expression.
+//  - an array expression.
+//
+// E.g. if we have the expression 'r.S.Arr[:12]', we want to retrieve the
+// reference to 'r'.
+//
+// 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 {
+  Sema 
+  OpenMPClauseKind CKind = OMPC_unknown;
+  OMPClauseMappableExprCommon::MappableExprComponentList 
+  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) {
-E = E->IgnoreParenImpCasts();
-
-if (auto *CurE = dyn_cast(E)) {
-  if (!isa(CurE->getDecl()))
-return nullptr;
-
-  RelevantExpr = CurE;
+  void emitErrorMsg() {
+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;
+}
+  }
 
-  // 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(DRE->getDecl())) {
+  emitErrorMsg();
+  return false;
+}
+RelevantExpr = DRE;
+// Record the component.
+Components.emplace_back(DRE, DRE->getDecl());
+return true;
+  }
 
-  // Record the component.
-  CurComponents.emplace_back(CurE, CurE->getDecl());
-} else if (auto *CurE = dyn_cast(E)) {
-  Expr *BaseE = CurE->getBase()->IgnoreParenImpCasts();
+  bool VisitMemberExpr(MemberExpr *ME) {
+Expr *E = ME;
+Expr *BaseE = ME->getBase()->IgnoreParenCasts();
 
-  if (isa(BaseE))
-// We found a base expression: this->Val.
-RelevantExpr = CurE;
-  else
-E = BaseE;
+if (isa(BaseE))
+  // We found a base expression: this->Val.
+  RelevantExpr = ME;
+else