[PATCH] D67833: [OpenMP 5.0] Codegen support to pass user-defined mapper functions to runtime

[Lingda Li via Phabricator via cfe-commits]

lildmh marked 5 inline comments as done.
lildmh added a comment.

Alexey, thanks for the review



================
Comment at: clang/include/clang/AST/OpenMPClause.h:4918
                         const OMPMappableExprListSizeTy &Sizes)
-      : OMPMappableExprListClause(OMPC_map, Locs, Sizes, &MapperQualifierLoc,
-                                  &MapperIdInfo),
+      : OMPMappableExprListClause(OMPC_map, Locs, Sizes, /*HasMapper=*/true,
+                                  &MapperQualifierLoc, &MapperIdInfo),
----------------
ABataev wrote:
> Do we really need to set `HasMapper` to `true` unconditionally here and in other places?
For `map`, `to`, and `from` clauses, they can have mappers, so it's set to `true`. For `is_device_ptr` and `use_device_ptr`, it's set to `false`.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7388
 
-  llvm::Value *getExprTypeSize(const Expr *E) const {
+  llvm::Value *getExprTypeSize(const Expr *E, bool hasMapper) const {
     QualType ExprTy = E->getType().getCanonicalType();
----------------
ABataev wrote:
> ABataev wrote:
> > lildmh wrote:
> > > ABataev wrote:
> > > > ABataev wrote:
> > > > > CamelCase
> > > > Seems to me, with mapper you're changing the contract of this function. It is supposed to return the size in bytes, with Mapper it returns just number of elements. Bad decision. Better to convert size in bytes to number of elements in place where you need it.
> > > Yes. The reason why I did this is I found this is the single place that I can consider all situations. Otherwise, I need to divide the size by the element size in other places. Since this function has discussed all situations for the element size, I will need to duplicate the work elsewhere if I don't do it here. The latter solution is not good for code mountainous I think.
> > Changing the original function contract is also not the best solution. Better to introduce the new function. If some of the code can be reused, need to factor out the common code and reuse it in the old function and the new one.
> Also, additional question. Does it mean that we pass to the runtime functions different values in different situations: in one case we pass size in bytes, with mappers - number of elements? If so, it is a very bad idea caused by bad design. We need to pass either size in bytes, or number of elements in this argument.
Yes, the current design is like this. Let me see how to make it better


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8821-8822
+
+    // No user-defined mapper for default mapping.
+    CurMappers.push_back(nullptr);
   }
----------------
ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > In general, there should not be branch for default mapping during the codegen, the implicit map clauses must be generated in sema.
> > Not sure where this code will be used. I guess it's still correct to add a null to the mappers here?
> I mean, that in general, it would be good to see here some kind of an assertion. Almost all situations must be resolved in Sema.
Okay, I guess it's not the work of this patch


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

https://reviews.llvm.org/D67833