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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
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),
----------------
Do we really need to set `HasMapper` to `true` unconditionally here and in other places?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:841-842
+  /// Get the function for the specified user-defined mapper, if any.
+  virtual llvm::Function *
+  getUserDefinedMapperFunc(const OMPDeclareMapperDecl *D);
 
----------------
Is this required to make it `virtual`?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:1556
       return BasePointersArray && PointersArray && SizesArray &&
-             MapTypesArray && NumberOfPtrs;
+             MapTypesArray && MappersArray && NumberOfPtrs;
     }
----------------
`(!HasMapper || MappersArray)`?


================
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();
----------------
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.


================
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:
> 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.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7564-7573
   void generateInfoForComponentList(
-      OpenMPMapClauseKind MapType,
-      ArrayRef<OpenMPMapModifierKind> MapModifiers,
+      OpenMPMapClauseKind MapType, ArrayRef<OpenMPMapModifierKind> MapModifiers,
       OMPClauseMappableExprCommon::MappableExprComponentListRef Components,
       MapBaseValuesArrayTy &BasePointers, MapValuesArrayTy &Pointers,
       MapValuesArrayTy &Sizes, MapFlagsArrayTy &Types,
-      StructRangeInfoTy &PartialStruct, bool IsFirstComponentList,
-      bool IsImplicit,
+      MapMappersArrayTy &Mappers, StructRangeInfoTy &PartialStruct,
+      bool IsFirstComponentList, bool IsImplicit,
----------------
lildmh wrote:
> ABataev wrote:
> > Too many params in the function, worth thinking about wrapping them in a record.
> Maybe not in this patch?
It is better to make it in this patch or make a separate patch and commit it before this one. We try to resolve this as soon as possible.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8821-8822
+
+    // No user-defined mapper for default mapping.
+    CurMappers.push_back(nullptr);
   }
----------------
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.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8953-8955
+      llvm::Value *M = CGF.Builder.CreateConstInBoundsGEP2_32(
+          llvm::ArrayType::get(CGM.VoidPtrTy, Info.NumberOfPtrs),
+          Info.MappersArray, 0, I);
----------------
lildmh wrote:
> ABataev wrote:
> > Better to use `Builder.CreateConstArrayGEP` 
> I saw all the other places are using `CreateConstInBoundsGEP2_32` to get sizes, types, etc. So I kept it consistent here. Maybe these modifications should be in a future patch?
Some of the modifications were made before we had these new functions. Now, we have the new ones and if we can use it, better to switch to the new safer versions.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8956-8957
+          Info.MappersArray, 0, I);
+      M = CGF.Builder.CreatePointerBitCastOrAddrSpaceCast(
+          M, MFunc->getType()->getPointerTo(/*AddrSpace=*/0));
+      Address MAddr(M, Ctx.getTypeAlignInChars(Ctx.VoidPtrTy));
----------------
lildmh wrote:
> ABataev wrote:
> > Easier cast function to the `voidptr`, I think
> Again, this is to keep it consistent with previous code above?
No need to rely on the old code if can make something cleaner and better. Cast Mfunc to `voidptr` in the `if` statement and no need to make some extra casts later.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8991-8994
+      MappersArrayArg = CGF.Builder.CreateConstInBoundsGEP2_32(
+          llvm::ArrayType::get(CGM.VoidPtrTy, Info.NumberOfPtrs),
+          Info.MappersArray,
+          /*Idx0=*/0, /*Idx1=*/0);
----------------
lildmh wrote:
> ABataev wrote:
> > Hmm, just cast of the `Info.MappersArray` to the correct type if all indices are `0`?
> Again, this is to keep consistent with previous code?
Same, use the best code if we can use it.


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

https://reviews.llvm.org/D67833