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

[Lingda Li via Phabricator via cfe-commits]

lildmh added a comment.

I'll split the patch into 2 later



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


================
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,
----------------
ABataev wrote:
> Too many params in the function, worth thinking about wrapping them in a record.
Maybe not in this patch?


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


================
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);
----------------
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?


================
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));
----------------
ABataev wrote:
> Easier cast function to the `voidptr`, I think
Again, this is to keep it consistent with previous code above?


================
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);
----------------
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?


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

https://reviews.llvm.org/D67833