[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7711-7713
+                isa<OMPArraySectionExpr>(Next->getAssociatedExpression()) ||
+                isa<UnaryOperator>(Next->getAssociatedExpression()) ||
+                isa<ConditionalOperator>(Next->getAssociatedExpression())) &&
----------------
This must be updated, I assume. Instead need to check that it is an lvalue.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7829
         if (EncounteredME) {
-          const auto *FD = dyn_cast<FieldDecl>(EncounteredME->getMemberDecl());
-          unsigned FieldIndex = FD->getFieldIndex();
-
-          // Update info about the lowest and highest elements for this struct
-          if (!PartialStruct.Base.isValid()) {
-            PartialStruct.LowestElem = {FieldIndex, LB};
-            PartialStruct.HighestElem = {FieldIndex, LB};
-            PartialStruct.Base = BP;
-          } else if (FieldIndex < PartialStruct.LowestElem.first) {
-            PartialStruct.LowestElem = {FieldIndex, LB};
-          } else if (FieldIndex > PartialStruct.HighestElem.first) {
-            PartialStruct.HighestElem = {FieldIndex, LB};
+          // MemberExpr can also be FunctionDecl after we allow all lvalue
+          if (const auto *FD =
----------------
Here we should not have any functiondecls. Must be an assert.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15212
+    DRE = E;
+    const auto *VD = dyn_cast<VarDecl>(E->getDecl());
+    const auto *FD = dyn_cast<FieldDecl>(E->getDecl());
----------------
I don't think anything except for VarDecls must be allowed here.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15235
+  bool VisitUnaryOperator(UnaryOperator *UO) {
+    DerefCnt++;
+    CurComponents.emplace_back(UO, nullptr);
----------------
Need a check that this is a dereference op. Also, maybe allow using an addr_of operation?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15244-15245
+  }
+  // checkMapClauseExpressionBase has already added the CurComponents, and we
+  // didn't clean it up, so don't add it here
+  bool VisitArraySubscriptExpr(ArraySubscriptExpr *AE) {
----------------
Why you have all these comments here?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15267-15271
+    Expr *LE = BO->getLHS()->IgnoreParenImpCasts();
+    Expr *RE = BO->getRHS()->IgnoreParenImpCasts();
+    if (LE && Visit(LE) && RE && Visit(RE)) {
+      return true;
+    }
----------------
Some extra checks for the allowed operators are required.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15276
+    for (Stmt *Child : S->children()) {
+      if (Child && Visit(Child))
+        return true;
----------------
I would add a check that the child is and expression and not a glvalue, then do not analyze it.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15281
+  }
+  DeclRefExpr *GetDRE() const { return DRE; }
+  explicit LocatorChecker(
----------------
`getFoundBase` or something like this?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15283
+  explicit LocatorChecker(
+      OMPClauseMappableExprCommon::MappableExprComponentList &CurComponents)
+      : CurComponents(CurComponents), DerefCnt(0) {}
----------------
Why do you need components list here?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15347
+      if (!(isa<VarDecl>(CurE->getDecl()) ||
+            isa<FunctionDecl>(CurE->getDecl())))
         return nullptr;
----------------
FunctionDecls should not be allowed here


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15369
+      if (!isa<FieldDecl>(CurE->getMemberDecl()) &&
+          !isa<CXXMethodDecl>(CurE->getMemberDecl())) {
         if (!NoDiagnose) {
----------------
Same here, no function decls


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15535
     } else {
+      if (OrigExpr->IgnoreParenImpCasts()->isLValue()) {
+        LocatorChecker Checker(CurComponents);
----------------
Why are using `IgnoreParenImpCasts()` here? It drops implicit casts to RValue.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16007
+    if (!RE->IgnoreParenImpCasts()->isLValue() &&
+        (MLValue != clang::Expr::isModifiableLvalueResult::MLV_LValueCast)) {
       SemaRef.Diag(ELoc,
----------------
Why `MLV_LValueCast` must be allowed here?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16009
       SemaRef.Diag(ELoc,
                    diag::err_omp_expected_named_var_member_or_array_expression)
           << RE->getSourceRange();
----------------
Message should be updated too.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16057
     const auto *FD = dyn_cast<FieldDecl>(CurDeclaration);
+    const auto *FuncD = dyn_cast<FunctionDecl>(CurDeclaration);
 
----------------
Again, functiondecls must not be allowed here. I would suggest emitting a warning for them (that user mapped a function) a drop them from the mapping list.


================
Comment at: clang/test/OpenMP/target_teams_map_messages.cpp:437
 #pragma omp target data map(to x) // expected-error {{expected ',' or ')' in 'map' clause}}
-#pragma omp target data map(tofrom: argc > 0 ? x : y) // expected-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}}
+#pragma omp target data map(tofrom: argc > 0 ? x : y)
 #pragma omp target data map(argc)
----------------
Not sure that this must be supported. What is the base decl here, `x` or `y`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811