[PATCH] D74970: [OpenMP] Refactor the analysis in checkMapClauseBaseExpression using StmtVisitor clause.
ABataev added inline comments.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15356
+ Expr::EvalResult Result;
+ if (AE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext())) {
+if (!Result.Val.getInt().isNullValue()) {
cchen wrote:
> ABataev wrote:
> > Need to check that `AE->getIdx()` is not value dependent, otherwise it may
> > crash
> It seems Clang would catch the error before we do the analysis:
>
> ```
> orig.cpp:6:24: error: array subscript is not an integer
> #pragma omp target map(a[b])
>^ ~
> orig.cpp:15:3: note: in instantiation of function template specialization
> 'gg' requested here
> gg(a, c);
> ^
> orig.cpp:8:5: error: array subscript is not an integer
> a[b] = 10;
> ^ ~
> 2 errors generated.
> ```
>
> Also, if we still need it, do we also check type dependent?
1. Yes, it will find incorrect expression at the instantiation. But what if
you're working with the template? In this case, the expression can be
value-dependent.
2. No, no need to check for type-dependence here, a check for value-dependent
expression should be enough.
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 clause.
cchen marked an inline comment as done.
cchen added inline comments.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15356
+ Expr::EvalResult Result;
+ if (AE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext())) {
+if (!Result.Val.getInt().isNullValue()) {
ABataev wrote:
> Need to check that `AE->getIdx()` is not value dependent, otherwise it may
> crash
It seems Clang would catch the error before we do the analysis:
```
orig.cpp:6:24: error: array subscript is not an integer
#pragma omp target map(a[b])
^ ~
orig.cpp:15:3: note: in instantiation of function template specialization
'gg' requested here
gg(a, c);
^
orig.cpp:8:5: error: array subscript is not an integer
a[b] = 10;
^ ~
2 errors generated.
```
Also, if we still need it, do we also check type dependent?
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 clause.
ABataev added inline comments.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15238
+ Sema
+ OpenMPClauseKind CKind;
+ OMPClauseMappableExprCommon::MappableExprComponentList
Default initializer here
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15240
+ OMPClauseMappableExprCommon::MappableExprComponentList
+ bool NoDiagnose;
+ const Expr *RelevantExpr{nullptr};
Default initializer
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15241-15243
+ const Expr *RelevantExpr{nullptr};
+ bool AllowUnitySizeArraySection{true};
+ bool AllowWholeSizeArraySection{true};
Better to use ` = ;` here. Just a nit.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15258
+if (!isa(DRE->getDecl()))
+ return false;
+RelevantExpr = DRE;
`emitErrorMsg()` here?
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15266
+ bool VisitMemberExpr(MemberExpr *ME) {
+Expr *E = cast(ME);
+Expr *BaseE = ME->getBase()->IgnoreParenImpCasts();
No need `cast` here.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15267
+Expr *E = cast(ME);
+Expr *BaseE = ME->getBase()->IgnoreParenImpCasts();
Better to use `IgnoreParenCasts()` to handle explicit casting to the base class.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15283
+return false;
+ return Visit(E->IgnoreParenImpCasts());;
+}
cchen wrote:
> @ABataev, in the previous patch you mentioned that we don't need
> `IgnoreParenImpCasts()` here, but I think I was just trying to do what line
> 15232 did.
Just `Visit(E)`
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15299
+return false;
+ return Visit(E->IgnoreParenImpCasts());;
+}
Just `Visit(E)`
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15317
}
+ return Visit(E->IgnoreParenImpCasts());;
+}
Just `Visit(E)`
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15332
+Components.emplace_back(ME, FD);
+return RelevantExpr || Visit(E->IgnoreParenImpCasts());
+ }
Just `Visit(E)`
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15344
}
+ return Visit(E->IgnoreParenImpCasts());
+}
`Visit(E)`
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15354
- // Record the component.
- CurComponents.emplace_back(CurE, FD);
-} else if (auto *CurE = dyn_cast(E)) {
- E = CurE->getBase()->IgnoreParenImpCasts();
-
- if (!E->getType()->isAnyPointerType() && !E->getType()->isArrayType()) {
-if (!NoDiagnose) {
- SemaRef.Diag(ELoc, diag::err_omp_expected_base_var_name)
- << 0 << CurE->getSourceRange();
- return nullptr;
+if (const auto *TE = dyn_cast(E)) {
+ Expr::EvalResult Result;
`E->IgnoreParensCasts()`
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15356
+ Expr::EvalResult Result;
+ if (AE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext())) {
+if (!Result.Val.getInt().isNullValue()) {
Need to check that `AE->getIdx()` is not value dependent, otherwise it may crash
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15370
- if (const auto *TE = dyn_cast(E)) {
-Expr::EvalResult Result;
-if (CurE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext())) {
- if (!Result.Val.getInt().isNullValue()) {
-SemaRef.Diag(CurE->getIdx()->getExprLoc(),
- diag::err_omp_invalid_map_this_expr);
-SemaRef.Diag(CurE->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);
-} else if (auto *CurE = dyn_cast(E)) {
- assert(!NoDiagnose && "Array sections cannot be implicitly mapped.");
- E = CurE->getBase()->IgnoreParenImpCasts();
+return E && Visit(E->IgnoreParenImpCasts());
+ }
Just `Visit(E)`
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15419-15428
+ if (OASE->getLength()->EvaluateAsInt(ResultR,
+ SemaRef.getASTContext())) {
+if (!ResultR.Val.getInt().isOneValue()) {
+ SemaRef.Diag(OASE->getLength()->getExprLoc(),
+ diag::err_omp_invalid_map_this_expr);
+ SemaRef.Diag(OASE->getLength()->getExprLoc(),
+ diag::note_omp_invalid_length_on_this_ptr_mapping);
Also, need to be sure that the expressions here are not value-dependent.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15444-15473
[PATCH] D74970: [OpenMP] Refactor the analysis in checkMapClauseBaseExpression using StmtVisitor clause.
cchen marked an inline comment as done. cchen added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:15283 +return false; + return Visit(E->IgnoreParenImpCasts());; +} @ABataev, in the previous patch you mentioned that we don't need `IgnoreParenImpCasts()` here, but I think I was just trying to do what line 15232 did. 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 clause.
cchen created this revision.
cchen added a reviewer: ABataev.
Herald added subscribers: cfe-commits, guansong.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
This step is the preparation of allowing lvalue in map/motion clause.
Repository:
rG LLVM Github Monorepo
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,306 @@
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';
-
- const Expr *RelevantExpr = nullptr;
+// 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
+// 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;
+ OMPClauseMappableExprCommon::MappableExprComponentList
+ bool NoDiagnose;
+ const Expr *RelevantExpr{nullptr};
+ bool AllowUnitySizeArraySection{true};
+ bool AllowWholeSizeArraySection{true};
+ SourceLocation ELoc;
+ SourceRange ERange;
+
+ 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;
+}
+ }
- bool AllowUnitySizeArraySection = true;
- bool AllowWholeSizeArraySection = true;
+public:
+ bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+if (!isa(DRE->getDecl()))
+ return false;
+RelevantExpr = DRE;
+// Record the component.
+Components.emplace_back(DRE, DRE->getDecl());
+return true;
+ }
- while (!RelevantExpr) {
-E = E->IgnoreParenImpCasts();
+ bool VisitMemberExpr(MemberExpr *ME) {
+Expr *E = cast(ME);
+Expr *BaseE = ME->getBase()->IgnoreParenImpCasts();
-if (auto *CurE = dyn_cast(E)) {
- if (!isa(CurE->getDecl()))
-return nullptr;
+if (isa(BaseE))
+ // We found a base expression: this->Val.
+ RelevantExpr = ME;
+else
+ E = BaseE;
- RelevantExpr = CurE;
+if (!isa(ME->getMemberDecl())) {
+ if (!NoDiagnose) {
+SemaRef.Diag(ELoc, diag::err_omp_expected_access_to_data_field)
+ << ME->getSourceRange();
+return false;
+ }
+ if (RelevantExpr)
+return false;
+ return Visit(E->IgnoreParenImpCasts());;
+}
- // If we got a reference to a
