[PATCH] D75077: [OpenMP5.0] Allow pointer arithmetic in motion/map clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9965
   "memory order clause '%0' is specified here">;
+def err_omp_non_lvalue_in_map_or_motion_clauses: Error<
+  "expected lvalue with no function call in '#pragma omp target update' and '#pragma omp target map'"
----------------
ABataev wrote:
> I would say that this lvalue must be addressable, no? Also, some function calls can be handled, probably, those returning rvalues, for example, or constexprs.
Fix this message, please, it is not quite correct.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7616-7633
+      const auto *UO = dyn_cast<UnaryOperator>(I->getAssociatedExpression());
+      const auto *BO = dyn_cast<BinaryOperator>(I->getAssociatedExpression());
       bool IsPointer =
           (OASE && OMPArraySectionExpr::getBaseOriginalType(OASE)
                        .getCanonicalType()
                        ->isAnyPointerType()) ||
           I->getAssociatedExpression()->getType()->isAnyPointerType();
----------------
Do you have the tests for these changes?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7751
+                 "Unexpected FunctionDecl");
           const auto *FD = dyn_cast<FieldDecl>(EncounteredME->getMemberDecl());
           unsigned FieldIndex = FD->getFieldIndex();
----------------
Change this line to just `cast<FieldDecl>`. In this case the assert is not required.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15695-15696
+    }
+    Expr *SubE = UO->getSubExpr()->IgnoreParenImpCasts();
+    return RelevantExpr || Visit(SubE);
+  }
----------------
I think, here we should have `return RelevantExpr || Visit(UO->getSubExpr())` without dropping parens and implicit casts. Again, we may drop some important implicit casts


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15703-15706
+    // Pointer arithmetic is the only thing we expect to happen
+    // here so we only need to visit the subtree that has the
+    // same type as root (so that we know the other subtree
+    // is just an offset)
----------------
Add a check here that the type of the expression must be a pointer type.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15715-15717
+    } else {
+      return Visit(RE);
+    }
----------------
Just `return Visit(RE);` without `else`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15718
+    }
+    return false;
+  }
----------------
Unreachable code


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15683
   }
+  bool VisitUnaryOperator(UnaryOperator *UO) {
+    if (SemaRef.getLangOpts().OpenMP < 50) {
----------------
cchen wrote:
> ABataev wrote:
> > cchen wrote:
> > > ABataev wrote:
> > > > I think you need to extend this to support paren expressions, at least.
> > > I have `UO->getSubExpr()->IgnoreParenImpCasts()` here, or I might not understand what you mean by "support paren expression".
> > `(*(..))` is not supported, I think. Or, maybe, `*((a)+b)`
> Both cases are handled correctly. For `(*((b)+a))`, the binop in `VisitBinaryOperator()`does not include the outer paren and for `*((a)+b)`, the parens can be handled by `Expr *SubE = UO->getSubExpr()->IgnoreParenImpCasts();`.
I think we need to change the way we check the expression in call for `MapBaseChecker::Visit` in `checkMapClauseExpressionBase` or extend the check in the `checkMapClauseExpressionBase`. `E->IgnoreParenImpCasts()` may drop implicit lvalue-to-rvalue cast and we may allow mapping of rvalues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75077