[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'"
----------------
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.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7616
           dyn_cast<OMPArraySectionExpr>(I->getAssociatedExpression());
+      const auto *UO = dyn_cast<UnaryOperator>(I->getAssociatedExpression());
       bool IsPointer =
----------------
cchen wrote:
> ABataev wrote:
> > What about binary operator?
> Didn't add check here since I didn't add any binary operator into Component in the MapBaseChecker class. Should I add it?
I think so. Otherwise those binary expressions are meaningless. Or treated in some unexpected way.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15698
+  }
+  bool VisitBinaryOperator(BinaryOperator *BO) {
+    if (SemaRef.getLangOpts().OpenMP < 50) {
----------------
cchen wrote:
> ABataev wrote:
> > Better to discuss the implementation for this in a separate patch, it really requires some additional analysis and extra work.
> Why should we do this in another patch? The only thing this patch does is extending for pointer arithmetic (doing analysis on BinOp).
You're doing some checks in this patch  using counting of '*' and '[' in this patch. This is definitely not correct.  That's why I suggest to move it in a separate patch and discus the solution there and do not block all other kinds of expressions.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15712
+    const std::string RTS = RT->getCanonicalTypeInternal().getAsString();
+    auto CntLayer = [](char c) { return c == '*' || c == '['; };
+    size_t LLayerCnt = std::count_if(LTS.begin(), LTS.end(), CntLayer);
----------------
cchen wrote:
> ABataev wrote:
> > Again, bad idea to count this stuff.
> Will it be better if just check if the subtree is an offset? So that we only need to check if it does not have any decorator in type?
What do you mean? Exclude, say, RHS or LHS from the analysis, if it is a complex expression and you can use another part as a base? That's worth trying, at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75077